Re: [PATCH rdma-next 1/3] RDMA/core: Preserve the selected source GID on failed AH modification

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] rdma: memory leak and UAF due to missing sgid_attr refcount in rvt_modify_ah()
--

commit cfb0c41a87200fc5df90876398650807044c23ad
Author: Leon Romanovsky <leon@kernel.org>

RDMA/core: Preserve the selected source GID on failed AH modification

When an address-handle modification fails, the core replaces its source-GID
reference. This commit ensures the selected source GID is preserved by only
updating ah->sgid_attr when the modification is successful.

> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> index edf62652f9d3e..7d52d30362087 100644
> --- a/drivers/infiniband/core/verbs.c
> +++ b/drivers/infiniband/core/verbs.c
> @@ -1011,7 +1011,8 @@ int rdma_modify_ah(struct ib_ah *ah, struct rdma_ah_attr *ah_attr)
>  		ah->device->ops.modify_ah(ah, ah_attr) :
>  		-EOPNOTSUPP;
>  
> -	ah->sgid_attr = rdma_update_sgid_attr(ah_attr, ah->sgid_attr);
> +	if (!ret)
> +		ah->sgid_attr = rdma_update_sgid_attr(ah_attr, ah->sgid_attr);
>  	rdma_unfill_sgid_attr(ah_attr, old_sgid_attr);

[Severity: High]
Does this sequence expose a memory leak and use-after-free in providers
that do a shallow copy of ah_attr?

For example, in drivers/infiniband/sw/rdmavt/ah.c:rvt_modify_ah(), the
structure is shallowly copied:

	ah->attr = *ah_attr;

This causes a memory leak of the old sgid_attr reference because
rdma_put_gid_attr() is not called on the old pointer, and it adopts the
new sgid_attr without incrementing its refcount.

When rdma_modify_ah() finishes, it calls rdma_unfill_sgid_attr() which
drops the temporary refcount that rvt_modify_ah() copied but didn't pin.
This leaves the driver's ah->attr exposed to a dangling pointer.

Later, rvt_destroy_ah() will unconditionally call:

	rdma_destroy_ah_attr(&ah->attr);

This calls rdma_put_gid_attr() on the unheld pointer, resulting in a
refcount underflow and use-after-free.

This path is concretely reachable via the hfi1 provider (for instance,
hfi1_modify_qp0_ah() uses rdma_modify_ah() with IB_AH_GRH enabled). 

Should providers like rdmavt be updated to use rdma_replace_ah_attr()
to properly manage these refcounts?

>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-ib-core-fixes-v1-0-60f6da3ddec3@nvidia.com?part=1