[patch] wireless: at76c50x: allocating too much data

[patch] wireless: at76c50x: allocating too much data

[Dan Carpenter]

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;

[patch] wireless: at76c50x: allocating too much data

[Dan Carpenter]

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;

Re: [patch] wireless: at76c50x: allocating too much data

[Julian Calaby]

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/

Re: [patch] wireless: at76c50x: allocating too much data

[Julian Calaby]

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

Re: [patch] wireless: at76c50x: allocating too much data

[Dan Carpenter]

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

Re: [patch] wireless: at76c50x: allocating too much data

[Dan Carpenter]

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

Re: [patch] wireless: at76c50x: allocating too much data

[Kalle Valo]

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

Re: [patch] wireless: at76c50x: allocating too much data

[Kalle Valo]

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

[patch v2] wireless: at76c50x: allocating too much data

[Dan Carpenter]

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);

[patch v2] wireless: at76c50x: allocating too much data

[Dan Carpenter]

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);

Re: [patch] wireless: at76c50x: allocating too much data

[Dan Carpenter]

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

Re: [patch] wireless: at76c50x: allocating too much data

[Dan Carpenter]

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

Re: [patch] wireless: at76c50x: allocating too much data

[Julia Lawall]

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

Re: [patch] wireless: at76c50x: allocating too much data

[Julia Lawall]

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

Re: [patch] wireless: at76c50x: allocating too much data

[Julia Lawall]

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

Re: [patch] wireless: at76c50x: allocating too much data

[Julia Lawall]

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

Re: [patch] wireless: at76c50x: allocating too much data

[Dan Carpenter]

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

Re: [patch] wireless: at76c50x: allocating too much data

[Dan Carpenter]

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

Re: [patch] wireless: at76c50x: allocating too much data

[Julia Lawall]

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

Re: [patch] wireless: at76c50x: allocating too much data

[Julia Lawall]

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

Re: [patch] wireless: at76c50x: allocating too much data

[Dan Carpenter]

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

Re: [patch] wireless: at76c50x: allocating too much data

[Dan Carpenter]

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

Re: [patch] wireless: at76c50x: allocating too much data

[Dan Carpenter]

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

Re: [patch] wireless: at76c50x: allocating too much data

[Dan Carpenter]

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

Re: [patch] wireless: at76c50x: allocating too much data

[Julia Lawall]

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

Re: [patch] wireless: at76c50x: allocating too much data

[Julia Lawall]

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