| Message ID | 20220124122502.GB31673@kili (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/cxgb4: fix refcounting leak in c4iw_ref_send_wait() | expand |
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
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
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 {
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(-)