[PATCH vhost v3 0/4] strictly check the acccess to the common cfg

[PATCH vhost v3 0/4] strictly check the acccess to the common cfg

[Xuan Zhuo]

1. fix the length of the pci_iomap_range() to the modern common cfg
2. add common cfg size check for the VIRTIO_F_NOTIF_CONFIG_DATA, VIRTIO_F_RING_RESET
3. add build size check to the new common cfg items

Xuan Zhuo (4):
  virtio: add definition of VIRTIO_F_NOTIF_CONFIG_DATA feature bit
  virtio_pci: fix the common cfg map size
  virtio_pci: add check for common cfg size
  virtio_pci: add build offset check for the new common cfg items

 drivers/virtio/virtio_pci_modern.c     | 36 ++++++++++++++++++++++++++
 drivers/virtio/virtio_pci_modern_dev.c |  8 ++++--
 include/linux/virtio_pci_modern.h      |  1 +
 include/uapi/linux/virtio_config.h     |  5 ++++
 4 files changed, 48 insertions(+), 2 deletions(-)

--
2.32.0.3.g01195cf9f

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH vhost v3 1/4] virtio: add definition of VIRTIO_F_NOTIF_CONFIG_DATA feature bit

[Xuan Zhuo]

This patch adds the definition of VIRTIO_F_NOTIF_CONFIG_DATA feature bit
in the relevant header file.

This feature indicates that the driver uses the data provided by the
device as a virtqueue identifier in available buffer notifications.

It comes from here:
    https://github.com/oasis-tcs/virtio-spec/issues/89

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 include/uapi/linux/virtio_config.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
index 2c712c654165..8881aea60f6f 100644
--- a/include/uapi/linux/virtio_config.h
+++ b/include/uapi/linux/virtio_config.h
@@ -105,6 +105,11 @@
  */
 #define VIRTIO_F_NOTIFICATION_DATA	38
 
+/* This feature indicates that the driver uses the data provided by the device
+ * as a virtqueue identifier in available buffer notifications.
+ */
+#define VIRTIO_F_NOTIF_CONFIG_DATA	39
+
 /*
  * This feature indicates that the driver can reset a queue individually.
  */
-- 
2.32.0.3.g01195cf9f

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH vhost v3 1/4] virtio: add definition of VIRTIO_F_NOTIF_CONFIG_DATA feature bit

[Jason Wang]

On Tue, Oct 10, 2023 at 11:11 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> This patch adds the definition of VIRTIO_F_NOTIF_CONFIG_DATA feature bit
> in the relevant header file.
>
> This feature indicates that the driver uses the data provided by the
> device as a virtqueue identifier in available buffer notifications.
>
> It comes from here:
>     https://github.com/oasis-tcs/virtio-spec/issues/89
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

> ---
>  include/uapi/linux/virtio_config.h | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> index 2c712c654165..8881aea60f6f 100644
> --- a/include/uapi/linux/virtio_config.h
> +++ b/include/uapi/linux/virtio_config.h
> @@ -105,6 +105,11 @@
>   */
>  #define VIRTIO_F_NOTIFICATION_DATA     38
>
> +/* This feature indicates that the driver uses the data provided by the device
> + * as a virtqueue identifier in available buffer notifications.
> + */
> +#define VIRTIO_F_NOTIF_CONFIG_DATA     39
> +
>  /*
>   * This feature indicates that the driver can reset a queue individually.
>   */
> --
> 2.32.0.3.g01195cf9f
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH vhost v3 2/4] virtio_pci: fix the common cfg map size

[Xuan Zhuo]

The function vp_modern_map_capability() takes the size parameter,
which corresponds to the size of virtio_pci_common_cfg. As a result,
this indicates the size of memory area to map.

Now the size is the size of virtio_pci_common_cfg, but some feature(such
as the _F_RING_RESET) needs the virtio_pci_modern_common_cfg, so this
commit changes the size to the size of virtio_pci_modern_common_cfg.

