Hello Colin Xu,
The patch 49ba1a2976c8: "vfio/pci: Add OpRegion 2.0+ Extended VBT
support." from Oct 12, 2021, leads to the following Smatch static
checker warning:
drivers/vfio/pci/vfio_pci_igd.c:101 vfio_pci_igd_rw()
warn: potential pointer math issue ('&version' is a 16 bit pointer)
drivers/vfio/pci/vfio_pci_igd.c:124 vfio_pci_igd_rw()
warn: potential pointer math issue ('&rvda' is a 64 bit pointer)
drivers/vfio/pci/vfio_pci_igd.c
64 static ssize_t vfio_pci_igd_rw(struct vfio_pci_core_device *vdev,
65 char __user *buf, size_t count, loff_t *ppos,
66 bool iswrite)
67 {
68 unsigned int i = VFIO_PCI_OFFSET_TO_INDEX(*ppos) - VFIO_PCI_NUM_REGIONS;
69 struct igd_opregion_vbt *opregionvbt = vdev->region[i].data;
70 loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK, off = 0;
71 size_t remaining;
72
73 if (pos >= vdev->region[i].size || iswrite)
74 return -EINVAL;
75
76 count = min_t(size_t, count, vdev->region[i].size - pos);
77 remaining = count;
78
79 /* Copy until OpRegion version */
80 if (remaining && pos < OPREGION_VERSION) {
81 size_t bytes = min_t(size_t, remaining, OPREGION_VERSION - pos);
82
83 if (igd_opregion_shift_copy(buf, &off,
84 opregionvbt->opregion + pos, &pos,
85 &remaining, bytes))
86 return -EFAULT;
87 }
88
89 /* Copy patched (if necessary) OpRegion version */
90 if (remaining && pos < OPREGION_VERSION + sizeof(__le16)) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The sizeof() means we know "pos" is a number of bytes.
91 size_t bytes = min_t(size_t, remaining,
92 OPREGION_VERSION + sizeof(__le16) - pos);
93 __le16 version = *(__le16 *)(opregionvbt->opregion +
^^^^^^^^^^^^^^
Version is a stack variable. I think version was supposed to be a
pointer.
u8 *v = opregionvbt->opregion + OPREGION_VERSION;
__le16 version = *(__le16 *)v;
94 OPREGION_VERSION);
95
96 /* Patch to 2.1 if OpRegion 2.0 has extended VBT */
97 if (le16_to_cpu(version) == 0x0200 && opregionvbt->vbt_ex)
98 version = cpu_to_le16(0x0201);
99
100 if (igd_opregion_shift_copy(buf, &off,
--> 101 &version + (pos - OPREGION_VERSION),
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The math doesn't work because we're changing from byte units to
sizeof(version) units, but the more important question is why are we
copying stack data anyway? :P
if (igd_opregion_shift_copy(buf, &off,
p + (pos - OPREGION_VERSION),
&pos, &remaining, bytes))
102 &pos, &remaining, bytes))
103 return -EFAULT;
104 }
105
106 /* Copy until RVDA */
107 if (remaining && pos < OPREGION_RVDA) {
108 size_t bytes = min_t(size_t, remaining, OPREGION_RVDA - pos);
109
110 if (igd_opregion_shift_copy(buf, &off,
111 opregionvbt->opregion + pos, &pos,
112 &remaining, bytes))
113 return -EFAULT;
114 }
115
116 /* Copy modified (if necessary) RVDA */
117 if (remaining && pos < OPREGION_RVDA + sizeof(__le64)) {
118 size_t bytes = min_t(size_t, remaining,
119 OPREGION_RVDA + sizeof(__le64) - pos);
120 __le64 rvda = cpu_to_le64(opregionvbt->vbt_ex ?
121 OPREGION_SIZE : 0);
122
123 if (igd_opregion_shift_copy(buf, &off,
124 &rvda + (pos - OPREGION_RVDA),
Same thing here. The pointer math is wrong and copying stack data is
wrong too.
125 &pos, &remaining, bytes))
126 return -EFAULT;
127 }
128
129 /* Copy the rest of OpRegion */
130 if (remaining && pos < OPREGION_SIZE) {
131 size_t bytes = min_t(size_t, remaining, OPREGION_SIZE - pos);
132
133 if (igd_opregion_shift_copy(buf, &off,
134 opregionvbt->opregion + pos, &pos,
135 &remaining, bytes))
136 return -EFAULT;
137 }
138
139 /* Copy extended VBT if exists */
140 if (remaining &&
141 copy_to_user(buf + off, opregionvbt->vbt_ex + (pos - OPREGION_SIZE),
142 remaining))
143 return -EFAULT;
144
145 *ppos += count;
146
147 return count;
148 }
regards,
dan carpenter
