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

[netdev-bot+sashiko@kernel.org]

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;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103163949.1554-1-yishaih%40nvidia.com