* [patch] wireless: at76c50x: allocating too much data @ 2012-04-20 6:47 ` Dan Carpenter 0 siblings, 0 replies; 26+ messages in thread From: Dan Carpenter @ 2012-04-20 6:47 UTC (permalink / raw) To: John W. Linville; +Cc: linux-wireless, kernel-janitors This is a cut and paste mistake, sizeof(struct mib_local) was intended instead of sizeof(struct mib_phy). The call to at76_get_mib() uses sizeof(struct mib_local) correctly. The current code works fine because mib_phy structs are larger than mib_local structs. But we may as well clean it up. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c index faa8bcb..0bba5ea 100644 --- a/drivers/net/wireless/at76c50x-usb.c +++ b/drivers/net/wireless/at76c50x-usb.c @@ -1122,7 +1122,7 @@ exit: static void at76_dump_mib_local(struct at76_priv *priv) { int ret; - struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL); + struct mib_local *m = kmalloc(sizeof(struct mib_local), GFP_KERNEL); if (!m) return; ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [patch] wireless: at76c50x: allocating too much data @ 2012-04-20 6:47 ` Dan Carpenter 0 siblings, 0 replies; 26+ messages in thread From: Dan Carpenter @ 2012-04-20 6:47 UTC (permalink / raw) To: John W. Linville; +Cc: linux-wireless, kernel-janitors This is a cut and paste mistake, sizeof(struct mib_local) was intended instead of sizeof(struct mib_phy). The call to at76_get_mib() uses sizeof(struct mib_local) correctly. The current code works fine because mib_phy structs are larger than mib_local structs. But we may as well clean it up. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c index faa8bcb..0bba5ea 100644 --- a/drivers/net/wireless/at76c50x-usb.c +++ b/drivers/net/wireless/at76c50x-usb.c @@ -1122,7 +1122,7 @@ exit: static void at76_dump_mib_local(struct at76_priv *priv) { int ret; - struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL); + struct mib_local *m = kmalloc(sizeof(struct mib_local), GFP_KERNEL); if (!m) return; ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data 2012-04-20 6:47 ` Dan Carpenter @ 2012-04-20 8:57 ` Julian Calaby -1 siblings, 0 replies; 26+ messages in thread From: Julian Calaby @ 2012-04-20 8:57 UTC (permalink / raw) To: Dan Carpenter; +Cc: John W. Linville, linux-wireless, kernel-janitors Hi Dan, On Fri, Apr 20, 2012 at 16:47, Dan Carpenter <dan.carpenter@oracle.com> wrote: > This is a cut and paste mistake, sizeof(struct mib_local) was intended > instead of sizeof(struct mib_phy). The call to at76_get_mib() uses > sizeof(struct mib_local) correctly. > > The current code works fine because mib_phy structs are larger than > mib_local structs. But we may as well clean it up. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c > index faa8bcb..0bba5ea 100644 > --- a/drivers/net/wireless/at76c50x-usb.c > +++ b/drivers/net/wireless/at76c50x-usb.c > @@ -1122,7 +1122,7 @@ exit: > static void at76_dump_mib_local(struct at76_priv *priv) > { > int ret; > - struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL); > + struct mib_local *m = kmalloc(sizeof(struct mib_local), GFP_KERNEL); Would it be better practice to use sizeof(*m)? Thanks, -- Julian Calaby Email: julian.calaby@gmail.com Profile: http://www.google.com/profiles/julian.calaby/ .Plan: http://sites.google.com/site/juliancalaby/ ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data @ 2012-04-20 8:57 ` Julian Calaby 0 siblings, 0 replies; 26+ messages in thread From: Julian Calaby @ 2012-04-20 8:57 UTC (permalink / raw) To: Dan Carpenter; +Cc: John W. Linville, linux-wireless, kernel-janitors Hi Dan, On Fri, Apr 20, 2012 at 16:47, Dan Carpenter <dan.carpenter@oracle.com> wrote: > This is a cut and paste mistake, sizeof(struct mib_local) was intended > instead of sizeof(struct mib_phy). The call to at76_get_mib() uses > sizeof(struct mib_local) correctly. > > The current code works fine because mib_phy structs are larger than > mib_local structs. But we may as well clean it up. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c > index faa8bcb..0bba5ea 100644 > --- a/drivers/net/wireless/at76c50x-usb.c > +++ b/drivers/net/wireless/at76c50x-usb.c > @@ -1122,7 +1122,7 @@ exit: > static void at76_dump_mib_local(struct at76_priv *priv) > { > int ret; > - struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL); > + struct mib_local *m = kmalloc(sizeof(struct mib_local), GFP_KERNEL); Would it be better practice to use sizeof(*m)? Thanks, -- Julian Calaby Email: julian.calaby@gmail.com Profile: http://www.google.com/profiles/julian.calaby/ .Plan: http://sites.google.com/site/juliancalaby/ -- 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data 2012-04-20 8:57 ` Julian Calaby @ 2012-04-20 9:14 ` Dan Carpenter -1 siblings, 0 replies; 26+ messages in thread From: Dan Carpenter @ 2012-04-20 9:14 UTC (permalink / raw) To: Julian Calaby; +Cc: John W. Linville, linux-wireless, kernel-janitors On Fri, Apr 20, 2012 at 06:57:00PM +1000, Julian Calaby wrote: > > - struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL); > > + struct mib_local *m = kmalloc(sizeof(struct mib_local), GFP_KERNEL); > > Would it be better practice to use sizeof(*m)? > That was my temptation as well... But I decided to make it match with the surrounding code. I'm happy to resend if people want. regards, dan carpenter ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data @ 2012-04-20 9:14 ` Dan Carpenter 0 siblings, 0 replies; 26+ messages in thread From: Dan Carpenter @ 2012-04-20 9:14 UTC (permalink / raw) To: Julian Calaby; +Cc: John W. Linville, linux-wireless, kernel-janitors On Fri, Apr 20, 2012 at 06:57:00PM +1000, Julian Calaby wrote: > > - struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL); > > + struct mib_local *m = kmalloc(sizeof(struct mib_local), GFP_KERNEL); > > Would it be better practice to use sizeof(*m)? > That was my temptation as well... But I decided to make it match with the surrounding code. I'm happy to resend if people want. regards, dan carpenter -- 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data 2012-04-20 9:14 ` Dan Carpenter @ 2012-04-20 18:14 ` Kalle Valo -1 siblings, 0 replies; 26+ messages in thread From: Kalle Valo @ 2012-04-20 18:14 UTC (permalink / raw) To: Dan Carpenter Cc: Julian Calaby, John W. Linville, linux-wireless, kernel-janitors Dan Carpenter <dan.carpenter@oracle.com> writes: > On Fri, Apr 20, 2012 at 06:57:00PM +1000, Julian Calaby wrote: >> > - struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL); >> > + struct mib_local *m = kmalloc(sizeof(struct mib_local), GFP_KERNEL); >> >> Would it be better practice to use sizeof(*m)? >> > > That was my temptation as well... But I decided to make it match > with the surrounding code. I'm happy to resend if people want. IMHO sizeof(*m) is better and I tend to use it. Related to this: I have a bad habit of sometimes dropping '*' from sizeof()? Is there a tool which could spot that? -- Kalle Valo ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data @ 2012-04-20 18:14 ` Kalle Valo 0 siblings, 0 replies; 26+ messages in thread From: Kalle Valo @ 2012-04-20 18:14 UTC (permalink / raw) To: Dan Carpenter Cc: Julian Calaby, John W. Linville, linux-wireless, kernel-janitors Dan Carpenter <dan.carpenter@oracle.com> writes: > On Fri, Apr 20, 2012 at 06:57:00PM +1000, Julian Calaby wrote: >> > - struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL); >> > + struct mib_local *m = kmalloc(sizeof(struct mib_local), GFP_KERNEL); >> >> Would it be better practice to use sizeof(*m)? >> > > That was my temptation as well... But I decided to make it match > with the surrounding code. I'm happy to resend if people want. IMHO sizeof(*m) is better and I tend to use it. Related to this: I have a bad habit of sometimes dropping '*' from sizeof()? Is there a tool which could spot that? -- Kalle Valo ^ permalink raw reply [flat|nested] 26+ messages in thread
* [patch v2] wireless: at76c50x: allocating too much data 2012-04-20 18:14 ` Kalle Valo @ 2012-04-21 12:23 ` Dan Carpenter -1 siblings, 0 replies; 26+ messages in thread From: Dan Carpenter @ 2012-04-21 12:23 UTC (permalink / raw) To: John W. Linville, Julian Calaby, Kalle Valo Cc: linux-wireless, kernel-janitors This is a cut and paste mistake, sizeof(struct mib_local) was intended instead of sizeof(struct mib_phy). The call to at76_get_mib() uses sizeof(struct mib_local) correctly, although I changed that to sizeof(*m) for style reasons after discussion with some of the wireless maintainers. The current code works fine because mib_phy structs are larger than mib_local structs. But we may as well clean it up. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- v2: use sizeof(*m) instead of sizeof(struct mib_local). diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c index faa8bcb..3036c0b 100644 --- a/drivers/net/wireless/at76c50x-usb.c +++ b/drivers/net/wireless/at76c50x-usb.c @@ -1122,12 +1122,12 @@ exit: static void at76_dump_mib_local(struct at76_priv *priv) { int ret; - struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL); + struct mib_local *m = kmalloc(sizeof(*m), GFP_KERNEL); if (!m) return; - ret = at76_get_mib(priv->udev, MIB_LOCAL, m, sizeof(struct mib_local)); + ret = at76_get_mib(priv->udev, MIB_LOCAL, m, sizeof(*m)); if (ret < 0) { wiphy_err(priv->hw->wiphy, "at76_get_mib (LOCAL) failed: %d\n", ret); ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [patch v2] wireless: at76c50x: allocating too much data @ 2012-04-21 12:23 ` Dan Carpenter 0 siblings, 0 replies; 26+ messages in thread From: Dan Carpenter @ 2012-04-21 12:23 UTC (permalink / raw) To: John W. Linville, Julian Calaby, Kalle Valo Cc: linux-wireless, kernel-janitors This is a cut and paste mistake, sizeof(struct mib_local) was intended instead of sizeof(struct mib_phy). The call to at76_get_mib() uses sizeof(struct mib_local) correctly, although I changed that to sizeof(*m) for style reasons after discussion with some of the wireless maintainers. The current code works fine because mib_phy structs are larger than mib_local structs. But we may as well clean it up. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- v2: use sizeof(*m) instead of sizeof(struct mib_local). diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c index faa8bcb..3036c0b 100644 --- a/drivers/net/wireless/at76c50x-usb.c +++ b/drivers/net/wireless/at76c50x-usb.c @@ -1122,12 +1122,12 @@ exit: static void at76_dump_mib_local(struct at76_priv *priv) { int ret; - struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL); + struct mib_local *m = kmalloc(sizeof(*m), GFP_KERNEL); if (!m) return; - ret = at76_get_mib(priv->udev, MIB_LOCAL, m, sizeof(struct mib_local)); + ret = at76_get_mib(priv->udev, MIB_LOCAL, m, sizeof(*m)); if (ret < 0) { wiphy_err(priv->hw->wiphy, "at76_get_mib (LOCAL) failed: %d\n", ret); ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data 2012-04-20 18:14 ` Kalle Valo @ 2012-04-21 12:45 ` Dan Carpenter -1 siblings, 0 replies; 26+ messages in thread From: Dan Carpenter @ 2012-04-21 12:45 UTC (permalink / raw) To: Kalle Valo Cc: Julian Calaby, John W. Linville, linux-wireless, kernel-janitors On Fri, Apr 20, 2012 at 09:14:44PM +0300, Kalle Valo wrote: > Dan Carpenter <dan.carpenter@oracle.com> writes: > > > On Fri, Apr 20, 2012 at 06:57:00PM +1000, Julian Calaby wrote: > >> > - struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL); > >> > + struct mib_local *m = kmalloc(sizeof(struct mib_local), GFP_KERNEL); > >> > >> Would it be better practice to use sizeof(*m)? > >> > > > > That was my temptation as well... But I decided to make it match > > with the surrounding code. I'm happy to resend if people want. > > IMHO sizeof(*m) is better and I tend to use it. > > Related to this: I have a bad habit of sometimes dropping '*' from > sizeof()? Is there a tool which could spot that? > That's what I was working on for Smatch when I sent this patch. The odd thing is that I can't find any bugs like this in the kernel. If sizeof(foo) is less than sizeof(*foo), which is probably the normal case, then these get caught early on in testing. Still I think people must have done manual audits as well... It feels too clean to be natural. regards, dan carpenter ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data @ 2012-04-21 12:45 ` Dan Carpenter 0 siblings, 0 replies; 26+ messages in thread From: Dan Carpenter @ 2012-04-21 12:45 UTC (permalink / raw) To: Kalle Valo Cc: Julian Calaby, John W. Linville, linux-wireless, kernel-janitors On Fri, Apr 20, 2012 at 09:14:44PM +0300, Kalle Valo wrote: > Dan Carpenter <dan.carpenter@oracle.com> writes: > > > On Fri, Apr 20, 2012 at 06:57:00PM +1000, Julian Calaby wrote: > >> > - struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL); > >> > + struct mib_local *m = kmalloc(sizeof(struct mib_local), GFP_KERNEL); > >> > >> Would it be better practice to use sizeof(*m)? > >> > > > > That was my temptation as well... But I decided to make it match > > with the surrounding code. I'm happy to resend if people want. > > IMHO sizeof(*m) is better and I tend to use it. > > Related to this: I have a bad habit of sometimes dropping '*' from > sizeof()? Is there a tool which could spot that? > That's what I was working on for Smatch when I sent this patch. The odd thing is that I can't find any bugs like this in the kernel. If sizeof(foo) is less than sizeof(*foo), which is probably the normal case, then these get caught early on in testing. Still I think people must have done manual audits as well... It feels too clean to be natural. regards, dan carpenter -- 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data 2012-04-21 12:45 ` Dan Carpenter @ 2012-04-21 13:19 ` Julia Lawall -1 siblings, 0 replies; 26+ messages in thread From: Julia Lawall @ 2012-04-21 13:19 UTC (permalink / raw) To: Dan Carpenter Cc: Kalle Valo, Julian Calaby, John W. Linville, linux-wireless, kernel-janitors [-- Attachment #1: Type: TEXT/PLAIN, Size: 1236 bytes --] On Sat, 21 Apr 2012, Dan Carpenter wrote: > On Fri, Apr 20, 2012 at 09:14:44PM +0300, Kalle Valo wrote: >> Dan Carpenter <dan.carpenter@oracle.com> writes: >> >>> On Fri, Apr 20, 2012 at 06:57:00PM +1000, Julian Calaby wrote: >>>>> - struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL); >>>>> + struct mib_local *m = kmalloc(sizeof(struct mib_local), GFP_KERNEL); >>>> >>>> Would it be better practice to use sizeof(*m)? >>>> >>> >>> That was my temptation as well... But I decided to make it match >>> with the surrounding code. I'm happy to resend if people want. >> >> IMHO sizeof(*m) is better and I tend to use it. >> >> Related to this: I have a bad habit of sometimes dropping '*' from >> sizeof()? Is there a tool which could spot that? >> > > That's what I was working on for Smatch when I sent this patch. > > The odd thing is that I can't find any bugs like this in the kernel. > If sizeof(foo) is less than sizeof(*foo), which is probably the > normal case, then these get caught early on in testing. > > Still I think people must have done manual audits as well... It > feels too clean to be natural. I sent some patches with respect to this. But that was probably around a year ago. julia ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data @ 2012-04-21 13:19 ` Julia Lawall 0 siblings, 0 replies; 26+ messages in thread From: Julia Lawall @ 2012-04-21 13:19 UTC (permalink / raw) To: Dan Carpenter Cc: Kalle Valo, Julian Calaby, John W. Linville, linux-wireless, kernel-janitors [-- Attachment #1: Type: TEXT/PLAIN, Size: 1269 bytes --] On Sat, 21 Apr 2012, Dan Carpenter wrote: > On Fri, Apr 20, 2012 at 09:14:44PM +0300, Kalle Valo wrote: >> Dan Carpenter <dan.carpenter@oracle.com> writes: >> >>> On Fri, Apr 20, 2012 at 06:57:00PM +1000, Julian Calaby wrote: >>>>> - struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL); >>>>> + struct mib_local *m = kmalloc(sizeof(struct mib_local), GFP_KERNEL); >>>> >>>> Would it be better practice to use sizeof(*m)? >>>> >>> >>> That was my temptation as well... But I decided to make it match >>> with the surrounding code. I'm happy to resend if people want. >> >> IMHO sizeof(*m) is better and I tend to use it. >> >> Related to this: I have a bad habit of sometimes dropping '*' from >> sizeof()? Is there a tool which could spot that? >> > > That's what I was working on for Smatch when I sent this patch. > > The odd thing is that I can't find any bugs like this in the kernel. > If sizeof(foo) is less than sizeof(*foo), which is probably the > normal case, then these get caught early on in testing. > > Still I think people must have done manual audits as well... It > feels too clean to be natural. I sent some patches with respect to this. But that was probably around a year ago. julia ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data 2012-04-21 12:45 ` Dan Carpenter @ 2012-04-21 13:51 ` Julia Lawall -1 siblings, 0 replies; 26+ messages in thread From: Julia Lawall @ 2012-04-21 13:51 UTC (permalink / raw) To: Dan Carpenter Cc: Kalle Valo, Julian Calaby, John W. Linville, linux-wireless, kernel-janitors [-- Attachment #1: Type: TEXT/PLAIN, Size: 1483 bytes --] On Sat, 21 Apr 2012, Dan Carpenter wrote: > On Fri, Apr 20, 2012 at 09:14:44PM +0300, Kalle Valo wrote: >> Dan Carpenter <dan.carpenter@oracle.com> writes: >> >>> On Fri, Apr 20, 2012 at 06:57:00PM +1000, Julian Calaby wrote: >>>>> - struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL); >>>>> + struct mib_local *m = kmalloc(sizeof(struct mib_local), GFP_KERNEL); >>>> >>>> Would it be better practice to use sizeof(*m)? >>>> >>> >>> That was my temptation as well... But I decided to make it match >>> with the surrounding code. I'm happy to resend if people want. >> >> IMHO sizeof(*m) is better and I tend to use it. >> >> Related to this: I have a bad habit of sometimes dropping '*' from >> sizeof()? Is there a tool which could spot that? >> > > That's what I was working on for Smatch when I sent this patch. > > The odd thing is that I can't find any bugs like this in the kernel. > If sizeof(foo) is less than sizeof(*foo), which is probably the > normal case, then these get caught early on in testing. > > Still I think people must have done manual audits as well... It > feels too clean to be natural. Looking for x = ... sizeof(x) ... I get 9 reports. In most cases it looks like sizeof(x) is coincidentally the same as the size that is wanted. Two cases that look like they could have some noticible effect are: arch/xtensa/platforms/iss/network.c, line 789 drivers/block/cciss.c, line 4211 I will send patches for those two. julia ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data @ 2012-04-21 13:51 ` Julia Lawall 0 siblings, 0 replies; 26+ messages in thread From: Julia Lawall @ 2012-04-21 13:51 UTC (permalink / raw) To: Dan Carpenter Cc: Kalle Valo, Julian Calaby, John W. Linville, linux-wireless, kernel-janitors [-- Attachment #1: Type: TEXT/PLAIN, Size: 1522 bytes --] On Sat, 21 Apr 2012, Dan Carpenter wrote: > On Fri, Apr 20, 2012 at 09:14:44PM +0300, Kalle Valo wrote: >> Dan Carpenter <dan.carpenter@oracle.com> writes: >> >>> On Fri, Apr 20, 2012 at 06:57:00PM +1000, Julian Calaby wrote: >>>>> - struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL); >>>>> + struct mib_local *m = kmalloc(sizeof(struct mib_local), GFP_KERNEL); >>>> >>>> Would it be better practice to use sizeof(*m)? >>>> >>> >>> That was my temptation as well... But I decided to make it match >>> with the surrounding code. I'm happy to resend if people want. >> >> IMHO sizeof(*m) is better and I tend to use it. >> >> Related to this: I have a bad habit of sometimes dropping '*' from >> sizeof()? Is there a tool which could spot that? >> > > That's what I was working on for Smatch when I sent this patch. > > The odd thing is that I can't find any bugs like this in the kernel. > If sizeof(foo) is less than sizeof(*foo), which is probably the > normal case, then these get caught early on in testing. > > Still I think people must have done manual audits as well... It > feels too clean to be natural. Looking for x = ... sizeof(x) ... I get 9 reports. In most cases it looks like sizeof(x) is coincidentally the same as the size that is wanted. Two cases that look like they could have some noticible effect are: arch/xtensa/platforms/iss/network.c, line 789 drivers/block/cciss.c, line 4211 I will send patches for those two. julia ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data 2012-04-21 13:51 ` Julia Lawall @ 2012-04-21 14:51 ` Dan Carpenter -1 siblings, 0 replies; 26+ messages in thread From: Dan Carpenter @ 2012-04-21 14:51 UTC (permalink / raw) To: Julia Lawall Cc: Kalle Valo, Julian Calaby, John W. Linville, linux-wireless, kernel-janitors On Sat, Apr 21, 2012 at 03:51:44PM +0200, Julia Lawall wrote: > Looking for x = ... sizeof(x) ... I get 9 reports. In most cases it > looks like sizeof(x) is coincidentally the same as the size that is > wanted. Two cases that look like they could have some noticible > effect are: > > arch/xtensa/platforms/iss/network.c, line 789 > drivers/block/cciss.c, line 4211 > Clever. You'd need to restrict it to places where x was a pointer. That's better than my check which was specific to kmalloc(). (So uh... I'm going to rewrite mine as well to be more generic. :P) regards, dan carpenter ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data @ 2012-04-21 14:51 ` Dan Carpenter 0 siblings, 0 replies; 26+ messages in thread From: Dan Carpenter @ 2012-04-21 14:51 UTC (permalink / raw) To: Julia Lawall Cc: Kalle Valo, Julian Calaby, John W. Linville, linux-wireless, kernel-janitors On Sat, Apr 21, 2012 at 03:51:44PM +0200, Julia Lawall wrote: > Looking for x = ... sizeof(x) ... I get 9 reports. In most cases it > looks like sizeof(x) is coincidentally the same as the size that is > wanted. Two cases that look like they could have some noticible > effect are: > > arch/xtensa/platforms/iss/network.c, line 789 > drivers/block/cciss.c, line 4211 > Clever. You'd need to restrict it to places where x was a pointer. That's better than my check which was specific to kmalloc(). (So uh... I'm going to rewrite mine as well to be more generic. :P) regards, dan carpenter ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data 2012-04-21 14:51 ` Dan Carpenter @ 2012-04-21 14:51 ` Julia Lawall -1 siblings, 0 replies; 26+ messages in thread From: Julia Lawall @ 2012-04-21 14:51 UTC (permalink / raw) To: Dan Carpenter Cc: Julia Lawall, Kalle Valo, Julian Calaby, John W. Linville, linux-wireless, kernel-janitors On Sat, 21 Apr 2012, Dan Carpenter wrote: > On Sat, Apr 21, 2012 at 03:51:44PM +0200, Julia Lawall wrote: >> Looking for x = ... sizeof(x) ... I get 9 reports. In most cases it >> looks like sizeof(x) is coincidentally the same as the size that is >> wanted. Two cases that look like they could have some noticible >> effect are: >> >> arch/xtensa/platforms/iss/network.c, line 789 >> drivers/block/cciss.c, line 4211 >> > > Clever. You'd need to restrict it to places where x was a pointer. Yes, I didn't put the whole thing: @r expression@ expression *x; position p; @@ x@p = <+... sizeof(x) ...+> julia > That's better than my check which was specific to kmalloc(). (So > uh... I'm going to rewrite mine as well to be more generic. :P) > > regards, > dan carpenter > > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data @ 2012-04-21 14:51 ` Julia Lawall 0 siblings, 0 replies; 26+ messages in thread From: Julia Lawall @ 2012-04-21 14:51 UTC (permalink / raw) To: Dan Carpenter Cc: Julia Lawall, Kalle Valo, Julian Calaby, John W. Linville, linux-wireless, kernel-janitors On Sat, 21 Apr 2012, Dan Carpenter wrote: > On Sat, Apr 21, 2012 at 03:51:44PM +0200, Julia Lawall wrote: >> Looking for x = ... sizeof(x) ... I get 9 reports. In most cases it >> looks like sizeof(x) is coincidentally the same as the size that is >> wanted. Two cases that look like they could have some noticible >> effect are: >> >> arch/xtensa/platforms/iss/network.c, line 789 >> drivers/block/cciss.c, line 4211 >> > > Clever. You'd need to restrict it to places where x was a pointer. Yes, I didn't put the whole thing: @r expression@ expression *x; position p; @@ x@p = <+... sizeof(x) ...+> julia > That's better than my check which was specific to kmalloc(). (So > uh... I'm going to rewrite mine as well to be more generic. :P) > > regards, > dan carpenter > > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data 2012-04-21 14:51 ` Dan Carpenter @ 2012-04-21 14:59 ` Dan Carpenter -1 siblings, 0 replies; 26+ messages in thread From: Dan Carpenter @ 2012-04-21 14:59 UTC (permalink / raw) To: Julia Lawall Cc: Kalle Valo, Julian Calaby, John W. Linville, linux-wireless, kernel-janitors On Sat, Apr 21, 2012 at 05:51:41PM +0300, Dan Carpenter wrote: > On Sat, Apr 21, 2012 at 03:51:44PM +0200, Julia Lawall wrote: > > Looking for x = ... sizeof(x) ... I get 9 reports. In most cases it > > looks like sizeof(x) is coincidentally the same as the size that is > > wanted. Two cases that look like they could have some noticible > > effect are: > > > > arch/xtensa/platforms/iss/network.c, line 789 > > drivers/block/cciss.c, line 4211 > > > > Clever. You'd need to restrict it to places where x was a pointer. > That's better than my check which was specific to kmalloc(). (So > uh... I'm going to rewrite mine as well to be more generic. :P) > The other thing would be to look for places that do: func(x, sizeof(x); Of course, you've found a lot of memset()/memcpy() bugs like that in the past, but probably it could be made more generic so it checks every function. regards, dan carpenter ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data @ 2012-04-21 14:59 ` Dan Carpenter 0 siblings, 0 replies; 26+ messages in thread From: Dan Carpenter @ 2012-04-21 14:59 UTC (permalink / raw) To: Julia Lawall Cc: Kalle Valo, Julian Calaby, John W. Linville, linux-wireless, kernel-janitors On Sat, Apr 21, 2012 at 05:51:41PM +0300, Dan Carpenter wrote: > On Sat, Apr 21, 2012 at 03:51:44PM +0200, Julia Lawall wrote: > > Looking for x = ... sizeof(x) ... I get 9 reports. In most cases it > > looks like sizeof(x) is coincidentally the same as the size that is > > wanted. Two cases that look like they could have some noticible > > effect are: > > > > arch/xtensa/platforms/iss/network.c, line 789 > > drivers/block/cciss.c, line 4211 > > > > Clever. You'd need to restrict it to places where x was a pointer. > That's better than my check which was specific to kmalloc(). (So > uh... I'm going to rewrite mine as well to be more generic. :P) > The other thing would be to look for places that do: func(x, sizeof(x); Of course, you've found a lot of memset()/memcpy() bugs like that in the past, but probably it could be made more generic so it checks every function. regards, dan carpenter ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data 2012-04-21 14:51 ` Dan Carpenter @ 2012-04-21 15:12 ` Dan Carpenter -1 siblings, 0 replies; 26+ messages in thread From: Dan Carpenter @ 2012-04-21 15:12 UTC (permalink / raw) To: Julia Lawall Cc: Kalle Valo, Julian Calaby, John W. Linville, linux-wireless, kernel-janitors On Sat, Apr 21, 2012 at 05:51:41PM +0300, Dan Carpenter wrote: > On Sat, Apr 21, 2012 at 03:51:44PM +0200, Julia Lawall wrote: > > Looking for x = ... sizeof(x) ... I get 9 reports. In most cases it > > looks like sizeof(x) is coincidentally the same as the size that is > > wanted. Two cases that look like they could have some noticible > > effect are: > > > > arch/xtensa/platforms/iss/network.c, line 789 > > drivers/block/cciss.c, line 4211 > > > > Clever. You'd need to restrict it to places where x was a pointer. > That's better than my check which was specific to kmalloc(). (So > uh... I'm going to rewrite mine as well to be more generic. :P) > Hm... Smatch is not really the right tool here. By the time Sparse gives you the sizeof(foo) information, it just looks like a number 8. I hacked up Sparse a bit so it works for simple expressions which are one token in from the c tokenizer. So: foo = kmalloc(sizeof(foo), GFP_KERNEL); => error. foo->bar = kmalloc(sizeof(foo->bar), GFP_KERNEL); => tricky. It's not ideal. Coccinelle is better for this. regards, dan carpenter ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data @ 2012-04-21 15:12 ` Dan Carpenter 0 siblings, 0 replies; 26+ messages in thread From: Dan Carpenter @ 2012-04-21 15:12 UTC (permalink / raw) To: Julia Lawall Cc: Kalle Valo, Julian Calaby, John W. Linville, linux-wireless, kernel-janitors On Sat, Apr 21, 2012 at 05:51:41PM +0300, Dan Carpenter wrote: > On Sat, Apr 21, 2012 at 03:51:44PM +0200, Julia Lawall wrote: > > Looking for x = ... sizeof(x) ... I get 9 reports. In most cases it > > looks like sizeof(x) is coincidentally the same as the size that is > > wanted. Two cases that look like they could have some noticible > > effect are: > > > > arch/xtensa/platforms/iss/network.c, line 789 > > drivers/block/cciss.c, line 4211 > > > > Clever. You'd need to restrict it to places where x was a pointer. > That's better than my check which was specific to kmalloc(). (So > uh... I'm going to rewrite mine as well to be more generic. :P) > Hm... Smatch is not really the right tool here. By the time Sparse gives you the sizeof(foo) information, it just looks like a number 8. I hacked up Sparse a bit so it works for simple expressions which are one token in from the c tokenizer. So: foo = kmalloc(sizeof(foo), GFP_KERNEL); => error. foo->bar = kmalloc(sizeof(foo->bar), GFP_KERNEL); => tricky. It's not ideal. Coccinelle is better for this. regards, dan carpenter ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data 2012-04-21 15:12 ` Dan Carpenter @ 2012-04-21 15:13 ` Julia Lawall -1 siblings, 0 replies; 26+ messages in thread From: Julia Lawall @ 2012-04-21 15:13 UTC (permalink / raw) To: Dan Carpenter Cc: Julia Lawall, Kalle Valo, Julian Calaby, John W. Linville, linux-wireless, kernel-janitors On Sat, 21 Apr 2012, Dan Carpenter wrote: > On Sat, Apr 21, 2012 at 05:51:41PM +0300, Dan Carpenter wrote: >> On Sat, Apr 21, 2012 at 03:51:44PM +0200, Julia Lawall wrote: >>> Looking for x = ... sizeof(x) ... I get 9 reports. In most cases it >>> looks like sizeof(x) is coincidentally the same as the size that is >>> wanted. Two cases that look like they could have some noticible >>> effect are: >>> >>> arch/xtensa/platforms/iss/network.c, line 789 >>> drivers/block/cciss.c, line 4211 >>> >> >> Clever. You'd need to restrict it to places where x was a pointer. >> That's better than my check which was specific to kmalloc(). (So >> uh... I'm going to rewrite mine as well to be more generic. :P) >> > > Hm... Smatch is not really the right tool here. By the time Sparse > gives you the sizeof(foo) information, it just looks like a number > 8. > > I hacked up Sparse a bit so it works for simple expressions which > are one token in from the c tokenizer. So: > > foo = kmalloc(sizeof(foo), GFP_KERNEL); => error. > foo->bar = kmalloc(sizeof(foo->bar), GFP_KERNEL); => tricky. > > It's not ideal. Coccinelle is better for this. On the other hand, Coccinelle has no idea what the size is, so it doesn't know how important the problem is. julia ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] wireless: at76c50x: allocating too much data @ 2012-04-21 15:13 ` Julia Lawall 0 siblings, 0 replies; 26+ messages in thread From: Julia Lawall @ 2012-04-21 15:13 UTC (permalink / raw) To: Dan Carpenter Cc: Julia Lawall, Kalle Valo, Julian Calaby, John W. Linville, linux-wireless, kernel-janitors On Sat, 21 Apr 2012, Dan Carpenter wrote: > On Sat, Apr 21, 2012 at 05:51:41PM +0300, Dan Carpenter wrote: >> On Sat, Apr 21, 2012 at 03:51:44PM +0200, Julia Lawall wrote: >>> Looking for x = ... sizeof(x) ... I get 9 reports. In most cases it >>> looks like sizeof(x) is coincidentally the same as the size that is >>> wanted. Two cases that look like they could have some noticible >>> effect are: >>> >>> arch/xtensa/platforms/iss/network.c, line 789 >>> drivers/block/cciss.c, line 4211 >>> >> >> Clever. You'd need to restrict it to places where x was a pointer. >> That's better than my check which was specific to kmalloc(). (So >> uh... I'm going to rewrite mine as well to be more generic. :P) >> > > Hm... Smatch is not really the right tool here. By the time Sparse > gives you the sizeof(foo) information, it just looks like a number > 8. > > I hacked up Sparse a bit so it works for simple expressions which > are one token in from the c tokenizer. So: > > foo = kmalloc(sizeof(foo), GFP_KERNEL); => error. > foo->bar = kmalloc(sizeof(foo->bar), GFP_KERNEL); => tricky. > > It's not ideal. Coccinelle is better for this. On the other hand, Coccinelle has no idea what the size is, so it doesn't know how important the problem is. julia ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2012-04-21 15:13 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-04-20 6:47 [patch] wireless: at76c50x: allocating too much data Dan Carpenter 2012-04-20 6:47 ` Dan Carpenter 2012-04-20 8:57 ` Julian Calaby 2012-04-20 8:57 ` Julian Calaby 2012-04-20 9:14 ` Dan Carpenter 2012-04-20 9:14 ` Dan Carpenter 2012-04-20 18:14 ` Kalle Valo 2012-04-20 18:14 ` Kalle Valo 2012-04-21 12:23 ` [patch v2] " Dan Carpenter 2012-04-21 12:23 ` Dan Carpenter 2012-04-21 12:45 ` [patch] " Dan Carpenter 2012-04-21 12:45 ` Dan Carpenter 2012-04-21 13:19 ` Julia Lawall 2012-04-21 13:19 ` Julia Lawall 2012-04-21 13:51 ` Julia Lawall 2012-04-21 13:51 ` Julia Lawall 2012-04-21 14:51 ` Dan Carpenter 2012-04-21 14:51 ` Dan Carpenter 2012-04-21 14:51 ` Julia Lawall 2012-04-21 14:51 ` Julia Lawall 2012-04-21 14:59 ` Dan Carpenter 2012-04-21 14:59 ` Dan Carpenter 2012-04-21 15:12 ` Dan Carpenter 2012-04-21 15:12 ` Dan Carpenter 2012-04-21 15:13 ` Julia Lawall 2012-04-21 15:13 ` Julia Lawall
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.