* [PATCH] staging/rdma/hfi1: Fix a possible null pointer dereference @ 2015-12-10 16:13 Mike Marciniszyn 2015-12-14 13:28 ` Nicholas Mc Guire 0 siblings, 1 reply; 5+ messages in thread From: Mike Marciniszyn @ 2015-12-10 16:13 UTC (permalink / raw) To: devel; +Cc: linux-rdma, dledford, linux-next From: Easwar Hariharan <easwar.hariharan@intel•com> A code inspection pointed out that kmalloc_array may return NULL and memset doesn't check the input pointer for NULL, resulting in a possible NULL dereference. This patch fixes this. Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel•com> Signed-off-by: Easwar Hariharan <easwar.hariharan@intel•com> --- drivers/staging/rdma/hfi1/chip.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c index dc69159..49d49b2 100644 --- a/drivers/staging/rdma/hfi1/chip.c +++ b/drivers/staging/rdma/hfi1/chip.c @@ -10129,6 +10129,8 @@ static void init_qos(struct hfi1_devdata *dd, u32 first_ctxt) if (num_vls * qpns_per_vl > dd->chip_rcv_contexts) goto bail; rsmmap = kmalloc_array(NUM_MAP_REGS, sizeof(u64), GFP_KERNEL); + if (!rsmmap) + goto bail; memset(rsmmap, rxcontext, NUM_MAP_REGS * sizeof(u64)); /* init the local copy of the table */ for (i = 0, ctxt = first_ctxt; i < num_vls; i++) { ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] staging/rdma/hfi1: Fix a possible null pointer dereference 2015-12-10 16:13 [PATCH] staging/rdma/hfi1: Fix a possible null pointer dereference Mike Marciniszyn @ 2015-12-14 13:28 ` Nicholas Mc Guire 2015-12-18 6:33 ` [Cocci] " Julia Lawall 0 siblings, 1 reply; 5+ messages in thread From: Nicholas Mc Guire @ 2015-12-14 13:28 UTC (permalink / raw) To: Mike Marciniszyn; +Cc: devel, linux-rdma, dledford, linux-next, Cocci On Thu, Dec 10, 2015 at 11:13:38AM -0500, Mike Marciniszyn wrote: > From: Easwar Hariharan <easwar.hariharan@intel•com> > > A code inspection pointed out that kmalloc_array may return NULL and > memset doesn't check the input pointer for NULL, resulting in a possible > NULL dereference. This patch fixes this. > > Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel•com> > Signed-off-by: Easwar Hariharan <easwar.hariharan@intel•com> > --- > drivers/staging/rdma/hfi1/chip.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c > index dc69159..49d49b2 100644 > --- a/drivers/staging/rdma/hfi1/chip.c > +++ b/drivers/staging/rdma/hfi1/chip.c > @@ -10129,6 +10129,8 @@ static void init_qos(struct hfi1_devdata *dd, u32 first_ctxt) > if (num_vls * qpns_per_vl > dd->chip_rcv_contexts) > goto bail; > rsmmap = kmalloc_array(NUM_MAP_REGS, sizeof(u64), GFP_KERNEL); > + if (!rsmmap) > + goto bail; > memset(rsmmap, rxcontext, NUM_MAP_REGS * sizeof(u64)); > /* init the local copy of the table */ > for (i = 0, ctxt = first_ctxt; i < num_vls; i++) { > > -- Based on this report a generalization of unchecked use turned up one more case in the current kernel (patch sent). Probably the when block needs some cleanup, but findings like this definitely are a case for coccinelle scanners. <snip> /// check for missing NULL check before use // // missing check in: // ./drivers/staging/rdma/hfi1/chip.c:10131 unchecked allocation // in -next-20151214 // reported-by Mike Marciniszyn <mike.marciniszyn@intel•com> // // after generalization this also found: // ./drivers/clk/shmobile/clk-div6.c:197 unchecked allocation virtual context virtual org virtual report @badmemset@ expression mem; position p; statement S; @@ <+... *mem = kmalloc_array@p(...); ... when != if (!mem || ...) S when != if (... && !mem) S when != if (mem == NULL || ...) S when != if (... && mem == NULL) S when != if (unlikely(mem == NULL)) S when != if (unlikely(!mem)) S when != if (likely(!mem)) S when != if (likely(mem == NULL)) S return; ...+> @script:python@ p << badmemset.p; @@ print "%s:%s unchecked allocation" % (p[0].file,p[0].line) <snip> thx! hofrat ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Cocci] [PATCH] staging/rdma/hfi1: Fix a possible null pointer dereference 2015-12-14 13:28 ` Nicholas Mc Guire @ 2015-12-18 6:33 ` Julia Lawall [not found] ` <alpine.DEB.2.02.1512180727310.2052-bi+AKbBUZKagILUCTcTcHdKyNwTtLsGr@public.gmane.org> 0 siblings, 1 reply; 5+ messages in thread From: Julia Lawall @ 2015-12-18 6:33 UTC (permalink / raw) To: Nicholas Mc Guire; +Cc: devel, linux-rdma, dledford, linux-next, Cocci On Mon, 14 Dec 2015, Nicholas Mc Guire wrote: > On Thu, Dec 10, 2015 at 11:13:38AM -0500, Mike Marciniszyn wrote: > > From: Easwar Hariharan <easwar.hariharan@intel•com> > > > > A code inspection pointed out that kmalloc_array may return NULL and > > memset doesn't check the input pointer for NULL, resulting in a possible > > NULL dereference. This patch fixes this. > > > > Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel•com> > > Signed-off-by: Easwar Hariharan <easwar.hariharan@intel•com> > > --- > > drivers/staging/rdma/hfi1/chip.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c > > index dc69159..49d49b2 100644 > > --- a/drivers/staging/rdma/hfi1/chip.c > > +++ b/drivers/staging/rdma/hfi1/chip.c > > @@ -10129,6 +10129,8 @@ static void init_qos(struct hfi1_devdata *dd, u32 first_ctxt) > > if (num_vls * qpns_per_vl > dd->chip_rcv_contexts) > > goto bail; > > rsmmap = kmalloc_array(NUM_MAP_REGS, sizeof(u64), GFP_KERNEL); > > + if (!rsmmap) > > + goto bail; > > memset(rsmmap, rxcontext, NUM_MAP_REGS * sizeof(u64)); > > /* init the local copy of the table */ > > for (i = 0, ctxt = first_ctxt; i < num_vls; i++) { > > > > -- > > Based on this report a generalization of unchecked use turned up one more > case in the current kernel (patch sent). Probably the when block needs > some cleanup, but findings like this definitely are a case for coccinelle > scanners. > > <snip> > /// check for missing NULL check before use > // > // missing check in: > // ./drivers/staging/rdma/hfi1/chip.c:10131 unchecked allocation > // in -next-20151214 > // reported-by Mike Marciniszyn <mike.marciniszyn@intel•com> > // > // after generalization this also found: > // ./drivers/clk/shmobile/clk-div6.c:197 unchecked allocation > > virtual context > virtual org > virtual report > > @badmemset@ > expression mem; > position p; > statement S; > @@ > > <+... > *mem = kmalloc_array@p(...); > ... when != if (!mem || ...) S > when != if (... && !mem) S > when != if (mem == NULL || ...) S > when != if (... && mem == NULL) S > when != if (unlikely(mem == NULL)) S > when != if (unlikely(!mem)) S > when != if (likely(!mem)) S > when != if (likely(mem == NULL)) S > return; > ...+> > > @script:python@ > p << badmemset.p; > @@ > > print "%s:%s unchecked allocation" % (p[0].file,p[0].line) > > <snip> How about the following? I got two hits with this, in drivers/clk/shmobile/clk-div6.c and drivers/staging/rdma/hfi1/chip.c. @@ expression mem; identifier f; @@ *mem = kmalloc_array(...); ... when != mem == NULL when != mem != NULL ( f(...,mem,...) | mem->f | mem[...] ) There is a semantic patch in the kernel called kmerr that goes in this direction, but it seems to be overly restrictive and it doesn't address this function: /// This semantic patch looks for kmalloc etc that are not followed by a /// NULL check. It only gives a report in the case where there is some /// error handling code later in the function, which may be helpful /// in determining what the error handling code for the call to kmalloc etc /// should be. Probably there are a lot of other functions that should be considered. julia ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <alpine.DEB.2.02.1512180727310.2052-bi+AKbBUZKagILUCTcTcHdKyNwTtLsGr@public.gmane.org>]
* Re: [Cocci] [PATCH] staging/rdma/hfi1: Fix a possible null pointer dereference [not found] ` <alpine.DEB.2.02.1512180727310.2052-bi+AKbBUZKagILUCTcTcHdKyNwTtLsGr@public.gmane.org> @ 2015-12-18 14:20 ` Nicholas Mc Guire 2015-12-20 12:59 ` Julia Lawall 0 siblings, 1 reply; 5+ messages in thread From: Nicholas Mc Guire @ 2015-12-18 14:20 UTC (permalink / raw) To: Julia Lawall Cc: Mike Marciniszyn, devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, linux-rdma-u79uwXL29TY76Z2rM5mHXA, dledford-H+wXaHxf7aLQT0dZR+AlfA, linux-next-u79uwXL29TY76Z2rM5mHXA, Cocci-/FJkirnvOdkvYVN+rsErww On Fri, Dec 18, 2015 at 07:33:36AM +0100, Julia Lawall wrote: > > > On Mon, 14 Dec 2015, Nicholas Mc Guire wrote: > > > On Thu, Dec 10, 2015 at 11:13:38AM -0500, Mike Marciniszyn wrote: > > > From: Easwar Hariharan <easwar.hariharan-ral2JQCrhuEAvxtiuMwx3w@public•gmane.org> > > > > > > A code inspection pointed out that kmalloc_array may return NULL and > > > memset doesn't check the input pointer for NULL, resulting in a possible > > > NULL dereference. This patch fixes this. > > > > > > Reviewed-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public•gmane.org> > > > Signed-off-by: Easwar Hariharan <easwar.hariharan-ral2JQCrhuEAvxtiuMwx3w@public•gmane.org> > > > --- > > > drivers/staging/rdma/hfi1/chip.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c > > > index dc69159..49d49b2 100644 > > > --- a/drivers/staging/rdma/hfi1/chip.c > > > +++ b/drivers/staging/rdma/hfi1/chip.c > > > @@ -10129,6 +10129,8 @@ static void init_qos(struct hfi1_devdata *dd, u32 first_ctxt) > > > if (num_vls * qpns_per_vl > dd->chip_rcv_contexts) > > > goto bail; > > > rsmmap = kmalloc_array(NUM_MAP_REGS, sizeof(u64), GFP_KERNEL); > > > + if (!rsmmap) > > > + goto bail; > > > memset(rsmmap, rxcontext, NUM_MAP_REGS * sizeof(u64)); > > > /* init the local copy of the table */ > > > for (i = 0, ctxt = first_ctxt; i < num_vls; i++) { > > > > > > -- > > > > Based on this report a generalization of unchecked use turned up one more > > case in the current kernel (patch sent). Probably the when block needs > > some cleanup, but findings like this definitely are a case for coccinelle > > scanners. > > > > <snip> > > /// check for missing NULL check before use > > // > > // missing check in: > > // ./drivers/staging/rdma/hfi1/chip.c:10131 unchecked allocation > > // in -next-20151214 > > // reported-by Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public•gmane.org> > > // > > // after generalization this also found: > > // ./drivers/clk/shmobile/clk-div6.c:197 unchecked allocation > > > > virtual context > > virtual org > > virtual report > > > > @badmemset@ > > expression mem; > > position p; > > statement S; > > @@ > > > > <+... > > *mem = kmalloc_array@p(...); > > ... when != if (!mem || ...) S > > when != if (... && !mem) S > > when != if (mem == NULL || ...) S > > when != if (... && mem == NULL) S > > when != if (unlikely(mem == NULL)) S > > when != if (unlikely(!mem)) S > > when != if (likely(!mem)) S > > when != if (likely(mem == NULL)) S > > return; > > ...+> > > > > @script:python@ > > p << badmemset.p; > > @@ > > > > print "%s:%s unchecked allocation" % (p[0].file,p[0].line) > > > > <snip> > > How about the following? I got two hits with this, in > drivers/clk/shmobile/clk-div6.c and drivers/staging/rdma/hfi1/chip.c. > > @@ > expression mem; > identifier f; > @@ > > *mem = kmalloc_array(...); > ... when != mem == NULL > when != mem != NULL > ( works perfectly for this case - thanks I actually initially used the "template" from api/alloc/kzalloc-simple.cocci but that did not catch all cases for the kmalloc_array scanner. Poping your proposal into kzalloc-simple.cocci seems to be finding quite a few additional cases will review them to see if there are any false positives - but a first scan did show that most of the reported cases seem to be valid. thx! hofrat -- 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Cocci] [PATCH] staging/rdma/hfi1: Fix a possible null pointer dereference 2015-12-18 14:20 ` Nicholas Mc Guire @ 2015-12-20 12:59 ` Julia Lawall 0 siblings, 0 replies; 5+ messages in thread From: Julia Lawall @ 2015-12-20 12:59 UTC (permalink / raw) To: Nicholas Mc Guire; +Cc: devel, linux-rdma, dledford, linux-next, Cocci Here is my proposition for finding missing NULL tests. I tried to limit it to generic kmalloc like functions. There are of course many other NULL returning functions, but maybe they could be in an other rule, julia --- /// Look for kmalloc etc that are not followed by a NULL check. //# May give a false positive when the dereference is an argument of sizeof, or //# when the value is passed to another function that returns an error code. /// // Confidence: Moderate // Copyright: (C) 2015 Julia Lawall, Inria. GPLv2. // URL: http://coccinelle.lip6.fr/ // Options: --no-includes --include-headers virtual context virtual org virtual report @ok forall@ expression x; position p; statement S1,S2; @@ ( x =@p \(vmalloc\|kmalloc\|kzalloc\|kcalloc\|kmem_cache_alloc\|krealloc\| kmemdup\|kstrdup\|devm_kzalloc\|devm_kmalloc\|devm_kcalloc\| devm_kasprintf\|devm_kstrdup\|kmalloc_array\) (...,<+... __GFP_NOFAIL ...+>,...); | x =@p \(vmalloc\|kmalloc\|kzalloc\|kcalloc\|kmem_cache_alloc\|krealloc\| kmemdup\|kstrdup\|devm_kzalloc\|devm_kmalloc\|devm_kcalloc\| devm_kasprintf\|devm_kstrdup\|kmalloc_array\)(...) ... when != x ( if (x || ...) S1 else S2 | (x) == NULL | (x) != NULL | (x) == 0 | (x) != 0 ) ) // ---------------------------------------------------------------------------- @err depends on context || org || report exists@ identifier fld; position p != ok.p; expression x, y; position j0, j1, j2; @@ * x@j0 =@p \(vmalloc@j1\|kmalloc@j1\|kzalloc@j1\|kcalloc@j1\| kmem_cache_alloc@j1\|krealloc@j1\|kmemdup@j1\|kstrdup@j1\| devm_kzalloc@j1\|devm_kmalloc@j1\|devm_kcalloc@j1\| devm_kasprintf@j1\|devm_kstrdup@j1\|kmalloc_array@j1\)(...); ... when != (x) == NULL when != (x) != NULL when != (x) == 0 when != (x) != 0 when != x = y ( x@j2->fld | *x@j2 | x@j2[...] ) // ---------------------------------------------------------------------------- @script:python err_org depends on org@ j0 << err.j0; j1 << err.j1; j2 << err.j2; @@ msg = "NULL test needed." coccilib.org.print_todo(j0[0], msg) coccilib.org.print_link(j1[0], "") coccilib.org.print_link(j2[0], "") // ---------------------------------------------------------------------------- @script:python err_report depends on report@ j0 << err.j0; j1 << err.j1; j2 << err.j2; @@ msg = "NULL test needed, around lines %s,%s." % (j1[0].line,j2[0].line) coccilib.report.print_report(j0[0], msg) ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-12-20 12:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-12-10 16:13 [PATCH] staging/rdma/hfi1: Fix a possible null pointer dereference Mike Marciniszyn
2015-12-14 13:28 ` Nicholas Mc Guire
2015-12-18 6:33 ` [Cocci] " Julia Lawall
[not found] ` <alpine.DEB.2.02.1512180727310.2052-bi+AKbBUZKagILUCTcTcHdKyNwTtLsGr@public.gmane.org>
2015-12-18 14:20 ` Nicholas Mc Guire
2015-12-20 12:59 ` Julia Lawall
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox