On Fri, Mar 25, 2022 at 12:27 AM Andrew Scull <asc...@google.com> wrote:
>
> 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.
I would use assert() for such case.
>
> > > 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
