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 <[email protected]> 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/[email protected]?part=1
