[PATCH] IB/hfi1: Prevent a NULL dereference

[PATCH] IB/hfi1: Prevent a NULL dereference

[Dan Carpenter]

In the original code, we set "fd->uctxt" to NULL and then dereference it
which will cause an Oops.

Fixes: f2a3bc00a03c ("IB/hfi1: Protect context array set/clear with spinlock")
Signed-off-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

diff --git a/drivers/infiniband/hw/hfi1/file_ops.c b/drivers/infiniband/hw/hfi1/file_ops.c
index 82086241aac3..3de1ac94bb85 100644
--- a/drivers/infiniband/hw/hfi1/file_ops.c
+++ b/drivers/infiniband/hw/hfi1/file_ops.c
@@ -763,10 +763,10 @@ static int complete_subctxt(struct hfi1_filedata *fd)
 	}
 
 	if (ret) {
+		__clear_bit(fd->subctxt, fd->uctxt->in_use_ctxts);
 		hfi1_rcd_put(fd->uctxt);
 		fd->uctxt = NULL;
 		spin_lock_irqsave(&fd->dd->uctxt_lock, flags);
-		__clear_bit(fd->subctxt, fd->uctxt->in_use_ctxts);
 		spin_unlock_irqrestore(&fd->dd->uctxt_lock, flags);
 	}
 
--
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] IB/hfi1: Prevent a NULL dereference

[Dennis Dalessandro]

On 1/9/2018 4:27 AM, Dan Carpenter wrote:
> In the original code, we set "fd->uctxt" to NULL and then dereference it
> which will cause an Oops.
> 
> Fixes: f2a3bc00a03c ("IB/hfi1: Protect context array set/clear with spinlock")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/infiniband/hw/hfi1/file_ops.c b/drivers/infiniband/hw/hfi1/file_ops.c
> index 82086241aac3..3de1ac94bb85 100644
> --- a/drivers/infiniband/hw/hfi1/file_ops.c
> +++ b/drivers/infiniband/hw/hfi1/file_ops.c
> @@ -763,10 +763,10 @@ static int complete_subctxt(struct hfi1_filedata *fd)
>   	}
>   
>   	if (ret) {
> +		__clear_bit(fd->subctxt, fd->uctxt->in_use_ctxts);
>   		hfi1_rcd_put(fd->uctxt);
>   		fd->uctxt = NULL;
>   		spin_lock_irqsave(&fd->dd->uctxt_lock, flags);
> -		__clear_bit(fd->subctxt, fd->uctxt->in_use_ctxts);
>   		spin_unlock_irqrestore(&fd->dd->uctxt_lock, flags);
>   	}
>   
> 

Thanks!

Acked-by: Dennis Dalessandro <dennis.dalessandro@intel.com>

RE: [PATCH] IB/hfi1: Prevent a NULL dereference

[Ruhl, Michael J]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org]
> Sent: Tuesday, January 9, 2018 4:27 AM
> To: Marciniszyn, Mike <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; Ruhl, Michael J
> <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Dalessandro, Dennis <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; Doug Ledford
> <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>; Jason Gunthorpe <jgg-uk2M96/98Pc@public.gmane.org>; linux-
> rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; kernel-janitors-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: [PATCH] IB/hfi1: Prevent a NULL dereference
> 
> In the original code, we set "fd->uctxt" to NULL and then dereference it
> which will cause an Oops.
> 
> Fixes: f2a3bc00a03c ("IB/hfi1: Protect context array set/clear with spinlock")
> Signed-off-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> 
> diff --git a/drivers/infiniband/hw/hfi1/file_ops.c
> b/drivers/infiniband/hw/hfi1/file_ops.c
> index 82086241aac3..3de1ac94bb85 100644
> --- a/drivers/infiniband/hw/hfi1/file_ops.c
> +++ b/drivers/infiniband/hw/hfi1/file_ops.c
> @@ -763,10 +763,10 @@ static int complete_subctxt(struct hfi1_filedata *fd)
>  	}
> 
>  	if (ret) {
> +		__clear_bit(fd->subctxt, fd->uctxt->in_use_ctxts);
>  		hfi1_rcd_put(fd->uctxt);
>  		fd->uctxt = NULL;
>  		spin_lock_irqsave(&fd->dd->uctxt_lock, flags);
> -		__clear_bit(fd->subctxt, fd->uctxt->in_use_ctxts);
>  		spin_unlock_irqrestore(&fd->dd->uctxt_lock, flags);
>  	}
> 

Hi Dan,

Thanks for catching this.

However, the patch is not quite correct.

The __clear_bit() spin_lock_irqsave/restore need stay together.  The patch should be:

