Re: [PATCH v2] virtio: pci: sanity check bar indexes

On Wed, Mar 23, 2022 at 01:21:55PM +0000, Keir Fraser wrote:

On Wed, Mar 23, 2022 at 08:01:42AM -0400, Michael S. Tsirkin wrote:

quoted

On Wed, Mar 23, 2022 at 03:57:59PM +0800, Jason Wang wrote:

quoted

On Tue, Mar 22, 2022 at 11:20 PM Keir Fraser [off-list ref] wrote:

quoted

The bar index is used as an index into the device's resource list
and should be checked as within range for a standard bar.

Also clean up an existing check to consistently use PCI_STD_NUM_BARS.

Signed-off-by: Keir Fraser <redacted>
---
 drivers/virtio/virtio_pci_modern.c     | 10 ++++++++--
 drivers/virtio/virtio_pci_modern_dev.c |  8 +++++++-
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 5455bc041fb6..84bace98dff5 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c

@@ -293,7 +293,7 @@ static int virtio_pci_find_shm_cap(struct pci_dev *dev, u8 required_id,

        for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR); pos > 0;
             pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
-               u8 type, cap_len, id;
+               u8 type, cap_len, id, res_bar;
                u32 tmp32;
                u64 res_offset, res_length;

@@ -317,7 +317,12 @@ static int virtio_pci_find_shm_cap(struct pci_dev *dev, u8 required_id,

                /* Type, and ID match, looks good */
                pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap,
-                                                        bar), bar);
+                                                        bar), &res_bar);
+               if (res_bar >= PCI_STD_NUM_BARS) {
+                       dev_err(&dev->dev, "%s: shm cap with bad bar: %d\n",
+                               __func__, res_bar);
+                       continue;
+               }

                /* Read the lower 32bit of length and offset */
                pci_read_config_dword(dev, pos + offsetof(struct virtio_pci_cap,

In fact, the spec says such BAR values are reserved, not bad, so
the capabiluty should be ignored, they should not cause the driver to error out
or print errors.

Ah yes, so I see. It makes sense then to silently ignore the capability and print nothing.
I will fix it.

quoted

quoted

quoted

@@ -337,6 +342,7 @@ static int virtio_pci_find_shm_cap(struct pci_dev *dev, u8 required_id,
                                                     length_hi), &tmp32);
                res_length |= ((u64)tmp32) << 32;

+               *bar = res_bar;
                *offset = res_offset;
                *len = res_length;

diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
index e8b3ff2b9fbc..a6911d1e212a 100644
--- a/drivers/virtio/virtio_pci_modern_dev.c
+++ b/drivers/virtio/virtio_pci_modern_dev.c

@@ -35,6 +35,12 @@ vp_modern_map_capability(struct virtio_pci_modern_device *mdev, int off,
        pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, length),
                              &length);

+       if (bar >= PCI_STD_NUM_BARS) {
+               dev_err(&dev->dev,
+                       "virtio_pci: bad capability bar %u\n", bar);

In fact, I would say the issue is less that bar is reserved.
The real issue is that the value apparently changed since
we read it the first time. I think it's a good idea to
reflect that in the message. Maybe find_capability should return
the capability structure so we don't need to re-read it from
the device?

I will have a look and fix it up one way or the other, and respin
this patch.

 Thanks,
  Keir

BTW avoiding extra reads is good for start up speed. This is slow path,
but still.

quoted

quoted

quoted

+               return NULL;
+       }
+
        if (length <= start) {
                dev_err(&dev->dev,
                        "virtio_pci: bad capability len %u (>%u expected)\n",

@@ -120,7 +126,7 @@ static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
                                     &bar);

                /* Ignore structures with reserved BAR values */
-               if (bar > 0x5)
+               if (bar >= PCI_STD_NUM_BARS)
                        continue;

Just notice that the spec said:

"
values 0x0 to 0x5 specify a Base Address register (BAR) belonging to
the function located beginning at 10h in PCI Configuration Space and
used to map the structure into Memory or I/O Space. The BAR is
permitted to be either 32-bit or 64-bit, it can map Memory Space or
I/O Space.

Any other value is reserved for future use.
"
So we probably need to stick 0x5 instead of 0x6 (PCI_STD_NUM_BARS) for
this and other places.

Thanks

It does not matter much IMHO, the reason spec uses 0 to 0x5 is precisely
because that's the standard number of BARs. Both ways work as long as we
are consistent, and I guess PCI_STD_NUM_BARS might be preferable since
people tend to copy paste values.

quoted

quoted

                if (type == cfg_type) {
--
2.35.1.894.gb6a874cedc-goog

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization