Re: [PATCH rdma-next 1/2] RDMA/bnxt_re: Validate udata before destroying resources

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing hash_del on error paths in CQ and SRQ creation leads to hash table corruption and use-after-free.
- [Critical] Lockless modifications and iterations over device-global hash tables (cq_hash and srq_hash) risk linked-list corruption.
- [High] Hardware Address Handle (AH) resource leak in bnxt_re_create_ah on udata response failure.
- [High] Hardware and driver state modification applied before udata validation in modify/resize callbacks.
- [High] Memory leak of rdma_user_mmap_entry in bnxt_re_alloc_ucontext if udata validation or response fails.
--

--- Patch [1]: [PATCH rdma-next 1/2] RDMA/bnxt_re: Validate udata before destroying resources ---
commit 1661d6c7bf0fdc123a46584fd78311e626482940
Author: Leon Romanovsky <leon@kernel.org>

RDMA/bnxt_re: Validate udata before destroying resources

This commit modifies the destroy callbacks to validate user data earlier
using ib_no_udata_io() before resources are torn down. This prevents
userspace access failures from leaving the driver and hardware in an
inconsistent state.

> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> index 5657625290076..ef9943be18861 100644
> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c

[ ... ]

> @@ -3471,7 +3471,7 @@ int bnxt_re_destroy_cq(struct ib_cq *ib_cq, struct ib_udata *udata)
>  	nq = cq->qplib_cq.nq;
>  	cctx = rdev->chip_ctx;
>  
> -	ret = ib_is_udata_in_empty(udata);
> +	ret = ib_no_udata_io(udata);
>  	if (ret)
>  		return ret;

[Severity: Critical]
This is a pre-existing issue, but does removing the CQ from the global hash
table here require locking?

In bnxt_re_destroy_cq(), just after this udata validation, the CQ is deleted
from rdev->cq_hash via hash_del() without any locks:

    if (cctx->modes.toggle_bits & BNXT_QPLIB_CQ_TOGGLE_BIT)
        hash_del(&cq->hash_entry);

However, concurrent additions occur in bnxt_re_create_user_cq() via
hash_add() and lockless iterations happen in bnxt_re_search_for_cq(). Can
this cause list corruption or use-after-free if multiple threads create,
destroy, or query CQs concurrently?

[Severity: Critical]
This isn't a bug introduced by this patch, but can this leak a dangling
pointer into the hash table on error?

In bnxt_re_create_user_cq(), if memory allocation for the page fails or
ib_respond_udata() fails, we jump to the error path and free the CQ without
removing it from the hash table:

bnxt_re_create_user_cq() {
    ...
    if (cctx->modes.toggle_bits & BNXT_QPLIB_CQ_TOGGLE_BIT) {
        hash_add(rdev->cq_hash, &cq->hash_entry, cq->qplib_cq.id);
        /* Allocate a page */
        cq->uctx_cq_page = (void *)get_zeroed_page(GFP_KERNEL);
        if (!cq->uctx_cq_page) {
            rc = -ENOMEM;
            goto destroy_cq;
        }
    ...
    rc = ib_respond_udata(udata, resp);
    if (rc)
        goto free_mem;
    ...
free_mem:
    free_page((unsigned long)cq->uctx_cq_page);
destroy_cq:
    bnxt_qplib_destroy_cq(&rdev->qplib_res, &cq->qplib_cq);
free_umem:
    ib_umem_release(cq->umem);
    return rc;
}

Since hash_del() isn't called here, could subsequent lookups hit a
use-after-free? This same pattern also appears in bnxt_re_create_srq()
where srq_hash is modified.

[Severity: High]
This isn't a bug introduced by this patch, but since this patch fixes udata
validation in destroy callbacks, should similar fixes be applied to the
modify and resize callbacks?

In functions like bnxt_re_modify_srq(), bnxt_re_modify_qp(), and
bnxt_re_resize_cq(), the hardware and driver state is modified before the
final copy-to-user response:

drivers/infiniband/hw/bnxt_re/ib_verbs.c:bnxt_re_modify_qp() {
    ...
    /* hardware QP state is modified here */
    ...
    return ib_respond_empty_udata(udata);
}

If the udata response buffer is invalid, the function fails, but the hardware
and driver states are already updated. Can this desynchronize kernel state
from userspace state?

[Severity: High]
This is a pre-existing issue, but does this error path permanently leak the
hardware Address Handle?

If ib_respond_empty_udata() fails in bnxt_re_create_ah(), the function
returns the error directly without cleaning up the hardware AH or
decrementing ah_count:

drivers/infiniband/hw/bnxt_re/ib_verbs.c:bnxt_re_create_ah() {
    ...
    return ib_respond_empty_udata(udata);
}

Can an unprivileged application repeatedly trigger this by passing an invalid
udata response buffer to exhaust hardware resources and kernel memory?

[Severity: High]
This is a pre-existing issue, but does the cfail error path leak the mmap
entry?

In bnxt_re_alloc_ucontext(), if validation or the udata response fails, we
jump to cfail:

drivers/infiniband/hw/bnxt_re/ib_verbs.c:bnxt_re_alloc_ucontext() {
    ...
    rc = ib_respond_udata(udata, resp);
    if (rc) {
        rc = -EFAULT;
        goto cfail;
    }
    ...
cfail:
    free_page((unsigned long)uctx->shpg);
    uctx->shpg = NULL;
fail:
    return rc;
}

This path skips calling rdma_user_mmap_entry_remove(uctx->shpage_mmap).
Does this permanently leak the kzalloc'd mmap entry, allowing memory
exhaustion if triggered repeatedly by userspace?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-fix-destroy-no-udata-v1-0-fcca2e34fd57@nvidia.com?part=1