On Tue, 10 Oct 2023 14:41:39 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> On Tue, Oct 10, 2023 at 11:11 AM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> >
> > Some buggy devices, the common cfg size may not match the features.
> >
> > This patch checks the common cfg size for the
> > features(VIRTIO_F_NOTIF_CONFIG_DATA, VIRTIO_F_RING_RESET). When the
> > common cfg size does not match the corresponding feature, we fail the
> > probe and print error message.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> > ---
> > drivers/virtio/virtio_pci_modern.c | 36 ++++++++++++++++++++++++++
> > drivers/virtio/virtio_pci_modern_dev.c | 2 +-
> > include/linux/virtio_pci_modern.h | 1 +
> > 3 files changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > index d6bb68ba84e5..6a8f5ff05636 100644
> > --- a/drivers/virtio/virtio_pci_modern.c
> > +++ b/drivers/virtio/virtio_pci_modern.c
> > @@ -39,6 +39,39 @@ static void vp_transport_features(struct virtio_device *vdev, u64 features)
> > __virtio_set_bit(vdev, VIRTIO_F_RING_RESET);
> > }
> >
> > +static int __vp_check_common_size_one_feature(struct virtio_device *vdev, u32 fbit,
> > + u32 offset, const char *fname)
> > +{
> > + struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > +
> > + if (!__virtio_test_bit(vdev, fbit))
> > + return 0;
> > +
> > + if (likely(vp_dev->mdev.common_len >= offset))
> > + return 0;
> > +
> > + dev_err(&vdev->dev,
> > + "virtio: common cfg size(%ld) does not match the feature %s\n",
> > + vp_dev->mdev.common_len, fname);
> > +
> > + return -EINVAL;
> > +}
> > +
> > +#define vp_check_common_size_one_feature(vdev, fbit, field) \
> > + __vp_check_common_size_one_feature(vdev, fbit, \
> > + offsetofend(struct virtio_pci_modern_common_cfg, field), #fbit)
> > +
> > +static int vp_check_common_size(struct virtio_device *vdev)
> > +{
> > + if (vp_check_common_size_one_feature(vdev, VIRTIO_F_NOTIF_CONFIG_DATA, queue_notify_data))
> > + return -EINVAL;
> > +
> > + if (vp_check_common_size_one_feature(vdev, VIRTIO_F_RING_RESET, queue_reset))
> > + return -EINVAL;
>
> Do we need to at least check the offset of the queue_device as well here?
Not need.
/*
* vp_modern_map_capability - map a part of virtio pci capability
* @mdev: the modern virtio-pci device
* @off: offset of the capability
* @minlen: minimal length of the capability
* @align: align requirement
* @start: start from the capability
* @size: map size
* @len: the length that is actually mapped
* @pa: physical address of the capability
*
* Returns the io address of for the part of the capability
*/
static void __iomem *
vp_modern_map_capability(struct virtio_pci_modern_device *mdev, int off,
size_t minlen, u32 align, u32 start, u32 size,
size_t *len, resource_size_t *pa)
caller:
mdev->common = vp_modern_map_capability(mdev, common,
sizeof(struct virtio_pci_common_cfg), 4,
0, sizeof(struct virtio_pci_modern_common_cfg),
&mdev->common_len, NULL);
We pass the sizeof(struct virtio_pci_common_cfg) as the minlen.
So we do not need to check the common cfg size is smaller then
sizeof(struct virtio_pci_common_cfg).
Thanks.
>
> Thanks
>
> > +
> > + return 0;
> > +}
> > +
> > /* virtio config->finalize_features() implementation */
> > static int vp_finalize_features(struct virtio_device *vdev)
> > {
> > @@ -57,6 +90,9 @@ static int vp_finalize_features(struct virtio_device *vdev)
> > return -EINVAL;
> > }
> >
> > + if (vp_check_common_size(vdev))
> > + return -EINVAL;
> > +
> > vp_modern_set_features(&vp_dev->mdev, vdev->features);
> >
> > return 0;
> > diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
> > index 9cb601e16688..33f319da1558 100644
> > --- a/drivers/virtio/virtio_pci_modern_dev.c
> > +++ b/drivers/virtio/virtio_pci_modern_dev.c
> > @@ -292,7 +292,7 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
> > mdev->common = vp_modern_map_capability(mdev, common,
> > sizeof(struct virtio_pci_common_cfg), 4,
> > 0, sizeof(struct virtio_pci_modern_common_cfg),
> > - NULL, NULL);
> > + &mdev->common_len, NULL);
> > if (!mdev->common)
> > goto err_map_common;
> > mdev->isr = vp_modern_map_capability(mdev, isr, sizeof(u8), 1,
> > diff --git a/include/linux/virtio_pci_modern.h b/include/linux/virtio_pci_modern.h
> > index 067ac1d789bc..edf62bae0474 100644
> > --- a/include/linux/virtio_pci_modern.h
> > +++ b/include/linux/virtio_pci_modern.h
> > @@ -28,6 +28,7 @@ struct virtio_pci_modern_device {
> > /* So we can sanity-check accesses. */
> > size_t notify_len;
> > size_t device_len;
> > + size_t common_len;
> >
> > /* Capability for when we need to map notifications per-vq. */
> > int notify_map_cap;
> > --
> > 2.32.0.3.g01195cf9f
> >
>
> _______________________________________________
> Virtualization mailing list
> Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
