Last week a bug was detected in n_tty.c where an array of char was replaced by a
char pointer making a "(len > sizeof(buf))" condition test for len > 4 (or 8)
bytes, instead of the original array size.
I decided to tweak sparse to give warnings on sizeof(pointer), so that I could
check for other cases like this. The tweak was a very crude hack that I'm not
proud of, and I am still trying to make it more reliable.
So far I found 2 interesting cases (in 2.6.11-rc5). I'm not sure they are bugs,
but they sure look suspicious.
1: drivers/usb/storage/usb.c:788
/*
* Since this is a new device, we need to register a SCSI
* host definition with the higher SCSI layers.
*/
us->host = scsi_host_alloc(&usb_stor_host_template, sizeof(us));
if (!us->host) {
printk(KERN_WARNING USB_STORAGE
"Unable to allocate the scsi host\n");
return -EBUSY;
}
"us" is a "struct us_data *". It seems pretty weird that we're allocating
something the size of a pointer, and then waste a pointer to keep the address
where it is allocated. So maybe this should be:
us->host = scsi_host_alloc(&usb_stor_host_template, sizeof(*us));
2: sound/core/control.c:936
ue = kcalloc(1, sizeof(struct user_element) + private_size + extra_size,
GFP_KERNEL);
if (ue == NULL)
return -ENOMEM;
ue->info = info;
ue->elem_data = (char *)ue + sizeof(ue);
ue->elem_data_size = private_size;
if (extra_size) {
ue->priv_data = (char *)ue + sizeof(ue) + private_size;
ue->priv_data_size = extra_size;
if (ue->info.type == SNDRV_CTL_ELEM_TYPE_ENUMERATED) {
if (copy_from_user(ue->priv_data, *(char __user
**)info.value.enumerated.name, extra_size))
return -EFAULT;
}
}
If we're allocating "sizeof(struct user_element) + private_size + extra_size" it
seems that in the instructions below we would be wanting to use that space, so
both "sizeof(ue)" there should in fact be "sizeof(*ue)"
I'm not *really* sure that these are bugs, but they look suspicious. I'm CC'ing
the maintainers of both these files so that they can check these out.
I'll probably bring more examples in the future, as the tool improves. Right now
it gives about 550 false positives, so I think it is better to improve it
before checking every case :)
--
Paulo Marques - http://www.grupopie.com
All that is necessary for the triumph of evil is that good men do nothing.
Edmund Burke (1729 - 1797)
On Sun, Feb 27, 2005 at 08:25:04PM +0000, [email protected] wrote:
> I decided to tweak sparse to give warnings on sizeof(pointer), so that I could
> check for other cases like this. The tweak was a very crude hack that I'm not
> proud of, and I am still trying to make it more reliable.
>
> So far I found 2 interesting cases (in 2.6.11-rc5). I'm not sure they are bugs,
> but they sure look suspicious.
>
> 1: drivers/usb/storage/usb.c:788
>
> /*
> * Since this is a new device, we need to register a SCSI
> * host definition with the higher SCSI layers.
> */
> us->host = scsi_host_alloc(&usb_stor_host_template, sizeof(us));
> if (!us->host) {
> printk(KERN_WARNING USB_STORAGE
> "Unable to allocate the scsi host\n");
> return -EBUSY;
> }
>
> "us" is a "struct us_data *". It seems pretty weird that we're allocating
> something the size of a pointer, and then waste a pointer to keep the address
> where it is allocated. So maybe this should be:
>
> us->host = scsi_host_alloc(&usb_stor_host_template, sizeof(*us));
This is actually correct as-is. We're allocating a host, and asking for
the sizeof(pointer) in the 'extra storage' section. We then store the
pointer (not what it points to) in the extra storage section a few lines down.
Matt
--
Matthew Dharm Home: [email protected]
Maintainer, Linux USB Mass Storage Driver
S: Another stupid question?
G: There's no such thing as a stupid question, only stupid people.
-- Stef and Greg
User Friendly, 7/15/1998
Quoting Matthew Dharm <[email protected]>:
> [...]
> us->host = scsi_host_alloc(&usb_stor_host_template, sizeof(*us));
>
> This is actually correct as-is. We're allocating a host, and asking for
> the sizeof(pointer) in the 'extra storage' section. We then store the
> pointer (not what it points to) in the extra storage section a few lines
> down.
Thanks for clarifying that. I guess the weekend effect got me, because at a
certain point I was starting to read the scsi_host_alloc as if it were a
kmalloc or something... :P
Anyway, after improving the tool and checking for false positives, there is only
one more suspicious piece of code in drivers/acpi/video.c:561
status = acpi_video_device_lcd_query_levels(device, &obj);
if (obj && obj->type == ACPI_TYPE_PACKAGE && obj->package.count >= 2) {
int count = 0;
union acpi_object *o;
br = kmalloc(sizeof &br, GFP_KERNEL);
if (!br) {
printk(KERN_ERR "can't allocate memory\n");
} else {
memset(br, 0, sizeof &br);
br->levels = kmalloc(obj->package.count * sizeof &br->levels, GFP_KERNEL);
if (!br->levels)
goto out;
"br" if of type "struct acpi_video_device_brightness *".
"sizeof &br" doesn't make much sense there (besides the unconventional use of
sizeof without parenthesis) because the rest of the code seem to suggest that a
whole structure should have been allocated. This is the last case I've seen,
and I've added the maintainer to the cc list, so that he can check the code for
correctness.
Well, sorry about bothering you with a false positive (that I should've
recognised on my own, anyway), and thanks for the help.
--
Paulo Marques - http://www.grupopie.com
All that is necessary for the triumph of evil is that good men do nothing.
Edmund Burke (1729 - 1797)
"" <[email protected]> wrote:
>
> Anyway, after improving the tool and checking for false positives, there is only
> one more suspicious piece of code in drivers/acpi/video.c:561
>
> status = acpi_video_device_lcd_query_levels(device, &obj);
>
> if (obj && obj->type == ACPI_TYPE_PACKAGE && obj->package.count >= 2) {
> int count = 0;
> union acpi_object *o;
>
> br = kmalloc(sizeof &br, GFP_KERNEL);
yup, bug.
> if (!br) {
> printk(KERN_ERR "can't allocate memory\n");
> } else {
> memset(br, 0, sizeof &br);
> br->levels = kmalloc(obj->package.count * sizeof &br->levels, GFP_KERNEL);
And another one, although it happens to work out OK.
I'll get these all fixed up, thanks.
--- ./drivers/acpi/video.c.orig 2005-03-08 13:07:42.000000000 +0000
+++ ./drivers/acpi/video.c 2005-03-08 13:09:05.000000000 +0000
@@ -564,11 +564,11 @@ acpi_video_device_find_cap (struct acpi_
int count = 0;
union acpi_object *o;
- br = kmalloc(sizeof &br, GFP_KERNEL);
+ br = kmalloc(sizeof(*br), GFP_KERNEL);
if (!br) {
printk(KERN_ERR "can't allocate memory\n");
} else {
- memset(br, 0, sizeof *br);
+ memset(br, 0, sizeof(*br));
br->levels = kmalloc(obj->package.count *
sizeof *(br->levels), GFP_KERNEL);
if (!br->levels)