diff --git a/drivers/infiniband/hw/hfi1/file_ops.c b/drivers/infiniband/hw/hfi1/
index 7750a9c..1df7da4 100644
--- a/drivers/infiniband/hw/hfi1/file_ops.c
+++ b/drivers/infiniband/hw/hfi1/file_ops.c
@@ -763,11 +763,11 @@ static int complete_subctxt(struct hfi1_filedata *fd)
        }
 
        if (ret) {
-               hfi1_rcd_put(fd->uctxt);
-               fd->uctxt = NULL;
                spin_lock_irqsave(&fd->dd->uctxt_lock, flags);
                __clear_bit(fd->subctxt, fd->uctxt->in_use_ctxts);
                spin_unlock_irqrestore(&fd->dd->uctxt_lock, flags);
+               hfi1_rcd_put(fd->uctxt);
+               fd->uctxt = NULL;
        }

--
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] IB/hfi1: Prevent a NULL dereference

[Dan Carpenter]

On Tue, Jan 09, 2018 at 02:16:59PM +0000, Ruhl, Michael J wrote:
> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > Sent: Tuesday, January 9, 2018 4:27 AM
> > To: Marciniszyn, Mike <mike.marciniszyn@intel.com>; Ruhl, Michael J
> > <michael.j.ruhl@intel.com>
> > Cc: Dalessandro, Dennis <dennis.dalessandro@intel.com>; Doug Ledford
> > <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca>; linux-
> > rdma@vger.kernel.org; kernel-janitors@vger.kernel.org
> > Subject: [PATCH] IB/hfi1: Prevent a NULL dereference
> > 
> > In the original code, we set "fd->uctxt" to NULL and then dereference it
> > which will cause an Oops.
> > 
> > Fixes: f2a3bc00a03c ("IB/hfi1: Protect context array set/clear with spinlock")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/drivers/infiniband/hw/hfi1/file_ops.c
> > b/drivers/infiniband/hw/hfi1/file_ops.c
> > index 82086241aac3..3de1ac94bb85 100644
> > --- a/drivers/infiniband/hw/hfi1/file_ops.c
> > +++ b/drivers/infiniband/hw/hfi1/file_ops.c
> > @@ -763,10 +763,10 @@ static int complete_subctxt(struct hfi1_filedata *fd)
> >  	}
> > 
> >  	if (ret) {
> > +		__clear_bit(fd->subctxt, fd->uctxt->in_use_ctxts);
> >  		hfi1_rcd_put(fd->uctxt);
> >  		fd->uctxt = NULL;
> >  		spin_lock_irqsave(&fd->dd->uctxt_lock, flags);
> > -		__clear_bit(fd->subctxt, fd->uctxt->in_use_ctxts);
> >  		spin_unlock_irqrestore(&fd->dd->uctxt_lock, flags);
> >  	}
> > 
> 
> Hi Dan,
> 
> Thanks for catching this.
> 
> However, the patch is not quite correct.
> 
> The __clear_bit() spin_lock_irqsave/restore need stay together.  The patch should be:
>

Oh.  Yeah.  I should have noticed that now the spin_lock is pointless.
Let me resend.  Thanks.

regards,
dan carpenter

[PATCH v2] IB/hfi1: Prevent a NULL dereference

[Dan Carpenter]

In the original code, we set "fd->uctxt" to NULL and then dereference it
which will cause an Oops.

Fixes: f2a3bc00a03c ("IB/hfi1: Protect context array set/clear with spinlock")
Signed-off-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
v2: In v1, I accidentally moved the __clear_bit() out from under the
    spin_lock().  Thanks for the review, Michael!

diff --git a/drivers/infiniband/hw/hfi1/file_ops.c b/drivers/infiniband/hw/hfi1/file_ops.c
index 82086241aac3..bd6f03cc5ee0 100644
--- a/drivers/infiniband/hw/hfi1/file_ops.c
+++ b/drivers/infiniband/hw/hfi1/file_ops.c
@@ -763,11 +763,11 @@ static int complete_subctxt(struct hfi1_filedata *fd)
 	}
 
 	if (ret) {
-		hfi1_rcd_put(fd->uctxt);
-		fd->uctxt = NULL;
 		spin_lock_irqsave(&fd->dd->uctxt_lock, flags);
 		__clear_bit(fd->subctxt, fd->uctxt->in_use_ctxts);
 		spin_unlock_irqrestore(&fd->dd->uctxt_lock, flags);
+		hfi1_rcd_put(fd->uctxt);
+		fd->uctxt = NULL;
 	}
 
 	return ret;
