[PATCH v2 0/3] fixes for mlx5_vdpa multiqueue support

[PATCH v2 0/3] fixes for mlx5_vdpa multiqueue support

[Si-Wei Liu]

This patchset contains the fixes and code cleanups for a couple issues
uncovered during the review for the "Allow for configuring max number
of virtqueue pairs" series.

It is based on Eli's fixes:
2e4cda633a22 ("vdpa/mlx5: Fix tracking of current number of VQs")
in the vhost tree.

--
v1 -> v2:
  - removed unneeded Fixes tag to avoid confusion
  - failing the set_features op instead of clearing invalid feature
  - add motivation to the commit log
  - fix or add code comments in place per review feedback
  - re-align the subject in some patches to better reflect the actual
    code change
  - fixed corrupted email address format in SoB

Si-Wei Liu (3):
  vdpa: factor out vdpa_set_features_unlocked for vdpa internal use
  vdpa/mlx5: should verify CTRL_VQ feature exists for MQ
  vdpa/mlx5: add validation for VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command

 drivers/vdpa/mlx5/net/mlx5_vnet.c | 34 ++++++++++++++++++++++++++++++++--
 drivers/vdpa/vdpa.c               |  2 +-
 drivers/vhost/vdpa.c              |  2 +-
 drivers/virtio/virtio_vdpa.c      |  2 +-
 include/linux/vdpa.h              | 18 ++++++++++++------
 5 files changed, 47 insertions(+), 11 deletions(-)

-- 
1.8.3.1

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

[PATCH v2 1/3] vdpa: factor out vdpa_set_features_unlocked for vdpa internal use

[Si-Wei Liu]

No functional change introduced. vdpa bus driver such as virtio_vdpa
or vhost_vdpa is not supposed to take care of the locking for core
by its own. The locked API vdpa_set_features should suffice the
bus driver's need.

Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
Reviewed-by: Eli Cohen <elic@nvidia.com>
---
 drivers/vdpa/vdpa.c          |  2 +-
 drivers/vhost/vdpa.c         |  2 +-
 drivers/virtio/virtio_vdpa.c |  2 +-
 include/linux/vdpa.h         | 18 ++++++++++++------
 4 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 9846c9d..1ea5254 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -393,7 +393,7 @@ static void vdpa_get_config_unlocked(struct vdpa_device *vdev,
 	 * If it does happen we assume a legacy guest.
 	 */
 	if (!vdev->features_valid)
-		vdpa_set_features(vdev, 0, true);
+		vdpa_set_features_unlocked(vdev, 0);
 	ops->get_config(vdev, offset, buf, len);
 }
 
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 8515398..ec5249e 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -286,7 +286,7 @@ static long vhost_vdpa_set_features(struct vhost_vdpa *v, u64 __user *featurep)
 	if (copy_from_user(&features, featurep, sizeof(features)))
 		return -EFAULT;
 
-	if (vdpa_set_features(vdpa, features, false))
+	if (vdpa_set_features(vdpa, features))
 		return -EINVAL;
 
 	return 0;
diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
index 7767a7f..7650455 100644
--- a/drivers/virtio/virtio_vdpa.c
+++ b/drivers/virtio/virtio_vdpa.c
@@ -317,7 +317,7 @@ static int virtio_vdpa_finalize_features(struct virtio_device *vdev)
 	/* Give virtio_ring a chance to accept features. */
 	vring_transport_features(vdev);
 
-	return vdpa_set_features(vdpa, vdev->features, false);
+	return vdpa_set_features(vdpa, vdev->features);
 }
 
 static const char *virtio_vdpa_bus_name(struct virtio_device *vdev)
diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 2de442e..721089b 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -401,18 +401,24 @@ static inline int vdpa_reset(struct vdpa_device *vdev)
 	return ret;
 }
 
-static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features, bool locked)
+static inline int vdpa_set_features_unlocked(struct vdpa_device *vdev, u64 features)
 {
 	const struct vdpa_config_ops *ops = vdev->config;
 	int ret;
 
-	if (!locked)
-		mutex_lock(&vdev->cf_mutex);
-
 	vdev->features_valid = true;
 	ret = ops->set_driver_features(vdev, features);
-	if (!locked)
-		mutex_unlock(&vdev->cf_mutex);
+
+	return ret;
+}
+
+static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features)
+{
+	int ret;
+
+	mutex_lock(&vdev->cf_mutex);
+	ret = vdpa_set_features_unlocked(vdev, features);
+	mutex_unlock(&vdev->cf_mutex);
 
 	return ret;
 }
