[PATCH] RDMA/cxgb4: fix refcounting leak in c4iw_ref_send_wait()

[PATCH] RDMA/cxgb4: fix refcounting leak in c4iw_ref_send_wait()

[Dan Carpenter]

Call c4iw_put_wr_wait() if c4iw_wait_for_reply() fails.  This
code uses kobject so the worst impact from this bug is a DoS.

Fixes: 2015f26cfade ("iw_cxgb4: add referencing to wait objects")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
From static analysis.  Not tested.

 drivers/infiniband/hw/cxgb4/iw_cxgb4.h | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
index 12f33467c672..1dc0e00aba5f 100644
--- a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
+++ b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
@@ -297,11 +297,18 @@ static inline int c4iw_ref_send_wait(struct c4iw_rdev *rdev,
 		 qpid);
 	c4iw_get_wr_wait(wr_waitp);
 	ret = c4iw_ofld_send(rdev, skb);
-	if (ret) {
-		c4iw_put_wr_wait(wr_waitp);
-		return ret;
-	}
-	return c4iw_wait_for_reply(rdev, wr_waitp, hwtid, qpid, func);
+	if (ret)
+		goto put_wait;
+
+	ret = c4iw_wait_for_reply(rdev, wr_waitp, hwtid, qpid, func);
+	if (ret)
+		goto put_wait;
+
+	return 0;
+
+put_wait:
+	c4iw_put_wr_wait(wr_waitp);
+	return ret;
 }
 
 enum db_state {
-- 
2.20.1

Re: [PATCH] RDMA/cxgb4: fix refcounting leak in c4iw_ref_send_wait()

[Jason Gunthorpe]

On Mon, Jan 24, 2022 at 03:25:02PM +0300, Dan Carpenter wrote:
> Call c4iw_put_wr_wait() if c4iw_wait_for_reply() fails.  This
> code uses kobject so the worst impact from this bug is a DoS.
> 
> Fixes: 2015f26cfade ("iw_cxgb4: add referencing to wait objects")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> >From static analysis.  Not tested.
> 
>  drivers/infiniband/hw/cxgb4/iw_cxgb4.h | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)

Are you sure?  

Looking at the caller alloc_srq_queue() it calls down to
c4iw_ref_send_wait() then immediately exits on failure

The only caller c4iw_create_srq() 

	ret = alloc_srq_queue(srq, ucontext ? &ucontext->uctx :
			&rhp->rdev.uctx, srq->wr_waitp);
	if (ret)
		goto err_free_skb;

And then

err_free_skb:
	kfree_skb(srq->destroy_skb);
err_free_srq_idx:
	c4iw_free_srq_idx(&rhp->rdev, srq->idx);
err_free_wr_wait:
	c4iw_put_wr_wait(srq->wr_waitp);

So we just double put the thing with this patch

I have no idea how this logic is supposed to work, and clearly
something is buggy in here,  but I can't say this is right..

Jason

Re: [PATCH] RDMA/cxgb4: fix refcounting leak in c4iw_ref_send_wait()

[Dan Carpenter]

On Fri, Jan 28, 2022 at 01:16:36PM -0400, Jason Gunthorpe wrote:
> On Mon, Jan 24, 2022 at 03:25:02PM +0300, Dan Carpenter wrote:
> > Call c4iw_put_wr_wait() if c4iw_wait_for_reply() fails.  This
> > code uses kobject so the worst impact from this bug is a DoS.
> > 
> > Fixes: 2015f26cfade ("iw_cxgb4: add referencing to wait objects")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > >From static analysis.  Not tested.
> > 
> >  drivers/infiniband/hw/cxgb4/iw_cxgb4.h | 17 ++++++++++++-----
> >  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> Are you sure?  
> 
> Looking at the caller alloc_srq_queue() it calls down to
> c4iw_ref_send_wait() then immediately exits on failure
> 
> The only caller c4iw_create_srq() 
> 
> 	ret = alloc_srq_queue(srq, ucontext ? &ucontext->uctx :
> 			&rhp->rdev.uctx, srq->wr_waitp);
> 	if (ret)
> 		goto err_free_skb;
> 
> And then
> 
> err_free_skb:
> 	kfree_skb(srq->destroy_skb);
> err_free_srq_idx:
> 	c4iw_free_srq_idx(&rhp->rdev, srq->idx);
> err_free_wr_wait:
> 	c4iw_put_wr_wait(srq->wr_waitp);
> 
> So we just double put the thing with this patch
> 
> I have no idea how this logic is supposed to work, and clearly
> something is buggy in here,  but I can't say this is right..

Yeah.  My patch isn't right.  That refcount from my patch is supposed to
be decremented in _c4iw_wake_up().  That function gets called when the
firmware responds in fw6_msg().  So if we cleanup everything and the
firmware sends a delayed response the it leads to a use after free.

Say the firmware never responds, then sure, that leads to a resource
leak.  But it's better to have a small memory leak than a use after free.

regards,
dan carpenter