Fixes: 0b50cece0b78 ("virtio_pci: introduce helper to get/set queue reset")
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_pci_modern_dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
index aad7d9296e77..9cb601e16688 100644
--- a/drivers/virtio/virtio_pci_modern_dev.c
+++ b/drivers/virtio/virtio_pci_modern_dev.c
@@ -291,7 +291,7 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
 	err = -EINVAL;
 	mdev->common = vp_modern_map_capability(mdev, common,
 				      sizeof(struct virtio_pci_common_cfg), 4,
-				      0, sizeof(struct virtio_pci_common_cfg),
+				      0, sizeof(struct virtio_pci_modern_common_cfg),
 				      NULL, NULL);
 	if (!mdev->common)
 		goto err_map_common;
-- 
2.32.0.3.g01195cf9f

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH vhost v3 2/4] virtio_pci: fix the common cfg map size

[Jason Wang]

On Tue, Oct 10, 2023 at 11:11 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> The function vp_modern_map_capability() takes the size parameter,
> which corresponds to the size of virtio_pci_common_cfg. As a result,
> this indicates the size of memory area to map.
>
> Now the size is the size of virtio_pci_common_cfg, but some feature(such
> as the _F_RING_RESET) needs the virtio_pci_modern_common_cfg, so this
> commit changes the size to the size of virtio_pci_modern_common_cfg.
>
> Fixes: 0b50cece0b78 ("virtio_pci: introduce helper to get/set queue reset")
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

> ---
>  drivers/virtio/virtio_pci_modern_dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
> index aad7d9296e77..9cb601e16688 100644
> --- a/drivers/virtio/virtio_pci_modern_dev.c
> +++ b/drivers/virtio/virtio_pci_modern_dev.c
> @@ -291,7 +291,7 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
>         err = -EINVAL;
>         mdev->common = vp_modern_map_capability(mdev, common,
>                                       sizeof(struct virtio_pci_common_cfg), 4,
> -                                     0, sizeof(struct virtio_pci_common_cfg),
> +                                     0, sizeof(struct virtio_pci_modern_common_cfg),
>                                       NULL, NULL);
>         if (!mdev->common)
>                 goto err_map_common;
> --
> 2.32.0.3.g01195cf9f
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH vhost v3 3/4] virtio_pci: add check for common cfg size

[Xuan Zhuo]

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@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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH vhost v3 3/4] virtio_pci: add check for common cfg size

[Jason Wang]

On Tue, Oct 10, 2023 at 11:11 AM Xuan Zhuo <xuanzhuo@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@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
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH vhost v3 3/4] virtio_pci: add check for common cfg size

[Xuan Zhuo]

On Tue, 10 Oct 2023 14:41:39 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Tue, Oct 10, 2023 at 11:11 AM Xuan Zhuo <xuanzhuo@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@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?


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@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH vhost v3 3/4] virtio_pci: add check for common cfg size

[Jason Wang]

On Tue, Oct 10, 2023 at 3:58 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Tue, 10 Oct 2023 14:41:39 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > On Tue, Oct 10, 2023 at 11:11 AM Xuan Zhuo <xuanzhuo@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@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?
>
>
> 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).

Great.

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

>
> 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@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH vhost v3 3/4] virtio_pci: add check for common cfg size

[Michael S. Tsirkin]

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@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.g01195cf9f


dropped this patch for now until the warning can be fixed.
rest of patchset still in.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH vhost v3 4/4] virtio_pci: add build offset check for the new common cfg items

[Xuan Zhuo]

Add checks to the check_offsets(void) for queue_notify_data and
queue_reset.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/virtio/virtio_pci_modern_dev.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
index 33f319da1558..e2a1fe7bb66c 100644
--- a/drivers/virtio/virtio_pci_modern_dev.c
+++ b/drivers/virtio/virtio_pci_modern_dev.c
@@ -203,6 +203,10 @@ static inline void check_offsets(void)
 		     offsetof(struct virtio_pci_common_cfg, queue_used_lo));
 	BUILD_BUG_ON(VIRTIO_PCI_COMMON_Q_USEDHI !=
 		     offsetof(struct virtio_pci_common_cfg, queue_used_hi));
+	BUILD_BUG_ON(VIRTIO_PCI_COMMON_Q_NDATA !=
+		     offsetof(struct virtio_pci_modern_common_cfg, queue_notify_data));
+	BUILD_BUG_ON(VIRTIO_PCI_COMMON_Q_RESET !=
+		     offsetof(struct virtio_pci_modern_common_cfg, queue_reset));
 }
 
 /*
-- 
2.32.0.3.g01195cf9f

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization