[KJ] [PATCH] toshiba_acpi check kmalloc return value

[KJ] [PATCH] toshiba_acpi check kmalloc return value

[Brandon Niemczyk]

[-- Attachment #1: Type: text/plain, Size: 574 bytes --]

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) {
+		return -ENOMEM;
+	} else if (copy_from_user(tmp_buffer, buffer, count)) {
 		result = -EFAULT;
 	}
 	else {




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

Re: [KJ] [PATCH] toshiba_acpi check kmalloc return value

[randy_dunlap]

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

Re: [KJ] [PATCH] toshiba_acpi check kmalloc return value

[Brandon Niemczyk]

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

Re: [KJ] [PATCH] toshiba_acpi check kmalloc return value

[walter harms]

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

Re: [KJ] [PATCH] toshiba_acpi check kmalloc return value

[Brandon Niemczyk]

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

Re: [KJ] [PATCH] toshiba_acpi check kmalloc return value

[Domen Puncer]

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

Re: [KJ] [PATCH] toshiba_acpi check kmalloc return value

[walter harms]

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

Re: [KJ] [PATCH] toshiba_acpi check kmalloc return value

[Domen Puncer]

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