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

[Yishai Hadas <yishaih@nvidia.com>]

On 07/11/2026 22:25, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] The new `entries > INT_MAX` check only keeps the CQ entry count
>    inside an int.
> 
> --
> 
> RDMA/mlx5: Clamp log_max_cq_sz shift in CQ size checks
> 
> This commit computes the CQ size limit with MLX5_CAP_GEN_LOGSZ_U64(), so
> an untrusted log_max_cq_sz no longer causes an undefined shift. The
> requested entry count is now rounded up in 64 bits. Sizes above INT_MAX
> are rejected in the 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
> 
> [ ... ]
> 
>> @@ -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;
> 
> [Severity: High]
> Is INT_MAX the right bound here? It keeps the entry count inside an int,
> but the CQ buffer's byte size is still computed from it in 32-bit
> arithmetic.
> 
> With a device-reported log_max_cq_sz of 26 or more, entries can now be up
> to 2^30. They are then passed to create_cq_kernel() and on to
> alloc_cq_frag_buf():
> 
> 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);
>      ...
> }
> 
> Both values are powers of two. So nent * cqe_size wraps to exactly 0 for
> nent >= 2^26 with 64-byte CQEs. With 128-byte CQEs
> (cache_line_size() == 128) it wraps at nent >= 2^25.
> 
> With size 0, mlx5_frag_buf_alloc_node() sets npages to 0, and
> kcalloc_node(0, ...) returns ZERO_SIZE_PTR. The function then returns
> success, and buf->nent is still set to the full count.
> 
> init_cq_frag_buf() then does:
> 
> 	for (i = 0; i < buf->nent; i++) {
> 		cqe = mlx5_frag_buf_get_wqe(&buf->fbc, i);
> 		...
> 	}
> 
> Won't mlx5_frag_buf_get_wqe() dereference fbc->frags[0].buf through
> ZERO_SIZE_PTR on the first iteration and oops?
> 
> mlx5_ib_resize_cq() with udata == NULL has the same INT_MAX-only check.
> It reaches the same code through resize_kernel()->alloc_cq_frag_buf()->
> init_cq_frag_buf().
> 
> The user path in mlx5_ib_create_user_cq() has a similar problem.
> create_cq_user() passes this as the minimum umem size:
> 
> 	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);
> 
> That is a 32-bit unsigned multiply, and it wraps to 0 the same way. On the
> UMEM-attribute path, the only length check in ib_umem_get_desc_check() is
> umem->length < min_size, so any umem passes. The device is still
> programmed with log_cq_size = ilog2(entries). resize_user() already uses
> (size_t)ucmd.cqe_size * entries for this.
> 
> The commit message says a large limit made the CQ size get "truncated
> when passed on as an int". Since create_cq_kernel() still multiplies that
> int by cqe_size, can this truncation still happen?
> 
> Should the bound account for the CQE size, for example by rejecting
> entries > INT_MAX / 128? Another option would be to compute the byte size
> in size_t with check_mul_overflow().
> 
>>
>>   	cq->ibcq.cqe = entries - 1;
> 
> [ ... ]
> 

Right, V1 will compute the buffer size with check_mul_overflow(), using 
the real CQE size, in the kernel and the user paths.

Yishai