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

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

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