* [PATCH vhost v3 0/4] strictly check the acccess to the common cfg
@ 2023-10-10 3:11 Xuan Zhuo
2023-10-10 3:11 ` [PATCH vhost v3 1/4] virtio: add definition of VIRTIO_F_NOTIF_CONFIG_DATA feature bit Xuan Zhuo
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Xuan Zhuo @ 2023-10-10 3:11 UTC (permalink / raw)
To: virtualization; +Cc: Xuan Zhuo, Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH vhost v3 1/4] virtio: add definition of VIRTIO_F_NOTIF_CONFIG_DATA feature bit
2023-10-10 3:11 [PATCH vhost v3 0/4] strictly check the acccess to the common cfg Xuan Zhuo
@ 2023-10-10 3:11 ` Xuan Zhuo
2023-10-10 6:43 ` Jason Wang
2023-10-10 3:11 ` [PATCH vhost v3 2/4] virtio_pci: fix the common cfg map size Xuan Zhuo
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Xuan Zhuo @ 2023-10-10 3:11 UTC (permalink / raw)
To: virtualization; +Cc: Xuan Zhuo, Michael S. Tsirkin
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH vhost v3 2/4] virtio_pci: fix the common cfg map size
2023-10-10 3:11 [PATCH vhost v3 0/4] strictly check the acccess to the common cfg Xuan Zhuo
2023-10-10 3:11 ` [PATCH vhost v3 1/4] virtio: add definition of VIRTIO_F_NOTIF_CONFIG_DATA feature bit Xuan Zhuo
@ 2023-10-10 3:11 ` Xuan Zhuo
2023-10-10 6:42 ` Jason Wang
2023-10-10 3:11 ` [PATCH vhost v3 3/4] virtio_pci: add check for common cfg size Xuan Zhuo
2023-10-10 3:11 ` [PATCH vhost v3 4/4] virtio_pci: add build offset check for the new common cfg items Xuan Zhuo
3 siblings, 1 reply; 11+ messages in thread
From: Xuan Zhuo @ 2023-10-10 3:11 UTC (permalink / raw)
To: virtualization; +Cc: Xuan Zhuo, Michael S. Tsirkin
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH vhost v3 3/4] virtio_pci: add check for common cfg size
2023-10-10 3:11 [PATCH vhost v3 0/4] strictly check the acccess to the common cfg Xuan Zhuo
2023-10-10 3:11 ` [PATCH vhost v3 1/4] virtio: add definition of VIRTIO_F_NOTIF_CONFIG_DATA feature bit Xuan Zhuo
2023-10-10 3:11 ` [PATCH vhost v3 2/4] virtio_pci: fix the common cfg map size Xuan Zhuo
@ 2023-10-10 3:11 ` Xuan Zhuo
2023-10-10 6:41 ` Jason Wang
2023-10-18 20:07 ` Michael S. Tsirkin
2023-10-10 3:11 ` [PATCH vhost v3 4/4] virtio_pci: add build offset check for the new common cfg items Xuan Zhuo
3 siblings, 2 replies; 11+ messages in thread
From: Xuan Zhuo @ 2023-10-10 3:11 UTC (permalink / raw)
To: virtualization; +Cc: Xuan Zhuo, Michael S. Tsirkin
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH vhost v3 4/4] virtio_pci: add build offset check for the new common cfg items
2023-10-10 3:11 [PATCH vhost v3 0/4] strictly check the acccess to the common cfg Xuan Zhuo
` (2 preceding siblings ...)
2023-10-10 3:11 ` [PATCH vhost v3 3/4] virtio_pci: add check for common cfg size Xuan Zhuo
@ 2023-10-10 3:11 ` Xuan Zhuo
3 siblings, 0 replies; 11+ messages in thread
From: Xuan Zhuo @ 2023-10-10 3:11 UTC (permalink / raw)
To: virtualization; +Cc: Xuan Zhuo, Michael S. Tsirkin
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH vhost v3 3/4] virtio_pci: add check for common cfg size
2023-10-10 3:11 ` [PATCH vhost v3 3/4] virtio_pci: add check for common cfg size Xuan Zhuo
@ 2023-10-10 6:41 ` Jason Wang
2023-10-10 7:55 ` Xuan Zhuo
2023-10-18 20:07 ` Michael S. Tsirkin
1 sibling, 1 reply; 11+ messages in thread
From: Jason Wang @ 2023-10-10 6:41 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: Michael S. Tsirkin, virtualization
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH vhost v3 2/4] virtio_pci: fix the common cfg map size
2023-10-10 3:11 ` [PATCH vhost v3 2/4] virtio_pci: fix the common cfg map size Xuan Zhuo
@ 2023-10-10 6:42 ` Jason Wang
0 siblings, 0 replies; 11+ messages in thread
From: Jason Wang @ 2023-10-10 6:42 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: Michael S. Tsirkin, virtualization
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH vhost v3 1/4] virtio: add definition of VIRTIO_F_NOTIF_CONFIG_DATA feature bit
2023-10-10 3:11 ` [PATCH vhost v3 1/4] virtio: add definition of VIRTIO_F_NOTIF_CONFIG_DATA feature bit Xuan Zhuo
@ 2023-10-10 6:43 ` Jason Wang
0 siblings, 0 replies; 11+ messages in thread
From: Jason Wang @ 2023-10-10 6:43 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: Michael S. Tsirkin, virtualization
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH vhost v3 3/4] virtio_pci: add check for common cfg size
2023-10-10 6:41 ` Jason Wang
@ 2023-10-10 7:55 ` Xuan Zhuo
2023-10-11 3:35 ` Jason Wang
0 siblings, 1 reply; 11+ messages in thread
From: Xuan Zhuo @ 2023-10-10 7:55 UTC (permalink / raw)
To: Jason Wang; +Cc: virtualization, Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH vhost v3 3/4] virtio_pci: add check for common cfg size
2023-10-10 7:55 ` Xuan Zhuo
@ 2023-10-11 3:35 ` Jason Wang
0 siblings, 0 replies; 11+ messages in thread
From: Jason Wang @ 2023-10-11 3:35 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: virtualization, Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH vhost v3 3/4] virtio_pci: add check for common cfg size
2023-10-10 3:11 ` [PATCH vhost v3 3/4] virtio_pci: add check for common cfg size Xuan Zhuo
2023-10-10 6:41 ` Jason Wang
@ 2023-10-18 20:07 ` Michael S. Tsirkin
1 sibling, 0 replies; 11+ messages in thread
From: Michael S. Tsirkin @ 2023-10-18 20:07 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: virtualization
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-10-18 20:07 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-10 3:11 [PATCH vhost v3 0/4] strictly check the acccess to the common cfg Xuan Zhuo
2023-10-10 3:11 ` [PATCH vhost v3 1/4] virtio: add definition of VIRTIO_F_NOTIF_CONFIG_DATA feature bit Xuan Zhuo
2023-10-10 6:43 ` Jason Wang
2023-10-10 3:11 ` [PATCH vhost v3 2/4] virtio_pci: fix the common cfg map size Xuan Zhuo
2023-10-10 6:42 ` Jason Wang
2023-10-10 3:11 ` [PATCH vhost v3 3/4] virtio_pci: add check for common cfg size Xuan Zhuo
2023-10-10 6:41 ` Jason Wang
2023-10-10 7:55 ` Xuan Zhuo
2023-10-11 3:35 ` Jason Wang
2023-10-18 20:07 ` Michael S. Tsirkin
2023-10-10 3:11 ` [PATCH vhost v3 4/4] virtio_pci: add build offset check for the new common cfg items Xuan Zhuo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.