[PATCH 06/11] virtio: pci: Read entire capability into memory
Andrew Scull
ascull at google.com
Fri Mar 25 08:03:44 CET 2022
On Fri, 25 Mar 2022 at 04:31, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> On Sun, Mar 20, 2022 at 7:42 PM Andrew Scull <ascull at google.com> wrote:
> >
> > Read the virtio PCI capability out of the device configuration space to
> > a struct rather than accessing fields directly from the configuration
> > space as they are needed. This both makes access to the fields easier
> > and avoids re-reading fields.
> >
> > Re-reading fields could result in time-of-check to time-of-use problems,
> > should the value in the configuration space change. The range check of
> > the `bar` field and the later call to `dm_pci_read_bar32()` is an
> > example of where this could happen.
>
> I don't see the need to avoid the time-of-check to time-of-use
> problems, as it can only happen with the PCI configuration access
> capability, which U-Boot driver does not touch.
>
> Am I missing something?
U-Boot doesn't touch the configuration space but the device could
have, whether that be accidently or maliciously. Linux has taken
similar precautions [1] to add more safety checks and I'll be looking
to do the same in other parts of the u-boot virtio drivers.
[1] -- https://lwn.net/Articles/865216
> >
> > Signed-off-by: Andrew Scull <ascull at google.com>
> > ---
> > drivers/virtio/virtio_pci_modern.c | 74 ++++++++++++++++--------------
> > 1 file changed, 40 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > index 4b346be257..38a0da0a84 100644
> > --- a/drivers/virtio/virtio_pci_modern.c
> > +++ b/drivers/virtio/virtio_pci_modern.c
> > @@ -393,15 +393,16 @@ 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
> > + * @cap: capability read from the config space
> > *
> > * Return: offset of the configuration structure
> > */
> > static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
> > - size_t cap_size)
> > + size_t cap_size,
> > + struct virtio_pci_cap *cap)
> > {
> > int pos;
> > int offset;
> > - u8 type, bar;
> >
> > if (cap_size < sizeof(struct virtio_pci_cap))
> > return 0;
> > @@ -409,6 +410,9 @@ static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
> > if (cap_size > PCI_CFG_SPACE_SIZE)
> > return 0;
> >
> > + if (!cap)
> > + return 0;
> > +
> > 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)) {
> > @@ -416,16 +420,26 @@ static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
> > if (PCI_CFG_SPACE_SIZE - cap_size < pos)
> > return 0;
> >
> > + offset = pos + offsetof(struct virtio_pci_cap, cap_vndr);
> > + dm_pci_read_config8(udev, offset, &cap->cap_vndr);
> > + offset = pos + offsetof(struct virtio_pci_cap, cap_next);
> > + dm_pci_read_config8(udev, offset, &cap->cap_next);
> > + offset = pos + offsetof(struct virtio_pci_cap, cap_len);
> > + dm_pci_read_config8(udev, offset, &cap->cap_len);
> > offset = pos + offsetof(struct virtio_pci_cap, cfg_type);
> > - dm_pci_read_config8(udev, offset, &type);
> > + dm_pci_read_config8(udev, offset, &cap->cfg_type);
> > offset = pos + offsetof(struct virtio_pci_cap, bar);
> > - dm_pci_read_config8(udev, offset, &bar);
> > + dm_pci_read_config8(udev, offset, &cap->bar);
> > + offset = pos + offsetof(struct virtio_pci_cap, offset);
> > + dm_pci_read_config32(udev, offset, &cap->offset);
> > + offset = pos + offsetof(struct virtio_pci_cap, length);
> > + dm_pci_read_config32(udev, offset, &cap->length);
> >
> > /* Ignore structures with reserved BAR values */
> > - if (bar > 0x5)
> > + if (cap->bar > 0x5)
> > continue;
> >
> > - if (type == cfg_type)
> > + if (cap->cfg_type == cfg_type)
> > return pos;
> > }
> >
> > @@ -436,33 +450,27 @@ static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
> > * virtio_pci_map_capability - map base address of the capability
> > *
> > * @udev: the transport device
> > - * @off: offset of the configuration structure
> > + * @cap: capability to map
> > *
> > * Return: base address of the capability
> > */
> > -static void __iomem *virtio_pci_map_capability(struct udevice *udev, int off)
> > +static void __iomem *virtio_pci_map_capability(struct udevice *udev,
> > + const struct virtio_pci_cap *cap)
> > {
> > - u8 bar;
> > - u32 offset;
> > ulong base;
> > void __iomem *p;
> >
> > - if (!off)
> > + if (!cap)
> > return NULL;
> >
> > - offset = off + offsetof(struct virtio_pci_cap, bar);
> > - dm_pci_read_config8(udev, offset, &bar);
> > - offset = off + offsetof(struct virtio_pci_cap, offset);
> > - dm_pci_read_config32(udev, offset, &offset);
> > -
> > /*
> > * TODO: adding 64-bit BAR support
> > *
> > * Per spec, the BAR is permitted to be either 32-bit or 64-bit.
> > * For simplicity, only read the BAR address as 32-bit.
> > */
> > - base = dm_pci_read_bar32(udev, bar);
> > - p = (void __iomem *)base + offset;
> > + base = dm_pci_read_bar32(udev, cap->bar);
> > + p = (void __iomem *)base + cap->offset;
> >
> > return p;
> > }
> > @@ -487,7 +495,7 @@ static int virtio_pci_probe(struct udevice *udev)
> > u16 subvendor;
> > u8 revision;
> > int common, notify, device;
> > - u32 common_length;
> > + struct virtio_pci_cap common_cap, notify_cap, device_cap;
> > int offset;
> >
> > /* We only own devices >= 0x1040 and <= 0x107f: leave the rest. */
> > @@ -504,46 +512,44 @@ static int virtio_pci_probe(struct udevice *udev)
> >
> > /* Check for a common config: if not, use legacy mode (bar 0) */
> > common = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_COMMON_CFG,
> > - sizeof(struct virtio_pci_cap));
> > + sizeof(struct virtio_pci_cap),
> > + &common_cap);
> > if (!common) {
> > printf("(%s): leaving for legacy driver\n", udev->name);
> > return -ENODEV;
> > }
> >
> > - offset = common + offsetof(struct virtio_pci_cap, length);
> > - dm_pci_read_config32(udev, offset, &common_length);
> > - if (common_length < sizeof(struct virtio_pci_common_cfg)) {
> > + if (common_cap.length < sizeof(struct virtio_pci_common_cfg)) {
> > printf("(%s): virtio common config too small\n", udev->name);
> > return -EINVAL;
> > }
> >
> > /* If common is there, notify should be too */
> > notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG,
> > - sizeof(struct virtio_pci_notify_cap));
> > + sizeof(struct virtio_pci_notify_cap),
> > + ¬ify_cap);
> > if (!notify) {
> > printf("(%s): missing capabilities %i/%i\n", udev->name,
> > common, notify);
> > return -EINVAL;
> > }
> >
> > - offset = notify + offsetof(struct virtio_pci_cap, length);
> > - dm_pci_read_config32(udev, offset, &priv->notify_len);
> > + priv->notify_len = notify_cap.length;
> >
> > /*
> > * Device capability is only mandatory for devices that have
> > * device-specific configuration.
> > */
> > 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);
> > - }
> > + sizeof(struct virtio_pci_cap),
> > + &device_cap);
> > + if (device)
> > + priv->device_len = device_cap.length;
> >
> > /* Map configuration structures */
> > - priv->common = virtio_pci_map_capability(udev, common);
> > - priv->notify_base = virtio_pci_map_capability(udev, notify);
> > - priv->device = virtio_pci_map_capability(udev, device);
> > + priv->common = virtio_pci_map_capability(udev, &common_cap);
> > + priv->notify_base = virtio_pci_map_capability(udev, ¬ify_cap);
> > + priv->device = virtio_pci_map_capability(udev, &device_cap);
> > debug("(%p): common @ %p, notify base @ %p, device @ %p\n",
> > udev, priv->common, priv->notify_base, priv->device);
> >
> > --
>
> Regards,
> Bin
More information about the U-Boot
mailing list