-- 
1.8.3.1

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

Re: [PATCH v2 1/3] vdpa: factor out vdpa_set_features_unlocked for vdpa internal use

[Jason Wang]

在 2022/1/15 上午8:27, Si-Wei Liu 写道:
> No functional change introduced. vdpa bus driver such as virtio_vdpa
> or vhost_vdpa is not supposed to take care of the locking for core
> by its own. The locked API vdpa_set_features should suffice the
> bus driver's need.
>
> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> Reviewed-by: Eli Cohen <elic@nvidia.com>


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


> ---
>   drivers/vdpa/vdpa.c          |  2 +-
>   drivers/vhost/vdpa.c         |  2 +-
>   drivers/virtio/virtio_vdpa.c |  2 +-
>   include/linux/vdpa.h         | 18 ++++++++++++------
>   4 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 9846c9d..1ea5254 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -393,7 +393,7 @@ static void vdpa_get_config_unlocked(struct vdpa_device *vdev,
>   	 * If it does happen we assume a legacy guest.
>   	 */
>   	if (!vdev->features_valid)
> -		vdpa_set_features(vdev, 0, true);
> +		vdpa_set_features_unlocked(vdev, 0);
>   	ops->get_config(vdev, offset, buf, len);
>   }
>   
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 8515398..ec5249e 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -286,7 +286,7 @@ static long vhost_vdpa_set_features(struct vhost_vdpa *v, u64 __user *featurep)
>   	if (copy_from_user(&features, featurep, sizeof(features)))
>   		return -EFAULT;
>   
> -	if (vdpa_set_features(vdpa, features, false))
> +	if (vdpa_set_features(vdpa, features))
>   		return -EINVAL;
>   
>   	return 0;
> diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> index 7767a7f..7650455 100644
> --- a/drivers/virtio/virtio_vdpa.c
> +++ b/drivers/virtio/virtio_vdpa.c
> @@ -317,7 +317,7 @@ static int virtio_vdpa_finalize_features(struct virtio_device *vdev)
>   	/* Give virtio_ring a chance to accept features. */
>   	vring_transport_features(vdev);
>   
> -	return vdpa_set_features(vdpa, vdev->features, false);
> +	return vdpa_set_features(vdpa, vdev->features);
>   }
>   
>   static const char *virtio_vdpa_bus_name(struct virtio_device *vdev)
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index 2de442e..721089b 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -401,18 +401,24 @@ static inline int vdpa_reset(struct vdpa_device *vdev)
>   	return ret;
>   }
>   
> -static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features, bool locked)
> +static inline int vdpa_set_features_unlocked(struct vdpa_device *vdev, u64 features)
>   {
>   	const struct vdpa_config_ops *ops = vdev->config;
>   	int ret;
>   
> -	if (!locked)
> -		mutex_lock(&vdev->cf_mutex);
> -
>   	vdev->features_valid = true;
>   	ret = ops->set_driver_features(vdev, features);
> -	if (!locked)
> -		mutex_unlock(&vdev->cf_mutex);
> +
> +	return ret;
> +}
> +
> +static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features)
> +{
> +	int ret;
> +
> +	mutex_lock(&vdev->cf_mutex);
> +	ret = vdpa_set_features_unlocked(vdev, features);
> +	mutex_unlock(&vdev->cf_mutex);
>   
>   	return ret;
>   }

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

[PATCH v2 2/3] vdpa/mlx5: should verify CTRL_VQ feature exists for MQ

[Si-Wei Liu]

Per VIRTIO v1.1 specification, section 5.1.3.1 Feature bit requirements:
"VIRTIO_NET_F_MQ Requires VIRTIO_NET_F_CTRL_VQ".

