toshiba_acpi check kmalloc return value
Subsystems (2)
PATCH 1/1 toshiba_acpi check kmalloc return value
drivers/acpi/toshiba_acpi.c renamed from drivers/acpi/toshiba_acpi.c.orig +4 -1
@@ -263,7 +263,10 @@ dispatch_write(struct file* file, const* destination so that sscanf can be used on it safely. */ tmp_buffer =kmalloc (count +1 ,GFP_KERNEL ); -if (copy_from_user (tmp_buffer, buffer, count)) { + +if (!tmp_buffer) { +return -ENOMEM ; + }else if (copy_from_user (tmp_buffer, buffer, count)) { result = -EFAULT ; }else {
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
On Sun, 10 Jul 2005 21:20:44 -0400 Brandon Niemczyk wrote:
| Signed-off-by: Brandon Niemczyk <[email protected]> | | --- linux/drivers/acpi/toshiba_acpi.c.orig 2005-07-10 20:40:47.000000000 -0400 | +++ linux/drivers/acpi/toshiba_acpi.c 2005-07-10 20:42:33.000000000 -0400 | @@ -263,7 +263,10 @@ dispatch_write(struct file* file, const | * destination so that sscanf can be used on it safely. | */ | tmp_buffer = kmalloc(count + 1, GFP_KERNEL); | - if (copy_from_user(tmp_buffer, buffer, count)) { | + | + if(!tmp_buffer) {
Looks correct functionally, but needs a space after "if", like "if (".
Oh, don't use braces for one-line "blocks".
Yes the copy_from_user() below does that too. :(
| + return -ENOMEM;
| + } else if (copy_from_user(tmp_buffer, buffer, count)) {
| result = -EFAULT;
| }
| else {---
~Randy
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
Here it is with the stylistic fixes:
drivers/acpi/toshiba_acpi.c renamed from drivers/acpi/toshiba_acpi.c.orig +4 -2
@@ -263,9 +263,11 @@ dispatch_write(struct file* file, const* destination so that sscanf can be used on it safely. */ tmp_buffer =kmalloc (count +1 ,GFP_KERNEL ); -if (copy_from_user (tmp_buffer, buffer, count)) { + +if (!tmp_buffer) +return -ENOMEM ; +else if (copy_from_user (tmp_buffer, buffer, count)) result = -EFAULT ; - }else { tmp_buffer[count] =0 ; result = item->write_func (tmp_buffer, count);
Quoted 6 lines · randy_dunlap
On Sun, 10 Jul 2005 21:20:44 -0400 Brandon Niemczyk wrote:
<snip>
Looks correct functionally, but needs a space after "if", like "if (".Oh, don't use braces for one-line "blocks".
Yes thecopy_from_user()below does that too. :(
Signature · 1 line
Brandon Niemczyk
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
hi Brandon,
keep it simple
tmp_buffer = kmalloc(count + 1, GFP_KERNEL);
if (!tmp_buffer)
return -ENOMEM;
if ( copy_from_user(tmp_buffer, buffer, count) )
result = -EFAULT;
tmp_buffer[count] = 0;
result = item->write_func(tmp_buffer, count);
just for the paranoid:
should tmp_buffer be filled with \0 to avoid an information leak ? (schroedinger bug ?)
re,
walter
Quoted 30 lines · Brandon Niemczyk
Here it is with the stylistic fixes:
Signed-off-by Brandon Niemczyk <[email protected]>drivers/acpi/toshiba_acpi.c renamed from drivers/acpi/toshiba_acpi.c.orig +4
-2@@ -263,9 +263,11 @@ dispatch_write(struct file* file, const* destination so that sscanf can be used on it safely. */ tmp_buffer =kmalloc (count +1 ,GFP_KERNEL ); -if (copy_from_user (tmp_buffer, buffer, count)) { + +if (!tmp_buffer) +return -ENOMEM ; +else if (copy_from_user (tmp_buffer, buffer, count)) result = -EFAULT ; - }else { tmp_buffer[count] =0 ; result = item->write_func (tmp_buffer, count);Quoted 6 lines · randy_dunlap
On Sun, 10 Jul 2005 21:20:44 -0400 Brandon Niemczyk wrote:
<snip>
Looks correct functionally, but needs a space after "if", like "if (".Oh, don't use braces for one-line "blocks".
Yes thecopy_from_user()below does that too. :(
Quoted 6 lines
------------------------------------------------------------------------
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
Quoted 2 lines · walter harms
hi Brandon,
keep it simple
this better?
drivers/acpi/toshiba_acpi.c renamed from drivers/acpi/toshiba_acpi.c.orig +13 -11
@@ -252,24 +252,26 @@ dispatch_read(char* page, char** start, }static int -dispatch_write (struct file * file,const char __user * buffer, -unsigned long count,ProcItem * item) +dispatch_write (struct file *file,const char __user * buffer, +unsigned long count,ProcItem * item) {int result; -char * tmp_buffer; +char *tmp_buffer; -/* Arg buffer points to userspace memory, which can't be accessed -* directly. Since we're making a copy, zero-terminate the -* destination so that sscanf can be used on it safely. -*/ tmp_buffer =kmalloc (count +1 ,GFP_KERNEL ); +if (!tmp_buffer) +return -ENOMEM ; +if (copy_from_user (tmp_buffer, buffer, count)) { result = -EFAULT ; + goto out; } -else { - tmp_buffer[count] =0 ; - result = item->write_func (tmp_buffer, count); - } + +/* make sure sscanf can be used safely */ + tmp_buffer[count] =0 ; + result = item->write_func (tmp_buffer, count); + +out:kfree (tmp_buffer);return result; }
Quoted 7 lines
tmp_buffer=kmalloc(count + 1, GFP_KERNEL);
if (!tmp_buffer)return -ENOMEM;if (
copy_from_user(tmp_buffer, buffer, count))result = -EFAULT;
Quoted 7 lines
tmp_buffer[count] = 0;
result =item->write_func(tmp_buffer, count);just for the paranoid:
shouldtmp_bufferbe filled with \0 to avoid an information leak ? (schroedinger bug ?)
Not sure.
Signature · 1 line
Brandon Niemczyk
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
Quoted 7 lines · walter harms
tmp_buffer=kmalloc(count + 1, GFP_KERNEL);
if (!tmp_buffer)return -ENOMEM;if (
copy_from_user(tmp_buffer, buffer, count))result = -EFAULT;
Quoted 7 lines
tmp_buffer[count] = 0;
result =item->write_func(tmp_buffer, count);just for the paranoid:
shouldtmp_bufferbe filled with \0 to avoid an information leak ? (schroedinger bug ?)
We copy _from_ userspace.
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
Quoted 12 lines · Domen Puncer
Quoted 11 lines · walter harms
tmp_buffer=kmalloc(count + 1, GFP_KERNEL);
if (!tmp_buffer)return -ENOMEM;if (
copy_from_user(tmp_buffer, buffer, count))result = -EFAULT;tmp_buffer[count] = 0;
result =item->write_func(tmp_buffer, count);
Quoted 6 lines
Quoted 3 lines
just for the paranoid:
shouldtmp_bufferbe filled with \0 to avoid an information leak ? (schroedinger bug ?)We copy _from_ userspace.
UPS, you are totaly right :)
ntl perhaps add to the TODO list.
Check buffers that are used with copy_to_user() a really clean to avoid information leakage.
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
Quoted 4 lines · walter harms
ntl perhaps add to the TODO list.
Check buffers that are used with
copy_to_user()a really clean to avoid information leakage.
Good idea, applied this:
TODO 11 Jul 2005 17:06:40 -0000 renamed from TODO 19 Jun 2005 19:40:32 -0000 1.34 +1 -0
@@ -169,6 +169,7 @@ - go through all the tty/serial drivers and make sure they don't give out excessively useful information to non CAP_SYS_RAWIO users, then loosen permissions. [D: http://lkml.org/lkml/2005/1/17/94] +- check that buffers used in copy_to_user() don't leak information. - check for dev_close calls without rntl_lock held (causes assertion failures). - timer_del() vs. timer_del_sync()
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors