FMC: NULL dereference on allocation failure

PATCH In review Dan Carpenter 3 participants 9 messages Jun 19, 2013 – Jun 20, 2013 Kernel janitors

lore

Share & tools

lore

Dan Carpenter

author

2013-06-19 14:49 UTC #0 lore raw

PATCH 1/1 FMC: NULL dereference on allocation failure

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

Signed-off-by Dan Carpenter <[email protected]>

drivers/fmc/fmc-sdb.c +6 -5

index 74fb326..79adc39 100644
@@ -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;

Alessandro Rubini

2013-06-19 15:25 UTC #1 lore raw

Quoted 2 lines

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

Dan Carpenter

author

2013-06-19 15:57 UTC #2 lore raw

Quoted 10 lines · Alessandro Rubini

Quoted 2 lines
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

Dan Carpenter

author

2013-06-19 16:01 UTC #3 lore raw

PATCH 1/1 FMC: NULL dereference on allocation failure

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

Signed-off-by Dan Carpenter <[email protected]>

Acked-by Alessandro Rubini <[email protected]>

drivers/fmc/fmc-sdb.c +6 -5

index 74fb326..79adc39 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Joe Perches

2013-06-19 16:15 UTC #4 lore raw

Quoted 2 lines · Dan Carpenter

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

[]

drivers/fmc/fmc-sdb.c

Quoted 1 line

drivers/fmc/fmc-sdb.c +0 -0

[]

drivers/fmc/fmc-sdb.c static struct sdb_array *__fmc_scan_sdb_tree(struct fmc_device *fmc,

Quoted 1 line

drivers/fmc/fmc-sdb.c +0 -0

@@ -46,16 +46,17 @@ static struct sdb_array *__fmc_scan_sdb_tree(struct fmc_device *fmc,

[]

drivers/fmc/fmc-sdb.c static struct sdb_array *__fmc_scan_sdb_tree(struct fmc_device *fmc, · line 51

Quoted 2 lines

drivers/fmc/fmc-sdb.c +0 -2

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

[]

drivers/fmc/fmc-sdb.c static struct sdb_array *__fmc_scan_sdb_tree(struct fmc_device *fmc, · line 52

Quoted 2 lines

drivers/fmc/fmc-sdb.c +2 -0

+	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.

Alessandro Rubini

2013-06-19 17:57 UTC #5 lore raw

Quoted 4 lines

drivers/fmc/fmc-sdb.c static struct sdb_array *__fmc_scan_sdb_tree(struct fmc_device *fmc, · line 52
Quoted 2 lines
drivers/fmc/fmc-sdb.c +2 -0
+	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.

Quoted 1 line

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

Joe Perches

2013-06-19 18:09 UTC #6 lore raw

Quoted 11 lines · Alessandro Rubini

Quoted 4 lines
drivers/fmc/fmc-sdb.c static struct sdb_array *__fmc_scan_sdb_tree(struct fmc_device *fmc, · line 52
Quoted 2 lines
drivers/fmc/fmc-sdb.c +2 -0
+	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.
Quoted 1 line
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.

Quoted 2 lines

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

Dan Carpenter

author

2013-06-20 08:06 UTC #7 lore raw

In reply to Alessandro Rubini

Quoted 1 line · Alessandro Rubini

I'm not a fan of kcalloc. I think it removes readability.

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

regards,
dan carpenter

Alessandro Rubini

2013-06-19 17:57 UTC #8 lore raw

In reply to Dan Carpenter

Quoted 5 lines

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.

Quoted 2 lines

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