* Re: [KJ] [PATCH] toshiba_acpi check kmalloc return value
2005-07-11 1:20 [KJ] [PATCH] toshiba_acpi check kmalloc return value Brandon Niemczyk
@ 2005-07-11 2:32 ` randy_dunlap
2005-07-11 3:07 ` Brandon Niemczyk
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: randy_dunlap @ 2005-07-11 2:32 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 844 bytes --]
On Sun, 10 Jul 2005 21:20:44 -0400 Brandon Niemczyk wrote:
| Signed-off-by: Brandon Niemczyk <brandon@snprogramming.com>
|
| --- 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
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [KJ] [PATCH] toshiba_acpi check kmalloc return value
2005-07-11 1:20 [KJ] [PATCH] toshiba_acpi check kmalloc return value Brandon Niemczyk
2005-07-11 2:32 ` randy_dunlap
@ 2005-07-11 3:07 ` Brandon Niemczyk
2005-07-11 9:53 ` walter harms
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Brandon Niemczyk @ 2005-07-11 3:07 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 1003 bytes --]
Here it is with the stylistic fixes:
Signed-off-by: Brandon Niemczyk <brandon@snprogramming.com>
--- linux/drivers/acpi/toshiba_acpi.c.orig 2005-07-10 20:40:47.000000000 -0400
+++ linux/drivers/acpi/toshiba_acpi.c 2005-07-10 23:00:43.000000000 -0400
@@ -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);
On Sun, 2005-07-10 at 19:32 -0700, randy_dunlap wrote:
> 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 the copy_from_user() below does that too. :(
--
Brandon Niemczyk
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [KJ] [PATCH] toshiba_acpi check kmalloc return value
2005-07-11 1:20 [KJ] [PATCH] toshiba_acpi check kmalloc return value Brandon Niemczyk
2005-07-11 2:32 ` randy_dunlap
2005-07-11 3:07 ` Brandon Niemczyk
@ 2005-07-11 9:53 ` walter harms
2005-07-11 11:24 ` Brandon Niemczyk
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: walter harms @ 2005-07-11 9:53 UTC (permalink / raw)
To: 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
Brandon Niemczyk wrote:
> Here it is with the stylistic fixes:
>
> Signed-off-by: Brandon Niemczyk <brandon@snprogramming.com>
>
> --- linux/drivers/acpi/toshiba_acpi.c.orig 2005-07-10 20:40:47.000000000 -0400
> +++ linux/drivers/acpi/toshiba_acpi.c 2005-07-10 23:00:43.000000000 -0400
> @@ -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);
>
> On Sun, 2005-07-10 at 19:32 -0700, randy_dunlap wrote:
>>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 the copy_from_user() below does that too. :(
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> Kernel-janitors mailing list
> Kernel-janitors@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/kernel-janitors
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [KJ] [PATCH] toshiba_acpi check kmalloc return value
2005-07-11 1:20 [KJ] [PATCH] toshiba_acpi check kmalloc return value Brandon Niemczyk
` (2 preceding siblings ...)
2005-07-11 9:53 ` walter harms
@ 2005-07-11 11:24 ` Brandon Niemczyk
2005-07-11 13:01 ` Domen Puncer
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Brandon Niemczyk @ 2005-07-11 11:24 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 1686 bytes --]
On Mon, 2005-07-11 at 11:53 +0200, walter harms wrote:
> hi Brandon,
> keep it simple
this better?
Signed-off-by: Brandon Niemczyk <brandon@snprogramming.com>
--- p1/drivers/acpi/toshiba_acpi.c.orig 2005-07-11 07:14:14.000000000 -0400
+++ p1/drivers/acpi/toshiba_acpi.c 2005-06-17 15:48:29.000000000 -0400
@@ -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;
}
> 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 ?)
Not sure.
--
Brandon Niemczyk
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [KJ] [PATCH] toshiba_acpi check kmalloc return value
2005-07-11 1:20 [KJ] [PATCH] toshiba_acpi check kmalloc return value Brandon Niemczyk
` (3 preceding siblings ...)
2005-07-11 11:24 ` Brandon Niemczyk
@ 2005-07-11 13:01 ` Domen Puncer
2005-07-11 15:55 ` walter harms
2005-07-11 17:08 ` Domen Puncer
6 siblings, 0 replies; 8+ messages in thread
From: Domen Puncer @ 2005-07-11 13:01 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 433 bytes --]
On 11/07/05 11:53 +0200, walter harms wrote:
> 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 ?)
We copy _from_ userspace.
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [KJ] [PATCH] toshiba_acpi check kmalloc return value
2005-07-11 1:20 [KJ] [PATCH] toshiba_acpi check kmalloc return value Brandon Niemczyk
` (4 preceding siblings ...)
2005-07-11 13:01 ` Domen Puncer
@ 2005-07-11 15:55 ` walter harms
2005-07-11 17:08 ` Domen Puncer
6 siblings, 0 replies; 8+ messages in thread
From: walter harms @ 2005-07-11 15:55 UTC (permalink / raw)
To: kernel-janitors
Domen Puncer wrote:
> On 11/07/05 11:53 +0200, walter harms wrote:
>>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 ?)
>
> 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
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [KJ] [PATCH] toshiba_acpi check kmalloc return value
2005-07-11 1:20 [KJ] [PATCH] toshiba_acpi check kmalloc return value Brandon Niemczyk
` (5 preceding siblings ...)
2005-07-11 15:55 ` walter harms
@ 2005-07-11 17:08 ` Domen Puncer
6 siblings, 0 replies; 8+ messages in thread
From: Domen Puncer @ 2005-07-11 17:08 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 719 bytes --]
On 11/07/05 17:55 +0200, walter harms wrote:
> 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 19 Jun 2005 19:40:32 -0000 1.34
+++ TODO 11 Jul 2005 17:06:40 -0000
@@ -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()
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 8+ messages in thread