* [bug report] vfio/pci: Add OpRegion 2.0+ Extended VBT support.
@ 2021-10-14 12:26 Dan Carpenter
0 siblings, 0 replies; only message in thread
From: Dan Carpenter @ 2021-10-14 12:26 UTC (permalink / raw)
To: colin.xu; +Cc: kvm
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
^ permalink raw reply [flat|nested] only message in thread
only message in thread, other threads:[~2021-10-14 12:26 UTC | newest]
Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-14 12:26 [bug report] vfio/pci: Add OpRegion 2.0+ Extended VBT support Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.