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

["Michael S. Tsirkin" <mst@redhat.com>]

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