Re: [PATCH 05/11] virtio: pci: Check virtio capability is in bounds

Bin Meng
2022-03-24T15:24:27Z

On Sun, Mar 20, 2022 at 7:41 PM Andrew Scull <ascull@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 <ascull@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.

> 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