There's assumption in the mlx5_vdpa multiqueue code that MQ must come
together with CTRL_VQ. However, there's nowhere in the upper layer to
guarantee this assumption would hold. Were there an untrusted driver
sending down MQ without CTRL_VQ, it would compromise various spots for
e.g. is_index_valid() and is_ctrl_vq_idx(). Although this doesn't end
up with immediate panic or security loophole as of today's code, the
chance for this to be taken advantage of due to future code change is
not zero.

Harden the crispy assumption by failing the set_driver_features() call
when seeing (MQ && !CTRL_VQ). For that end, verify_min_features() is
renamed to verify_driver_features() to reflect the fact that it now does
more than just validate the minimum features. verify_driver_features()
is now used to accommodate various checks against the driver features
for set_driver_features().

Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index b53603d..6fa968f 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1897,11 +1897,25 @@ static u64 mlx5_vdpa_get_device_features(struct vdpa_device *vdev)
 	return ndev->mvdev.mlx_features;
 }
 
-static int verify_min_features(struct mlx5_vdpa_dev *mvdev, u64 features)
+static int verify_driver_features(struct mlx5_vdpa_dev *mvdev, u64 features)
 {
+	/* Minimum features to expect */
 	if (!(features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
 		return -EOPNOTSUPP;
 
+	/* Double check features combination sent down by the driver.
+	 * Fail invalid features due to absence of the depended feature.
+	 *
+	 * Per VIRTIO v1.1 specification, section 5.1.3.1 Feature bit
+	 * requirements: "VIRTIO_NET_F_MQ Requires VIRTIO_NET_F_CTRL_VQ".
+	 * By failing the invalid features sent down by untrusted drivers,
+	 * we're assured the assumption made upon is_index_valid() and
+	 * is_ctrl_vq_idx() will not be compromised.
+	 */
+	if ((features & (BIT_ULL(VIRTIO_NET_F_MQ) | BIT_ULL(VIRTIO_NET_F_CTRL_VQ))) ==
+            BIT_ULL(VIRTIO_NET_F_MQ))
+		return -EINVAL;
+
 	return 0;
 }
 
@@ -1977,7 +1991,7 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
 
 	print_features(mvdev, features, true);
 
-	err = verify_min_features(mvdev, features);
+	err = verify_driver_features(mvdev, features);
 	if (err)
 		return err;
 
-- 
1.8.3.1

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

Re: [PATCH v2 2/3] vdpa/mlx5: should verify CTRL_VQ feature exists for MQ

[Jason Wang]

在 2022/1/15 上午8:28, Si-Wei Liu 写道:
> Per VIRTIO v1.1 specification, section 5.1.3.1 Feature bit requirements:
> "VIRTIO_NET_F_MQ Requires VIRTIO_NET_F_CTRL_VQ".
>
> There's assumption in the mlx5_vdpa multiqueue code that MQ must come
> together with CTRL_VQ. However, there's nowhere in the upper layer to
> guarantee this assumption would hold. Were there an untrusted driver
> sending down MQ without CTRL_VQ, it would compromise various spots for
> e.g. is_index_valid() and is_ctrl_vq_idx(). Although this doesn't end
> up with immediate panic or security loophole as of today's code, the
> chance for this to be taken advantage of due to future code change is
> not zero.
>
> Harden the crispy assumption by failing the set_driver_features() call
> when seeing (MQ && !CTRL_VQ). For that end, verify_min_features() is
> renamed to verify_driver_features() to reflect the fact that it now does
> more than just validate the minimum features. verify_driver_features()
> is now used to accommodate various checks against the driver features
> for set_driver_features().
>
> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>


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


> ---
>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index b53603d..6fa968f 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1897,11 +1897,25 @@ static u64 mlx5_vdpa_get_device_features(struct vdpa_device *vdev)
>   	return ndev->mvdev.mlx_features;
>   }
>   
> -static int verify_min_features(struct mlx5_vdpa_dev *mvdev, u64 features)
> +static int verify_driver_features(struct mlx5_vdpa_dev *mvdev, u64 features)
>   {
> +	/* Minimum features to expect */
>   	if (!(features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
>   		return -EOPNOTSUPP;
>   
> +	/* Double check features combination sent down by the driver.
> +	 * Fail invalid features due to absence of the depended feature.
> +	 *
> +	 * Per VIRTIO v1.1 specification, section 5.1.3.1 Feature bit
> +	 * requirements: "VIRTIO_NET_F_MQ Requires VIRTIO_NET_F_CTRL_VQ".
> +	 * By failing the invalid features sent down by untrusted drivers,
> +	 * we're assured the assumption made upon is_index_valid() and
> +	 * is_ctrl_vq_idx() will not be compromised.
> +	 */
> +	if ((features & (BIT_ULL(VIRTIO_NET_F_MQ) | BIT_ULL(VIRTIO_NET_F_CTRL_VQ))) ==
> +            BIT_ULL(VIRTIO_NET_F_MQ))
> +		return -EINVAL;
> +
>   	return 0;
>   }
>   
> @@ -1977,7 +1991,7 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
>   
>   	print_features(mvdev, features, true);
>   
> -	err = verify_min_features(mvdev, features);
> +	err = verify_driver_features(mvdev, features);
>   	if (err)
>   		return err;
>   

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

[PATCH v2 3/3] vdpa/mlx5: add validation for VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command

[Si-Wei Liu]

When control vq receives a VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command
request from the driver, presently there is no validation against the
number of queue pairs to configure, or even if multiqueue had been
negotiated or not is unverified. This may lead to kernel panic due to
uninitialized resource for the queues were there any bogus request
sent down by untrusted driver. Tie up the loose ends there.

Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 6fa968f..86f84dc 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1563,11 +1563,27 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
 
 	switch (cmd) {
 	case VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET:
+		/* This mq feature check aligns with pre-existing userspace
+		 * implementation.
+		 *
+		 * Without it, an untrusted driver could fake a multiqueue config
+		 * request down to a non-mq device that may cause kernel to
+		 * panic due to uninitialized resources for extra vqs. Even with
+		 * a well behaving guest driver, it is not expected to allow
+		 * changing the number of vqs on a non-mq device.
+		 */
+		if (!MLX5_FEATURE(mvdev, VIRTIO_NET_F_MQ))
+			break;
+
 		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, (void *)&mq, sizeof(mq));
 		if (read != sizeof(mq))
 			break;
 
 		newqps = mlx5vdpa16_to_cpu(mvdev, mq.virtqueue_pairs);
+		if (newqps < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
+		    newqps > mlx5_vdpa_max_qps(mvdev->max_vqs))
+			break;
+
 		if (ndev->cur_num_vqs == 2 * newqps) {
 			status = VIRTIO_NET_OK;
 			break;
-- 
1.8.3.1

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

Re: [PATCH v2 3/3] vdpa/mlx5: add validation for VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command

[Jason Wang]

在 2022/1/15 上午8:28, Si-Wei Liu 写道:
> When control vq receives a VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command
> request from the driver, presently there is no validation against the
> number of queue pairs to configure, or even if multiqueue had been
> negotiated or not is unverified. This may lead to kernel panic due to
> uninitialized resource for the queues were there any bogus request
> sent down by untrusted driver. Tie up the loose ends there.
>
> Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>


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


> ---
>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 6fa968f..86f84dc 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1563,11 +1563,27 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>   
>   	switch (cmd) {
>   	case VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET:
> +		/* This mq feature check aligns with pre-existing userspace
> +		 * implementation.
> +		 *
> +		 * Without it, an untrusted driver could fake a multiqueue config
> +		 * request down to a non-mq device that may cause kernel to
> +		 * panic due to uninitialized resources for extra vqs. Even with
> +		 * a well behaving guest driver, it is not expected to allow
> +		 * changing the number of vqs on a non-mq device.
> +		 */
> +		if (!MLX5_FEATURE(mvdev, VIRTIO_NET_F_MQ))
> +			break;
> +
>   		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, (void *)&mq, sizeof(mq));
>   		if (read != sizeof(mq))
>   			break;
>   
>   		newqps = mlx5vdpa16_to_cpu(mvdev, mq.virtqueue_pairs);
> +		if (newqps < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
> +		    newqps > mlx5_vdpa_max_qps(mvdev->max_vqs))
> +			break;
> +
>   		if (ndev->cur_num_vqs == 2 * newqps) {
>   			status = VIRTIO_NET_OK;
>   			break;

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