[bug report] vfio/pci: Add OpRegion 2.0+ Extended VBT support.

[bug report] vfio/pci: Add OpRegion 2.0+ Extended VBT support.

[Dan Carpenter]

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