[patch] FMC: NULL dereference on allocation failure

[patch] FMC: NULL dereference on allocation failure

[Dan Carpenter]

If we don't allocate "arr" then the cleanup path will dereference it and
oops.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/fmc/fmc-sdb.c b/drivers/fmc/fmc-sdb.c
index 74fb326..79adc39 100644
--- a/drivers/fmc/fmc-sdb.c
+++ b/drivers/fmc/fmc-sdb.c
@@ -46,16 +46,17 @@ static struct sdb_array *__fmc_scan_sdb_tree(struct fmc_device *fmc,
 	onew = __sdb_rd(fmc, sdb_addr + 4, convert);
 	n = __be16_to_cpu(*(uint16_t *)&onew);
 	arr = kzalloc(sizeof(*arr), GFP_KERNEL);
-	if (arr) {
-		arr->record = kzalloc(sizeof(arr->record[0]) * n, GFP_KERNEL);
-		arr->subtree = kzalloc(sizeof(arr->subtree[0]) * n, GFP_KERNEL);
-	}
-	if (!arr || !arr->record || !arr->subtree) {
+	if (!arr)
+		return ERR_PTR(-ENOMEM);
+	arr->record = kzalloc(sizeof(arr->record[0]) * n, GFP_KERNEL);
+	arr->subtree = kzalloc(sizeof(arr->subtree[0]) * n, GFP_KERNEL);
+	if (!arr->record || !arr->subtree) {
 		kfree(arr->record);
 		kfree(arr->subtree);
 		kfree(arr);
 		return ERR_PTR(-ENOMEM);
 	}
+
 	arr->len = n;
 	arr->level = level;
 	arr->fmc = fmc;

Re: [patch] FMC: NULL dereference on allocation failure

[Alessandro Rubini]

> If we don't allocate "arr" then the cleanup path will dereference it and
> oops.

You are right, thanks (acked).

How is the procedure here? I don't have my own git tree on
kernel.org for pull requests. Can this go through the janitors?

(if it makes sense, I can try the procedure to have a tree, but last
time I checked it was strictly denied to anyone).

thanks
/alessandro

Re: [patch] FMC: NULL dereference on allocation failure

[Dan Carpenter]

On Wed, Jun 19, 2013 at 05:25:28PM +0200, Alessandro Rubini wrote:
> > If we don't allocate "arr" then the cleanup path will dereference it and
> > oops.
> 
> You are right, thanks (acked).
> 
> How is the procedure here? I don't have my own git tree on
> kernel.org for pull requests. Can this go through the janitors?
> 
> (if it makes sense, I can try the procedure to have a tree, but last
> time I checked it was strictly denied to anyone).


Apparently these are going through Greg K-H.  I'll resend, with Greg
CC'd so he can pick it up from the mailing list.

Could you add an entry to the MAINTAINERS file so that Greg will be
CC'd automatically using get_maintainer.pl?  Is there a dedicated
list for FMC development?  That would go in the MAINTAINERS file as
well.

regards,
dan carpenter

[patch -next] FMC: NULL dereference on allocation failure

[Dan Carpenter]

If we don't allocate "arr" then the cleanup path will dereference it and
oops.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Alessandro Rubini <rubini@gnudd.com>

diff --git a/drivers/fmc/fmc-sdb.c b/drivers/fmc/fmc-sdb.c
index 74fb326..79adc39 100644
--- a/drivers/fmc/fmc-sdb.c
+++ b/drivers/fmc/fmc-sdb.c
@@ -46,16 +46,17 @@ static struct sdb_array *__fmc_scan_sdb_tree(struct fmc_device *fmc,
 	onew = __sdb_rd(fmc, sdb_addr + 4, convert);
 	n = __be16_to_cpu(*(uint16_t *)&onew);
 	arr = kzalloc(sizeof(*arr), GFP_KERNEL);
-	if (arr) {
-		arr->record = kzalloc(sizeof(arr->record[0]) * n, GFP_KERNEL);
-		arr->subtree = kzalloc(sizeof(arr->subtree[0]) * n, GFP_KERNEL);
-	}
-	if (!arr || !arr->record || !arr->subtree) {
+	if (!arr)
+		return ERR_PTR(-ENOMEM);
+	arr->record = kzalloc(sizeof(arr->record[0]) * n, GFP_KERNEL);
+	arr->subtree = kzalloc(sizeof(arr->subtree[0]) * n, GFP_KERNEL);
+	if (!arr->record || !arr->subtree) {
 		kfree(arr->record);
 		kfree(arr->subtree);
 		kfree(arr);
 		return ERR_PTR(-ENOMEM);
 	}
+
 	arr->len = n;
 	arr->level = level;
 	arr->fmc = fmc;
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch -next] FMC: NULL dereference on allocation failure

[Joe Perches]

On Wed, 2013-06-19 at 19:01 +0300, Dan Carpenter wrote:
> If we don't allocate "arr" then the cleanup path will dereference it and
> oops.
[]
> diff --git a/drivers/fmc/fmc-sdb.c b/drivers/fmc/fmc-sdb.c
[]
> @@ -46,16 +46,17 @@ static struct sdb_array *__fmc_scan_sdb_tree(struct fmc_device *fmc,
[]
> -		arr->record = kzalloc(sizeof(arr->record[0]) * n, GFP_KERNEL);
> -		arr->subtree = kzalloc(sizeof(arr->subtree[0]) * n, GFP_KERNEL);
[]
> +	arr->record = kzalloc(sizeof(arr->record[0]) * n, GFP_KERNEL);
> +	arr->subtree = kzalloc(sizeof(arr->subtree[0]) * n, GFP_KERNEL);

n comes from the hardware no?
Maybe make these kcalloc too.

Re: [patch -next] FMC: NULL dereference on allocation failure

[Alessandro Rubini]

>> +	arr->record = kzalloc(sizeof(arr->record[0]) * n, GFP_KERNEL);
>> +	arr->subtree = kzalloc(sizeof(arr->subtree[0]) * n, GFP_KERNEL);

> n comes from the hardware no?

Yes. Length of hardware description array.

> Maybe make these kcalloc too.

I'm not a fan of kcalloc. I think it removes readability. I remeber
kernel patches to swap the arguments, because people get them wrong.
Even Kernighan said it was a design error (in "the practice of
programming").  That said, I'm not the leader here.

thanks
/alessandro

Re: [patch -next] FMC: NULL dereference on allocation failure

[Joe Perches]

On Wed, 2013-06-19 at 19:57 +0200, Alessandro Rubini wrote:
> >> +	arr->record = kzalloc(sizeof(arr->record[0]) * n, GFP_KERNEL);
> >> +	arr->subtree = kzalloc(sizeof(arr->subtree[0]) * n, GFP_KERNEL);
> 
> > n comes from the hardware no?
> 
> Yes. Length of hardware description array.
> 
> > Maybe make these kcalloc too.
> 
> I'm not a fan of kcalloc. I think it removes readability. I remeber
> kernel patches to swap the arguments, because people get them wrong.

kcalloc's sole benefit is multiplication overflow protection.
If sizes are small and known, kcalloc isn't particularly useful.

Those kcalloc arg swap patches were just for style with no net effect.

> Even Kernighan said it was a design error (in "the practice of
> programming").  That said, I'm not the leader here.

I think the real design pattern error was realloc()

Anyway, no worries...

Re: [patch -next] FMC: NULL dereference on allocation failure

[Dan Carpenter]

On Wed, Jun 19, 2013 at 07:57:58PM +0200, Alessandro Rubini wrote:
> I'm not a fan of kcalloc. I think it removes readability.

Ok.  We'll go with my original patch then.

regards,
dan carpenter

Re: [patch] FMC: NULL dereference on allocation failure

[Alessandro Rubini]

> Apparently these are going through Greg K-H.  I'll resend, with Greg
> CC'd so he can pick it up from the mailing list.
> 
> Could you add an entry to the MAINTAINERS file so that Greg will be
> CC'd automatically using get_maintainer.pl?

Ok.  Added to my todo list.

> Is there a dedicated list for FMC development?  That would go in the
> MAINTAINERS file as well.

No, there is no list. Or "not yet".  The project was born externally
(on ohwr.org), mainly within the "white rabbit" research group.  If it
makes sense, we can have a public list, I'm all for it.

thanks
/alessandro