[PATCH vhost v1 0/2] strictly check the acccess to the common cfg

[PATCH vhost v1 0/2] strictly check the acccess to the common cfg

[Xuan Zhuo]

1. fix the length of the pci_iomap_range() to the common cfg
2. add size check for the vq-reset
3. add build size check to the new common cfg items

Xuan Zhuo (2):
  virtio_pci: fix the common map size and add check for vq-reset
  virtio_pci: add build offset check for the new common cfg items

 drivers/virtio/virtio_pci_modern_dev.c | 31 ++++++++++++++++++++++++--
 1 file changed, 29 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 v1 1/2] virtio_pci: fix the common map size and add check for vq-reset

[Xuan Zhuo]

Now, 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.

However, if the _F_RING_RESET is negotiated, the extra items will be
used. Therefore, we need to use the size of virtio_pci_modre_common_cfg
to map more space.

Meanwhile, this patch checks the common cfg size when _F_RING_ERSET is
negotiated.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_pci_modern_dev.c | 27 ++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
index aad7d9296e77..45d41e6c7799 100644
--- a/drivers/virtio/virtio_pci_modern_dev.c
+++ b/drivers/virtio/virtio_pci_modern_dev.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 
 #include <linux/virtio_pci_modern.h>
+#include <linux/virtio_config.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/delay.h>
@@ -142,6 +143,22 @@ static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
 	return 0;
 }
 
+static inline int check_common_size(struct virtio_pci_modern_device *mdev,
+				     size_t common_len)
+{
+	u64 features;
+
+	features = vp_modern_get_features(mdev);
+
+	if (features & BIT_ULL(VIRTIO_F_RING_RESET)) {
+		if (unlikely(common_len < offsetofend(struct virtio_pci_modern_common_cfg,
+						      queue_reset)))
+			return -ENOENT;
+	}
+
+	return 0;
+}
+
 /* This is part of the ABI.  Don't screw with it. */
 static inline void check_offsets(void)
 {
@@ -218,6 +235,7 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
 	int err, common, isr, notify, device;
 	u32 notify_length;
 	u32 notify_offset;
+	size_t common_len;
 	int devid;
 
 	check_offsets();
@@ -291,10 +309,14 @@ 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),
-				      NULL, NULL);
+				      0, sizeof(struct virtio_pci_modern_common_cfg),
+				      &common_len, NULL);
 	if (!mdev->common)
 		goto err_map_common;
+
+	if (check_common_size(mdev, common_len))
+		goto err_common_size;
+
 	mdev->isr = vp_modern_map_capability(mdev, isr, sizeof(u8), 1,
 					     0, 1,
 					     NULL, NULL);
@@ -353,6 +375,7 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
 err_map_notify:
 	pci_iounmap(pci_dev, mdev->isr);
 err_map_isr:
+err_common_size:
 	pci_iounmap(pci_dev, mdev->common);
 err_map_common:
 	pci_release_selected_regions(pci_dev, mdev->modern_bars);
-- 
2.32.0.3.g01195cf9f

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

[PATCH vhost v1 2/2] 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 45d41e6c7799..3a49bc963a71 100644
--- a/drivers/virtio/virtio_pci_modern_dev.c
+++ b/drivers/virtio/virtio_pci_modern_dev.c
@@ -220,6 +220,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

Re: [PATCH vhost v1 1/2] virtio_pci: fix the common map size and add check for vq-reset

[Michael S. Tsirkin]

On Sun, Oct 08, 2023 at 03:45:34PM +0800, Xuan Zhuo wrote:
> Now, 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.
> 
> However, if the _F_RING_RESET is negotiated, the extra items will be
> used. Therefore, we need to use the size of virtio_pci_modre_common_cfg

typo

> to map more space.
> 
> Meanwhile, this patch checks the common cfg size when _F_RING_ERSET is
> negotiated.
> 

Fixes: tag please?

> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>  drivers/virtio/virtio_pci_modern_dev.c | 27 ++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
> index aad7d9296e77..45d41e6c7799 100644
> --- a/drivers/virtio/virtio_pci_modern_dev.c
> +++ b/drivers/virtio/virtio_pci_modern_dev.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  
>  #include <linux/virtio_pci_modern.h>
> +#include <linux/virtio_config.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
>  #include <linux/delay.h>
> @@ -142,6 +143,22 @@ static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
>  	return 0;
>  }
>  
> +static inline int check_common_size(struct virtio_pci_modern_device *mdev,
> +				     size_t common_len)
> +{
> +	u64 features;
> +
> +	features = vp_modern_get_features(mdev);

Better to combine this assignment into definition.
Or even better just drop the variable.

But more importantly this is called before DRIVER is set, right?
Not good then, this is a spec violation to read feature bits
before setting DRIVER:

The driver MUST follow this sequence to initialize a device:

\begin{enumerate}
\item Reset the device.

\item Set the ACKNOWLEDGE status bit: the guest OS has noticed the device.

\item Set the DRIVER status bit: the guest OS knows how to drive the device.

\item\label{itm:General Initialization And Device Operation /
Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
   understood by the OS and driver to the device.  During this step the
   driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it.


I guess we can add common_len alongside notify_len and device_len?

I think would also prefer to just clear VIRTIO_F_RING_RESET and not
fail probe.




> +
> +	if (features & BIT_ULL(VIRTIO_F_RING_RESET)) {
> +		if (unlikely(common_len < offsetofend(struct virtio_pci_modern_common_cfg,
> +						      queue_reset)))
> +			return -ENOENT;

Why ENOENT?


Also as long as you are doing this, please give the same
treatment to VIRTIO_F_NOTIFICATION_DATA.

> +	}
> +
> +	return 0;
> +}
> +
>  /* This is part of the ABI.  Don't screw with it. */
>  static inline void check_offsets(void)
>  {
> @@ -218,6 +235,7 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
>  	int err, common, isr, notify, device;
>  	u32 notify_length;
>  	u32 notify_offset;
> +	size_t common_len;
>  	int devid;
>  
>  	check_offsets();
> @@ -291,10 +309,14 @@ 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),
> -				      NULL, NULL);
> +				      0, sizeof(struct virtio_pci_modern_common_cfg),
> +				      &common_len, NULL);
>  	if (!mdev->common)
>  		goto err_map_common;
> +
> +	if (check_common_size(mdev, common_len))
> +		goto err_common_size;
> +
>  	mdev->isr = vp_modern_map_capability(mdev, isr, sizeof(u8), 1,
>  					     0, 1,
>  					     NULL, NULL);
> @@ -353,6 +375,7 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
>  err_map_notify:
>  	pci_iounmap(pci_dev, mdev->isr);
>  err_map_isr:
> +err_common_size:
>  	pci_iounmap(pci_dev, mdev->common);
>  err_map_common:
>  	pci_release_selected_regions(pci_dev, mdev->modern_bars);
> -- 
> 2.32.0.3.g01195cf9f

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