Re: [PATCH rdma-next 19/23] RDMA/mlx5: Clamp log_max_cq_sz shift in CQ size checks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] RDMA/mlx5: 32-bit entries * cqe_size overflow in CQ create and resize
--

commit 23d4d478ec288a1c091e2edf580c64eccf4e7c49
Author: Yishai Hadas <yishaih@nvidia.com>

RDMA/mlx5: Clamp log_max_cq_sz shift in CQ size checks

This commit replaces the 32-bit shift of log_max_cq_sz with
MLX5_CAP_GEN_LOGSZ_U64(), rounds up the requested CQ entry count using 64-bit
arithmetic, and rejects rounded-up entry counts above INT_MAX in the CQ create
and resize paths.

> diff --git a/drivers/infiniband/hw/mlx5/cq.c b/drivers/infiniband/hw/mlx5/cq.c
> index f955eb762051d..25344dfba1c12 100644
> --- a/drivers/infiniband/hw/mlx5/cq.c
> +++ b/drivers/infiniband/hw/mlx5/cq.c
> @@ -1117,6 +1117,15 @@ static void notify_soft_wc_handler(struct work_struct *work)
>  	cq->ibcq.comp_handler(&cq->ibcq, cq->ibcq.cq_context);
>  }
>  
> +/*
> + * Next power of 2 above entries. It can be 2^32, which doesn't fit the
> + * unsigned long of roundup_pow_of_two() on 32-bit archs.
> + */
> +static u64 cq_roundup_entries(u64 entries)
> +{
> +	return 1ULL << fls64(entries);
> +}
> +
>  int mlx5_ib_create_user_cq(struct ib_cq *ibcq,
>  			   const struct ib_cq_init_attr *attr,
>  			   struct uverbs_attr_bundle *attrs)
> @@ -1124,7 +1133,7 @@ int mlx5_ib_create_user_cq(struct ib_cq *ibcq,
>  	struct ib_udata *udata = &attrs->driver_udata;
>  	struct ib_device *ibdev = ibcq->device;
>  	struct mlx5_ib_create_cq_resp uresp = {};
> -	int entries = attr->cqe;
> +	u64 entries = attr->cqe;
>  	int vector = attr->comp_vector;
>  	struct mlx5_ib_dev *dev = to_mdev(ibdev);
>  	struct mlx5_ib_cq *cq = to_mcq(ibcq);
> @@ -1136,15 +1145,17 @@ int mlx5_ib_create_user_cq(struct ib_cq *ibcq,
>  	int cqe_size;
>  	int eqn;
>  	int err;
> +	u64 max_cq_entries = MLX5_CAP_GEN_LOGSZ_U64(dev->mdev, log_max_cq_sz);
>  
> -	if (attr->cqe > (1 << MLX5_CAP_GEN(dev->mdev, log_max_cq_sz)))
> +	if (attr->cqe > max_cq_entries)
>  		return -EINVAL;
>  
>  	if (check_cq_create_flags(attr->flags))
>  		return -EOPNOTSUPP;
>  
> -	entries = roundup_pow_of_two(entries + 1);
> -	if (entries > (1 << MLX5_CAP_GEN(dev->mdev, log_max_cq_sz)))
> +	entries = cq_roundup_entries(entries);
> +	/* cq->ibcq.cqe and create_cq_user() take an int */
> +	if (entries > max_cq_entries || entries > INT_MAX)
>  		return -EINVAL;
>  
>  	cq->ibcq.cqe = entries - 1;
> @@ -1214,7 +1225,7 @@ int mlx5_ib_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
>  		      struct uverbs_attr_bundle *attrs)
>  {
>  	struct ib_device *ibdev = ibcq->device;
> -	int entries = attr->cqe;
> +	u64 entries = attr->cqe;
>  	int vector = attr->comp_vector;
>  	struct mlx5_ib_dev *dev = to_mdev(ibdev);
>  	struct mlx5_ib_cq *cq = to_mcq(ibcq);
> @@ -1226,12 +1237,14 @@ int mlx5_ib_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
>  	int cqe_size;
>  	int eqn;
>  	int err;
> +	u64 max_cq_entries = MLX5_CAP_GEN_LOGSZ_U64(dev->mdev, log_max_cq_sz);
>  
> -	if (attr->cqe > (1 << MLX5_CAP_GEN(dev->mdev, log_max_cq_sz)))
> +	if (attr->cqe > max_cq_entries)
>  		return -EINVAL;
>  
> -	entries = roundup_pow_of_two(entries + 1);
> -	if (entries > (1 << MLX5_CAP_GEN(dev->mdev, log_max_cq_sz)))
> +	entries = cq_roundup_entries(entries);
> +	/* cq->ibcq.cqe and create_cq_kernel() take an int */
> +	if (entries > max_cq_entries || entries > INT_MAX)
>  		return -EINVAL;
>  
>  	cq->ibcq.cqe = entries - 1;
> @@ -1532,18 +1545,22 @@ int mlx5_ib_resize_cq(struct ib_cq *ibcq, unsigned int entries,
>  	int inlen;
>  	int cqe_size;
>  	unsigned long flags;
> +	u64 entries64 = entries;
> +	u64 max_cq_entries = MLX5_CAP_GEN_LOGSZ_U64(dev->mdev, log_max_cq_sz);
>  
>  	if (!MLX5_CAP_GEN(dev->mdev, cq_resize)) {
>  		pr_info("Firmware does not support resize CQ\n");
>  		return -ENOSYS;
>  	}
>  
> -	if (entries > (1 << MLX5_CAP_GEN(dev->mdev, log_max_cq_sz)))
> +	if (entries64 > max_cq_entries)
>  		return -EINVAL;
>  
> -	entries = roundup_pow_of_two(entries + 1);
> -	if (entries > (1 << MLX5_CAP_GEN(dev->mdev, log_max_cq_sz)) + 1)
> +	entries64 = cq_roundup_entries(entries64);
> +	/* resize_user() and resize_kernel() take an int */
> +	if (entries64 > max_cq_entries + 1 || entries64 > INT_MAX)
>  		return -EINVAL;
> +	entries = entries64;

[Severity: High]
When an untrusted NIC reports log_max_cq_sz >= 24, bounding the rounded-up
entry count only by INT_MAX allows power-of-two values of entries from 2^24
up to 2^30 to reach the downstream buffer size calculations. Can those 32-bit
multiplications by cqe_size still overflow?

In mlx5_ib_create_user_cq() -> create_cq_user(), entries (int) is multiplied
by ucmd.cqe_size (__u32, validated as 64 or 128) in 32-bit unsigned
arithmetic before widening to size_t:

drivers/infiniband/hw/mlx5/cq.c:create_cq_user() {
	...
	cq->buf.umem = ib_umem_get_cq_buf_or_va(&dev->ib_dev, attrs,
						ucmd.buf_addr,
						entries * ucmd.cqe_size,
						IB_ACCESS_LOCAL_WRITE);
	...
}

For entries in 2^25..2^30 with ucmd.cqe_size == 128 (or 2^26..2^30 with
ucmd.cqe_size == 64), entries * ucmd.cqe_size wraps to 0, which lets a
1-page user buffer pass the min_size check in ib_umem_get_desc_check() while
programming log_cq_size = 25..30 in the CQ context.

Similarly, in mlx5_ib_create_cq() -> create_cq_kernel() ->
alloc_cq_frag_buf() and mlx5_ib_resize_cq() -> resize_kernel() ->
alloc_cq_frag_buf(), nent * cqe_size is evaluated as a signed 32-bit int and
passed as int size to mlx5_frag_buf_alloc_node():

drivers/infiniband/hw/mlx5/cq.c:alloc_cq_frag_buf() {
	...
	err = mlx5_frag_buf_alloc_node(dev->mdev,
				       nent * cqe_size,
				       frag_buf,
				       dev->mdev->priv.numa_node);
	...
}

For nent == 2^24 (with cqe_size == 128) or nent == 2^25 (with cqe_size == 64),
nent * cqe_size overflows to INT_MIN, and for nent >= 2^25 (cqe_size == 128)
or nent >= 2^26 (cqe_size == 64), it wraps to 0. When size is 0,
mlx5_frag_buf_alloc_node() sets frag_buf->frags = ZERO_SIZE_PTR and returns 0,
and init_cq_frag_buf() then dereferences ZERO_SIZE_PTR when iterating over
buf->nent:

drivers/infiniband/hw/mlx5/cq.c:init_cq_frag_buf() {
	for (i = 0; i < buf->nent; i++) {
		cqe = mlx5_frag_buf_get_wqe(&buf->fbc, i);
		cqe64 = buf->cqe_size == 64 ? cqe : cqe + 64;
		cqe64->op_own = MLX5_CQE_INVALID << 4;
	}
}

Would it make sense to cap entries so that entries * cqe_size cannot overflow
int (or use check_mul_overflow() in create_cq_user() and alloc_cq_frag_buf())?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103163949.1554-1-yishaih@nvidia.com?part=19