Xuan Zhuo
2023-Oct-10 03:11 UTC
[PATCH vhost v3 3/4] virtio_pci: add check for common cfg size
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 at linux.alibaba.com>
---
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;
+
+ 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
Jason Wang
2023-Oct-10 06:41 UTC
[PATCH vhost v3 3/4] virtio_pci: add check for common cfg size
On Tue, Oct 10, 2023 at 11:11?AM Xuan Zhuo <xuanzhuo at linux.alibaba.com> 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 at linux.alibaba.com> > --- > 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? 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 >
Michael S. Tsirkin
2023-Oct-18 20:07 UTC
[PATCH vhost v3 3/4] virtio_pci: add check for common cfg size
On Tue, Oct 10, 2023 at 11:11:19AM +0800, Xuan Zhuo 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 at linux.alibaba.com> > --- > 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);You made common_len size_t so printing it with %ld is wrong. Causes warnings at least on on 32 bit.> + > + 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; > + > + 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.g01195cf9fdropped this patch for now until the warning can be fixed. rest of patchset still in.