--
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 v2] IB/hfi1: Prevent a NULL dereference

[Ruhl, Michael J]

> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Dan Carpenter
> Sent: Tuesday, January 9, 2018 3:04 PM
> To: Marciniszyn, Mike <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Dalessandro, Dennis <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; Doug Ledford
> <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>; Jason Gunthorpe <jgg-uk2M96/98Pc@public.gmane.org>; linux-
> rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; kernel-janitors-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: [PATCH v2] IB/hfi1: Prevent a NULL dereference
> 
> In the original code, we set "fd->uctxt" to NULL and then dereference it
> which will cause an Oops.
> 
> Fixes: f2a3bc00a03c ("IB/hfi1: Protect context array set/clear with spinlock")
> Signed-off-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> ---
> v2: In v1, I accidentally moved the __clear_bit() out from under the
>     spin_lock().  Thanks for the review, Michael!
> 
> diff --git a/drivers/infiniband/hw/hfi1/file_ops.c
> b/drivers/infiniband/hw/hfi1/file_ops.c
> index 82086241aac3..bd6f03cc5ee0 100644
> --- a/drivers/infiniband/hw/hfi1/file_ops.c
> +++ b/drivers/infiniband/hw/hfi1/file_ops.c
> @@ -763,11 +763,11 @@ static int complete_subctxt(struct hfi1_filedata *fd)
>  	}
> 
>  	if (ret) {
> -		hfi1_rcd_put(fd->uctxt);
> -		fd->uctxt = NULL;
>  		spin_lock_irqsave(&fd->dd->uctxt_lock, flags);
>  		__clear_bit(fd->subctxt, fd->uctxt->in_use_ctxts);
>  		spin_unlock_irqrestore(&fd->dd->uctxt_lock, flags);
> +		hfi1_rcd_put(fd->uctxt);
> +		fd->uctxt = NULL;
>  	}
> 
>  	return ret;
> --

Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> # 4.14.x
Reviewed-by: Michael J. Ruhl <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Looks good.  Adding tag for stable too.

Thanks,

Mike

> 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
--
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 v2] IB/hfi1: Prevent a NULL dereference

[Doug Ledford]

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

On Tue, 2018-01-09 at 20:39 +0000, Ruhl, Michael J wrote:
> > -----Original Message-----
> > From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
> > owner@vger.kernel.org] On Behalf Of Dan Carpenter
> > Sent: Tuesday, January 9, 2018 3:04 PM
> > To: Marciniszyn, Mike <mike.marciniszyn@intel.com>
> > Cc: Dalessandro, Dennis <dennis.dalessandro@intel.com>; Doug Ledford
> > <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca>; linux-
> > rdma@vger.kernel.org; kernel-janitors@vger.kernel.org
> > Subject: [PATCH v2] IB/hfi1: Prevent a NULL dereference
> > 
> > In the original code, we set "fd->uctxt" to NULL and then dereference it
> > which will cause an Oops.
> > 
> > Fixes: f2a3bc00a03c ("IB/hfi1: Protect context array set/clear with spinlock")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > v2: In v1, I accidentally moved the __clear_bit() out from under the
> >     spin_lock().  Thanks for the review, Michael!
> > 
> > diff --git a/drivers/infiniband/hw/hfi1/file_ops.c
> > b/drivers/infiniband/hw/hfi1/file_ops.c
> > index 82086241aac3..bd6f03cc5ee0 100644
> > --- a/drivers/infiniband/hw/hfi1/file_ops.c
> > +++ b/drivers/infiniband/hw/hfi1/file_ops.c
> > @@ -763,11 +763,11 @@ static int complete_subctxt(struct hfi1_filedata *fd)
> >  	}
> > 
> >  	if (ret) {
> > -		hfi1_rcd_put(fd->uctxt);
> > -		fd->uctxt = NULL;
> >  		spin_lock_irqsave(&fd->dd->uctxt_lock, flags);
> >  		__clear_bit(fd->subctxt, fd->uctxt->in_use_ctxts);
> >  		spin_unlock_irqrestore(&fd->dd->uctxt_lock, flags);
> > +		hfi1_rcd_put(fd->uctxt);
> > +		fd->uctxt = NULL;
> >  	}
> > 
> >  	return ret;
> > --
> 
> Cc: <stable@vger.kernel.org> # 4.14.x

Unfortunately, patchworks doesn't pick up additional Cc: tags, only
reviewed-bys and acks.  In any case, I hand added it.  Thanks, applied
to for-rc.

> Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> 
> Looks good.  Adding tag for stable too.
> 
> Thanks,
> 
> Mike
> 
> > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
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 --]