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

[Yishai Hadas <yishaih@nvidia.com>]

On 04/11/2026 18:43, sashiko-bot@kernel.org wrote:
> 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())?
> 

Yes, v1 will use check_mul_overflow() in both, with the real CQE size.

Yishai