On Thu, 24 Mar 2022 at 15:24, Bin Meng <bmeng...@gmail.com> wrote:
>
> On Sun, Mar 20, 2022 at 7:41 PM Andrew Scull <asc...@google.com> wrote:
> >
> > Ensure the virtio PCI capabilities are contained within the bounds of
> > the device's configuration space. The expected size of the capability is
> > passed when searching for the capability to enforce this check.
> >
> > Signed-off-by: Andrew Scull <asc...@google.com>
> > ---
> > drivers/virtio/virtio_pci_modern.c | 23 +++++++++++++++++++----
> > 1 file changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_pci_modern.c
> > b/drivers/virtio/virtio_pci_modern.c
> > index 3403ff5cca..4b346be257 100644
> > --- a/drivers/virtio/virtio_pci_modern.c
> > +++ b/drivers/virtio/virtio_pci_modern.c
> > @@ -392,18 +392,30 @@ static int virtio_pci_notify(struct udevice *udev,
> > struct virtqueue *vq)
> > *
> > * @udev: the transport device
> > * @cfg_type: the VIRTIO_PCI_CAP_* value we seek
> > + * @cap_size: expected size of the capability
> > *
> > * Return: offset of the configuration structure
> > */
> > -static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type)
> > +static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
> > + size_t cap_size)
> > {
> > int pos;
> > int offset;
> > u8 type, bar;
> >
> > + if (cap_size < sizeof(struct virtio_pci_cap))
> > + return 0;
> > +
> > + if (cap_size > PCI_CFG_SPACE_SIZE)
> > + return 0;
> > +
>
> The above 2 checks are not necessary as this helper is local to this
> driver, and we know the callers do things correctly.
The checks aren't absolutely necessary but they do help to document
the constraints and catch any future problems. I'm not sure of the
philosophy the u-boot applies but I like the idea of the safeguards
being in place.
> > for (pos = dm_pci_find_capability(udev, PCI_CAP_ID_VNDR);
> > pos > 0;
> > pos = dm_pci_find_next_capability(udev, pos, PCI_CAP_ID_VNDR))
> > {
> > + /* Ensure the capability is within bounds */
> > + if (PCI_CFG_SPACE_SIZE - cap_size < pos)
> > + return 0;
> > +
> > offset = pos + offsetof(struct virtio_pci_cap, cfg_type);
> > dm_pci_read_config8(udev, offset, &type);
> > offset = pos + offsetof(struct virtio_pci_cap, bar);
> > @@ -491,7 +503,8 @@ static int virtio_pci_probe(struct udevice *udev)
> > uc_priv->vendor = subvendor;
> >
> > /* Check for a common config: if not, use legacy mode (bar 0) */
> > - common = virtio_pci_find_capability(udev,
> > VIRTIO_PCI_CAP_COMMON_CFG);
> > + common = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_COMMON_CFG,
> > + sizeof(struct virtio_pci_cap));
> > if (!common) {
> > printf("(%s): leaving for legacy driver\n", udev->name);
> > return -ENODEV;
> > @@ -505,7 +518,8 @@ static int virtio_pci_probe(struct udevice *udev)
> > }
> >
> > /* If common is there, notify should be too */
> > - notify = virtio_pci_find_capability(udev,
> > VIRTIO_PCI_CAP_NOTIFY_CFG);
> > + notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG,
> > + sizeof(struct
> > virtio_pci_notify_cap));
> > if (!notify) {
> > printf("(%s): missing capabilities %i/%i\n", udev->name,
> > common, notify);
> > @@ -519,7 +533,8 @@ static int virtio_pci_probe(struct udevice *udev)
> > * Device capability is only mandatory for devices that have
> > * device-specific configuration.
> > */
> > - device = virtio_pci_find_capability(udev,
> > VIRTIO_PCI_CAP_DEVICE_CFG);
> > + device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG,
> > + sizeof(struct virtio_pci_cap));
> > if (device) {
> > offset = device + offsetof(struct virtio_pci_cap, length);
> > dm_pci_read_config32(udev, offset, &priv->device_len);
> > --
>
> Regards,
> Bin
