[PATCH] iser-target: Fix possible use-after-free in connection establishment error

[PATCH] iser-target: Fix possible use-after-free in connection establishment error

[Sagi Grimberg]

In case we fail to establish the connection we must drain our pre-posted
login recieve work request before continuing safely with connection
teardown.

Reported-by: Amrani, Ram <Ram.Amrani-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
---
 drivers/infiniband/ulp/isert/ib_isert.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index ceabdb85df8b..9d4785ba24cb 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -741,6 +741,7 @@ isert_connect_error(struct rdma_cm_id *cma_id)
 {
 	struct isert_conn *isert_conn = cma_id->qp->qp_context;
 
+	ib_drain_qp(isert_conn->qp);
 	list_del_init(&isert_conn->node);
 	isert_conn->cm_id = NULL;
 	isert_put_conn(isert_conn);
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iser-target: Fix possible use-after-free in connection establishment error

[Sagi Grimberg]

Oops,

This should go via the target tree, CCing target-devel and nab

On 11/26/2017 03:31 PM, Sagi Grimberg wrote:
> In case we fail to establish the connection we must drain our pre-posted
> login recieve work request before continuing safely with connection
> teardown.
> 
> Reported-by: Amrani, Ram <Ram.Amrani-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
> ---
>   drivers/infiniband/ulp/isert/ib_isert.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index ceabdb85df8b..9d4785ba24cb 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> @@ -741,6 +741,7 @@ isert_connect_error(struct rdma_cm_id *cma_id)
>   {
>   	struct isert_conn *isert_conn = cma_id->qp->qp_context;
>   
> +	ib_drain_qp(isert_conn->qp);
>   	list_del_init(&isert_conn->node);
>   	isert_conn->cm_id = NULL;
>   	isert_put_conn(isert_conn);
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] iser-target: Fix possible use-after-free in connection establishment error

[Amrani, Ram]

> On 11/26/2017 03:31 PM, Sagi Grimberg wrote:
> > In case we fail to establish the connection we must drain our pre-posted
> > login recieve work request before continuing safely with connection
> > teardown.
> >
> > Reported-by: Amrani, Ram <Ram.Amrani@cavium.com>
> > Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> > ---
> >   drivers/infiniband/ulp/isert/ib_isert.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> > index ceabdb85df8b..9d4785ba24cb 100644
> > --- a/drivers/infiniband/ulp/isert/ib_isert.c
> > +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> > @@ -741,6 +741,7 @@ isert_connect_error(struct rdma_cm_id *cma_id)
> >   {
> >   	struct isert_conn *isert_conn = cma_id->qp->qp_context;
> >
> > +	ib_drain_qp(isert_conn->qp);
> >   	list_del_init(&isert_conn->node);
> >   	isert_conn->cm_id = NULL;
> >   	isert_put_conn(isert_conn);

With this patch our test behaves as expected.

Thanks,
Ram

RE: [PATCH] iser-target: Fix possible use-after-free in connection establishment error

[Amrani, Ram]

Hi Sagi,

> > Reported-by: Amrani, Ram <Ram.Amrani@cavium.com>
> > Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> > ---
> >   drivers/infiniband/ulp/isert/ib_isert.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> > index ceabdb85df8b..9d4785ba24cb 100644
> > --- a/drivers/infiniband/ulp/isert/ib_isert.c
> > +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> > @@ -741,6 +741,7 @@ isert_connect_error(struct rdma_cm_id *cma_id)
> >   {
> >   	struct isert_conn *isert_conn = cma_id->qp->qp_context;
> >
> > +	ib_drain_qp(isert_conn->qp);
> >   	list_del_init(&isert_conn->node);
> >   	isert_conn->cm_id = NULL;
> >   	isert_put_conn(isert_conn);
> >
> --

If it's not already committed, can you add a "Fixes:" so stable maintainers will catch it?

Thanks,
Ram

Re: [PATCH] iser-target: Fix possible use-after-free in connection establishment error

[Sagi Grimberg]

> If it's not already committed, can you add a "Fixes:" so stable maintainers will catch it?

I'm not sure at all that there is a clear cut commit that this fixes,
both the target code and the rdma interface code changed a lot so I
can't find any clear indication which broke this.

I can send a separate patch to stable from when we introduced
ib_drain_qp though.

Re: [PATCH] iser-target: Fix possible use-after-free in connection establishment error

[Max Gurtovoy]

hi Guys,
did we forget taking this fix to mainline/stable or we found another 
solution ?

On 11/30/2017 12:46 AM, Sagi Grimberg wrote:
> 
>> If it's not already committed, can you add a "Fixes:" so stable 
>> maintainers will catch it?
> 
> I'm not sure at all that there is a clear cut commit that this fixes,
> both the target code and the rdma interface code changed a lot so I
> can't find any clear indication which broke this.
> 
> I can send a separate patch to stable from when we introduced
> ib_drain_qp though.
> -- 
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.kernel.org%2Fmajordomo-info.html&data=02%7C01%7Cmaxg%40mellanox.com%7C18cac1be99bb4d99661c08d5377b08c6%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636475923920075321&sdata=WDJ6wr2o8jFyyGdHcLo06EhE18GTz9wUbF%2BezWzkGbc%3D&reserved=0 
> 

-Max.

Re: [PATCH] iser-target: Fix possible use-after-free in connection establishment error

[Jason Gunthorpe]

On Tue, Jan 09, 2018 at 07:04:16PM +0200, Max Gurtovoy wrote:
> hi Guys,
> did we forget taking this fix to mainline/stable or we found another
> solution ?

I thought it was going to NAB's tree.. My mistake, I put it back into
patchworks and it will get into for-next

Jason

RE: [PATCH] iser-target: Fix possible use-after-free in connection establishment error

[Amrani, Ram]

> I thought it was going to NAB's tree.. My mistake, I put it back into
> patchworks and it will get into for-next
> 
> Jason

Thanks.
BTW, you can use for-rc, as it is a bug fix.

Ram

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iser-target: Fix possible use-after-free in connection establishment error

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 727 bytes --]

On Tue, 2018-01-09 at 10:17 -0700, Jason Gunthorpe wrote:
> On Tue, Jan 09, 2018 at 07:04:16PM +0200, Max Gurtovoy wrote:
> > hi Guys,
> > did we forget taking this fix to mainline/stable or we found another
> > solution ?
> 
> I thought it was going to NAB's tree.. My mistake, I put it back into
> patchworks and it will get into for-next

In fairness, the email thread said exactly that.  But, it never got
picked up by NAB.  And being that it's in our tree and calling an rdma-
core function, it can easily enough go through our tree, so I applied it
to for-rc.

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] iser-target: Fix possible use-after-free in connection establishment error

[Nicholas A. Bellinger]

On Wed, 2018-01-10 at 16:54 -0500, Doug Ledford wrote:
> On Tue, 2018-01-09 at 10:17 -0700, Jason Gunthorpe wrote:
> > On Tue, Jan 09, 2018 at 07:04:16PM +0200, Max Gurtovoy wrote:
> > > hi Guys,
> > > did we forget taking this fix to mainline/stable or we found another
> > > solution ?
> > 
> > I thought it was going to NAB's tree.. My mistake, I put it back into
> > patchworks and it will get into for-next
> 
> In fairness, the email thread said exactly that.  But, it never got
> picked up by NAB.  And being that it's in our tree and calling an rdma-
> core function, it can easily enough go through our tree, so I applied it
> to for-rc.

Thanks Sagi + Ram, and Doug for picking this up.

Per the earlier discussion, it looks like this needs a stable CC,
right..?  As Sagi mentioned, it's not clear if this is a regression vs.
generic RDMA READ/WRITE API logic, or a day one issue.

So how about adding a 4.7+ stable tag, to match commit a060b5629..?

Re: [PATCH] iser-target: Fix possible use-after-free in connection establishment error

[Jason Gunthorpe]

On Fri, Jan 12, 2018 at 09:46:05PM -0800, Nicholas A. Bellinger wrote:
> On Wed, 2018-01-10 at 16:54 -0500, Doug Ledford wrote:
> > On Tue, 2018-01-09 at 10:17 -0700, Jason Gunthorpe wrote:
> > > On Tue, Jan 09, 2018 at 07:04:16PM +0200, Max Gurtovoy wrote:
> > > > hi Guys,
> > > > did we forget taking this fix to mainline/stable or we found another
> > > > solution ?
> > > 
> > > I thought it was going to NAB's tree.. My mistake, I put it back into
> > > patchworks and it will get into for-next
> > 
> > In fairness, the email thread said exactly that.  But, it never got
> > picked up by NAB.  And being that it's in our tree and calling an rdma-
> > core function, it can easily enough go through our tree, so I applied it
> > to for-rc.
> 
> Thanks Sagi + Ram, and Doug for picking this up.
> 
> Per the earlier discussion, it looks like this needs a stable CC,
> right..?  As Sagi mentioned, it's not clear if this is a regression vs.
> generic RDMA READ/WRITE API logic, or a day one issue.
> 
> So how about adding a 4.7+ stable tag, to match commit a060b5629..?

That can be done:

Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API")
Cc: <stable@vger.kernel.org> # 4.7+
Reported-by: Amrani, Ram <Ram.Amrani@cavium.com>
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Doug Ledford <dledford@redhat.com>

Doug: I took your wip/dl-for-rc branch, revised the commit message and
pushed it to for-rc.

Jason

Re: [PATCH] iser-target: Fix possible use-after-free in connection establishment error

[Sagi Grimberg]

Thanks Jason for picking it up, and sorry for the
late reply on this.

> That can be done:
> 
> Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API")

This patch is not the offending patch, if at all its this one:

Fixes: 572a143489a1 ("iser-target: Use ib_drain_qp")

But, the missing qp drain existed way before this commit.
I think any fixes tag would be confusing as there isn't a clear
culprit for this one...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html