[PATCH] toshiba_acpi 0.18

[PATCH] toshiba_acpi 0.18

[John Belmonte]

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

Attached is a patch for linux-2.6.4 which yields toshiba_acpi 0.18.  It 
should apply against the 2.4 kernel also.

This version fixes illegal userspace memory access reported at 
<http://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=117682>.

It appears that the asus_acpi driver has the same issue, as it was 
derived from mine.

-John


-- 
http:// if  ile.org/

[-- Attachment #2: toshiba_acpi_0.18-linux_2.6.4.patch --]
[-- Type: text/x-patch, Size: 3207 bytes --]

diff -urN linux-2.6.4/drivers/acpi/toshiba_acpi.c new/drivers/acpi/toshiba_acpi.c
--- linux-2.6.4/drivers/acpi/toshiba_acpi.c	2004-03-13 21:09:26.000000000 -0500
+++ new/drivers/acpi/toshiba_acpi.c	2004-03-13 21:09:35.000000000 -0500
@@ -33,7 +33,7 @@
  *
  */
 
-#define TOSHIBA_ACPI_VERSION	"0.17"
+#define TOSHIBA_ACPI_VERSION	"0.18"
 #define PROC_INTERFACE_VERSION	1
 
 #include <linux/kernel.h>
@@ -41,6 +41,7 @@
 #include <linux/init.h>
 #include <linux/types.h>
 #include <linux/proc_fs.h>
+#include <asm/uaccess.h>
 
 #include <acpi/acpi_drivers.h>
 
@@ -105,24 +106,6 @@
 	*word = (*word & ~mask) | (mask * value);
 }
 
-/* an sscanf that takes explicit string length */
-static int
-snscanf(const char* str, int n, const char* format, ...)
-{
-	va_list args;
-	int result;
-	char* str2 = kmalloc(n + 1, GFP_KERNEL);
-	if (str2 == 0) return 0;
-	/* NOTE: don't even _think_ about replacing this with strlcpy */
-	strncpy(str2, str, n);
-	str2[n] = 0;
-	va_start(args, format);
-	result = vsscanf(str2, format, args);
-	va_end(args);
-	kfree(str2);
-	return result;
-}
-
 /* acpi interface wrappers
  */
 
@@ -272,7 +255,23 @@
 dispatch_write(struct file* file, const char* buffer, unsigned long count,
 	ProcItem* item)
 {
-	return item->write_func(buffer, count);
+	int result;
+	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 (copy_from_user(tmp_buffer, buffer, count)) {
+		result = -EFAULT;
+	}
+	else {
+		tmp_buffer[count] = 0;
+		result = item->write_func(tmp_buffer, count);
+	}
+	kfree(tmp_buffer);
+	return result;
 }
 
 static char*
@@ -300,7 +299,7 @@
 	int value;
 	u32 hci_result;
 
-	if (snscanf(buffer, count, " brightness : %i", &value) == 1 &&
+	if (sscanf(buffer, " brightness : %i", &value) == 1 &&
 			value >= 0 && value < HCI_LCD_BRIGHTNESS_LEVELS) {
 		value = value << HCI_LCD_BRIGHTNESS_SHIFT;
 		hci_write1(HCI_LCD_BRIGHTNESS, value, &hci_result);
@@ -350,11 +349,11 @@
 	 *  NOTE: to keep scanning simple, invalid fields are ignored
 	 */
 	while (remain) {
-		if (snscanf(buffer, remain, " lcd_out : %i", &value) == 1)
+		if (sscanf(buffer, " lcd_out : %i", &value) == 1)
 			lcd_out = value & 1;
-		else if (snscanf(buffer, remain, " crt_out : %i", &value) == 1)
+		else if (sscanf(buffer, " crt_out : %i", &value) == 1)
 			crt_out = value & 1;
-		else if (snscanf(buffer, remain, " tv_out : %i", &value) == 1)
+		else if (sscanf(buffer, " tv_out : %i", &value) == 1)
 			tv_out = value & 1;
 		/* advance to one character past the next ; */
 		do {
@@ -407,7 +406,7 @@
 	int value;
 	u32 hci_result;
 
-	if (snscanf(buffer, count, " force_on : %i", &value) == 1 &&
+	if (sscanf(buffer, " force_on : %i", &value) == 1 &&
 			value >= 0 && value <= 1) {
 		hci_write1(HCI_FAN, value, &hci_result);
 		if (hci_result != HCI_SUCCESS)
@@ -458,7 +457,7 @@
 {
 	int value;
 
-	if (snscanf(buffer, count, " hotkey_ready : %i", &value) == 1 &&
+	if (sscanf(buffer, " hotkey_ready : %i", &value) == 1 &&
 			value == 0) {
 		key_event_valid = 0;
 	} else {

Re: [PATCH] toshiba_acpi 0.18

[Len Brown]

I think that the "__user" attribute is needed on the user pointer param
to copy_from_user() -- at least in 2.6.

thanks,
-Len

On Sat, 2004-03-13 at 21:35, John Belmonte wrote:
> Attached is a patch for linux-2.6.4 which yields toshiba_acpi 0.18.  It 
> should apply against the 2.4 kernel also.
> 
> This version fixes illegal userspace memory access reported at 
> <http://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=117682>.
> 
> It appears that the asus_acpi driver has the same issue, as it was 
> derived from mine.
> 
> -John
> 



-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click

Re: [PATCH] toshiba_acpi 0.18

[John Belmonte]

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

Len Brown wrote:
> I think that the "__user" attribute is needed on the user pointer param
> to copy_from_user() -- at least in 2.6.

Thanks Len, updated patch attached.

-John


-- 
http:// if  ile.org/

[-- Attachment #2: toshiba_acpi_0.18-linux_2.6.4.patch --]
[-- Type: text/x-patch, Size: 3326 bytes --]

diff -urN linux-2.6.4/drivers/acpi/toshiba_acpi.c new/drivers/acpi/toshiba_acpi.c
--- linux-2.6.4/drivers/acpi/toshiba_acpi.c	2004-03-13 21:09:26.000000000 -0500
+++ new/drivers/acpi/toshiba_acpi.c	2004-03-14 00:59:45.000000000 -0500
@@ -33,7 +33,7 @@
  *
  */
 
-#define TOSHIBA_ACPI_VERSION	"0.17"
+#define TOSHIBA_ACPI_VERSION	"0.18"
 #define PROC_INTERFACE_VERSION	1
 
 #include <linux/kernel.h>
@@ -41,6 +41,7 @@
 #include <linux/init.h>
 #include <linux/types.h>
 #include <linux/proc_fs.h>
+#include <asm/uaccess.h>
 
 #include <acpi/acpi_drivers.h>
 
@@ -105,24 +106,6 @@
 	*word = (*word & ~mask) | (mask * value);
 }
 
-/* an sscanf that takes explicit string length */
-static int
-snscanf(const char* str, int n, const char* format, ...)
-{
-	va_list args;
-	int result;
-	char* str2 = kmalloc(n + 1, GFP_KERNEL);
-	if (str2 == 0) return 0;
-	/* NOTE: don't even _think_ about replacing this with strlcpy */
-	strncpy(str2, str, n);
-	str2[n] = 0;
-	va_start(args, format);
-	result = vsscanf(str2, format, args);
-	va_end(args);
-	kfree(str2);
-	return result;
-}
-
 /* acpi interface wrappers
  */
 
@@ -269,10 +252,26 @@
 }
 
 static int
-dispatch_write(struct file* file, const char* buffer, unsigned long count,
-	ProcItem* item)
+dispatch_write(struct file* file, __user const char* buffer,
+	unsigned long count, ProcItem* item)
 {
-	return item->write_func(buffer, count);
+	int result;
+	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 (copy_from_user(tmp_buffer, buffer, count)) {
+		result = -EFAULT;
+	}
+	else {
+		tmp_buffer[count] = 0;
+		result = item->write_func(tmp_buffer, count);
+	}
+	kfree(tmp_buffer);
+	return result;
 }
 
 static char*
@@ -300,7 +299,7 @@
 	int value;
 	u32 hci_result;
 
-	if (snscanf(buffer, count, " brightness : %i", &value) == 1 &&
+	if (sscanf(buffer, " brightness : %i", &value) == 1 &&
 			value >= 0 && value < HCI_LCD_BRIGHTNESS_LEVELS) {
 		value = value << HCI_LCD_BRIGHTNESS_SHIFT;
 		hci_write1(HCI_LCD_BRIGHTNESS, value, &hci_result);
@@ -350,11 +349,11 @@
 	 *  NOTE: to keep scanning simple, invalid fields are ignored
 	 */
 	while (remain) {
-		if (snscanf(buffer, remain, " lcd_out : %i", &value) == 1)
+		if (sscanf(buffer, " lcd_out : %i", &value) == 1)
 			lcd_out = value & 1;
-		else if (snscanf(buffer, remain, " crt_out : %i", &value) == 1)
+		else if (sscanf(buffer, " crt_out : %i", &value) == 1)
 			crt_out = value & 1;
-		else if (snscanf(buffer, remain, " tv_out : %i", &value) == 1)
+		else if (sscanf(buffer, " tv_out : %i", &value) == 1)
 			tv_out = value & 1;
 		/* advance to one character past the next ; */
 		do {
@@ -407,7 +406,7 @@
 	int value;
 	u32 hci_result;
 
-	if (snscanf(buffer, count, " force_on : %i", &value) == 1 &&
+	if (sscanf(buffer, " force_on : %i", &value) == 1 &&
 			value >= 0 && value <= 1) {
 		hci_write1(HCI_FAN, value, &hci_result);
 		if (hci_result != HCI_SUCCESS)
@@ -458,7 +457,7 @@
 {
 	int value;
 
-	if (snscanf(buffer, count, " hotkey_ready : %i", &value) == 1 &&
+	if (sscanf(buffer, " hotkey_ready : %i", &value) == 1 &&
 			value == 0) {
 		key_event_valid = 0;
 	} else {

Re: [PATCH] toshiba_acpi 0.18

[Karol Kozimor]

Thus wrote John Belmonte:
> It appears that the asus_acpi driver has the same issue, as it was 
> derived from mine.

While I think it doesn't oops (at least I can't reproduce it), it doesn't
work either when CONFIG_X86_4G=y. I'll have a fix later on.
Best regards,

-- 
Karol 'sziwan' Kozimor
sziwan-DETuoxkZsSqrDJvtcaxF/A@public.gmane.org


-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click

Re: [PATCH] toshiba_acpi 0.18

[Len Brown]

Appplied.

thanks,
-Len

On Sun, 2004-03-14 at 01:02, John Belmonte wrote:
> Len Brown wrote:
> > I think that the "__user" attribute is needed on the user pointer param
> > to copy_from_user() -- at least in 2.6.
> 
> Thanks Len, updated patch attached.
> 
> -John
> 



-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click

Re: [PATCH] toshiba_acpi 0.18

[Karol Kozimor]

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

Thus wrote Karol Kozimor:
> > It appears that the asus_acpi driver has the same issue, as it was 
> > derived from mine.
> While I think it doesn't oops (at least I can't reproduce it), it doesn't
> work either when CONFIG_X86_4G=y. I'll have a fix later on.

The attached patches introduce copy_from_user() where appropriate. I only
tested this code with 2.6.5-rc1-mm2, where it worked both with and without
CONFIG_X86_4G. For 2.4, compile-test only.

Note: I'm not sure if the 2.4 version is needed, there's no CONFIG_X86_4G
there. Does a highmem-enabled kernel need such a conversion? I can't really
test those changes reliably having only 256 MB RAM, but at least those
changes don't seem to do any harm.

Please apply if you feel so.
Best regards,

-- 
Karol 'sziwan' Kozimor
sziwan-DETuoxkZsSqrDJvtcaxF/A@public.gmane.org

[-- Attachment #2: copy_from_user-2.4.diff --]
[-- Type: text/plain, Size: 2443 bytes --]

--- a/drivers/acpi/asus_acpi.c	2004-02-18 14:36:31.000000000 +0100
+++ b/drivers/acpi/asus_acpi.c	2004-03-23 16:41:48.000000000 +0100
@@ -40,6 +40,7 @@
 #include <linux/proc_fs.h>
 #include <acpi/acpi_drivers.h>
 #include <acpi/acpi_bus.h>
+#include <asm/uaccess.h>
 
 #define ASUS_ACPI_VERSION "0.27"
 
@@ -499,9 +500,16 @@ write_led(const char *buffer, unsigned l
 {
 	int value;
 	int led_out = 0;
+	char *tmp_buffer;
 
-	if (sscanf(buffer, "%i", &value) == 1)
+	tmp_buffer = kmalloc(count + 1, GFP_KERNEL);
+	if (copy_from_user(tmp_buffer, buffer, count))
+		return -EFAULT;
+	else
+		tmp_buffer[count] = '\0';
+	if (sscanf(tmp_buffer, "%i", &value) == 1)
 		led_out = value ? 1 : 0;
+	kfree(tmp_buffer);
 
 	hotk->status =
 	    (led_out) ? (hotk->status | ledmask) : (hotk->status & ~ledmask);
@@ -656,9 +664,17 @@ proc_write_lcd(struct file *file, const 
 {
 	int value;
 	struct asus_hotk *hotk = (struct asus_hotk *) data;
+	char *tmp_buffer;
 	
-	if (sscanf(buffer, "%i", &value) == 1)
+	tmp_buffer = kmalloc(count + 1, GFP_KERNEL);
+	if (copy_from_user(tmp_buffer, buffer, count))
+		return -EFAULT;
+	else
+		tmp_buffer[count] = '\0';
+	if (sscanf(tmp_buffer, "%i", &value) == 1)
 		set_lcd_state(hotk, value);
+	kfree(tmp_buffer);
+
 	return count;
 }
 
@@ -723,14 +739,21 @@ proc_write_brn(struct file *file, const 
 {
 	int value;
 	struct asus_hotk *hotk = (struct asus_hotk *) data;
+	char *tmp_buffer;
 
-	if (sscanf(buffer, "%d", &value) == 1) {
+	tmp_buffer = kmalloc(count + 1, GFP_KERNEL);
+	if (copy_from_user(tmp_buffer, buffer, count))
+		return -EFAULT;
+	else
+		tmp_buffer[count] = '\0';
+	if (sscanf(tmp_buffer, "%d", &value) == 1) {
 		value = (0 < value) ? ((15 < value) ? 15 : value) : 0;
 			/* 0 <= value <= 15 */
 		set_brightness(value, hotk);
 	} else {
 		printk(KERN_WARNING "Asus ACPI: Error reading user input\n");
 	}
+	kfree(tmp_buffer);
 
 	return count;
 }
@@ -772,12 +795,19 @@ proc_write_disp(struct file *file, const
 {
 	int value;
 	struct asus_hotk *hotk = (struct asus_hotk *) data;
+	char *tmp_buffer;
 
-	if (sscanf(buffer, "%d", &value) == 1)
+	tmp_buffer = kmalloc(count + 1, GFP_KERNEL);
+	if (copy_from_user(tmp_buffer, buffer, count))
+		return -EFAULT;
+	else
+		tmp_buffer[count] = '\0';
+	if (sscanf(tmp_buffer, "%d", &value) == 1)
 		set_display(value, hotk);
 	else {
 		printk(KERN_WARNING "Asus ACPI: Error reading user input\n");
 	}
+	kfree(tmp_buffer);
 
 	return count;
 }

[-- Attachment #3: copy_from_user-2.6.diff --]
[-- Type: text/plain, Size: 3154 bytes --]

--- asus_acpi.c.orig	2004-03-21 16:48:28.000000000 +0100
+++ asus_acpi.c	2004-03-21 16:55:18.000000000 +0100
@@ -40,6 +40,7 @@
 #include <linux/proc_fs.h>
 #include <acpi/acpi_drivers.h>
 #include <acpi/acpi_bus.h>
+#include <asm/uaccess.h>
 
 #define ASUS_ACPI_VERSION "0.27"
 
@@ -494,14 +495,21 @@
 
 /* FIXME: kill extraneous args so it can be called independently */
 static int
-write_led(const char *buffer, unsigned long count, struct asus_hotk *hotk, 
+write_led(const char __user *buffer, unsigned long count, struct asus_hotk *hotk, 
           char *ledname, int ledmask, int invert)
 {
 	int value;
 	int led_out = 0;
+	char *tmp_buffer;
 
-	if (sscanf(buffer, "%i", &value) == 1)
+	tmp_buffer = kmalloc(count + 1, GFP_KERNEL);
+	if (copy_from_user(tmp_buffer, buffer, count))
+		return -EFAULT;
+	else
+		tmp_buffer[count] = '\0';
+	if (sscanf(tmp_buffer, "%i", &value) == 1)
 		led_out = value ? 1 : 0;
+	kfree(tmp_buffer);
 
 	hotk->status =
 	    (led_out) ? (hotk->status | ledmask) : (hotk->status & ~ledmask);
@@ -651,14 +659,22 @@
 
 
 static int
-proc_write_lcd(struct file *file, const char *buffer,
+proc_write_lcd(struct file *file, const char __user *buffer,
 	       unsigned long count, void *data)
 {
 	int value;
 	struct asus_hotk *hotk = (struct asus_hotk *) data;
+	char *tmp_buffer;
 	
-	if (sscanf(buffer, "%i", &value) == 1)
+	tmp_buffer = kmalloc(count + 1, GFP_KERNEL);
+	if (copy_from_user(tmp_buffer, buffer, count))
+		return -EFAULT;
+	else
+		tmp_buffer[count] = '\0';
+	if (sscanf(tmp_buffer, "%i", &value) == 1)
 		set_lcd_state(hotk, value);
+	kfree(tmp_buffer);
+
 	return count;
 }
 
@@ -718,19 +734,26 @@
 }
 
 static int
-proc_write_brn(struct file *file, const char *buffer,
+proc_write_brn(struct file *file, const char __user *buffer,
 	       unsigned long count, void *data)
 {
 	int value;
 	struct asus_hotk *hotk = (struct asus_hotk *) data;
+	char *tmp_buffer;
 
-	if (sscanf(buffer, "%d", &value) == 1) {
+	tmp_buffer = kmalloc(count + 1, GFP_KERNEL);
+	if (copy_from_user(tmp_buffer, buffer, count))
+		return -EFAULT;
+	else
+		tmp_buffer[count] = '\0';
+	if (sscanf(tmp_buffer, "%d", &value) == 1) {
 		value = (0 < value) ? ((15 < value) ? 15 : value) : 0;
 			/* 0 <= value <= 15 */
 		set_brightness(value, hotk);
 	} else {
 		printk(KERN_WARNING "Asus ACPI: Error reading user input\n");
 	}
+	kfree(tmp_buffer);
 
 	return count;
 }
@@ -767,17 +790,24 @@
  * simultaneously, so be warned. See the acpi4asus README for more info.
  */
 static int
-proc_write_disp(struct file *file, const char *buffer,
+proc_write_disp(struct file *file, const char __user *buffer,
 	       unsigned long count, void *data)
 {
 	int value;
 	struct asus_hotk *hotk = (struct asus_hotk *) data;
+	char *tmp_buffer;
 
-	if (sscanf(buffer, "%d", &value) == 1)
+	tmp_buffer = kmalloc(count + 1, GFP_KERNEL);
+	if (copy_from_user(tmp_buffer, buffer, count))
+		return -EFAULT;
+	else
+		tmp_buffer[count] = '\0';
+	if (sscanf(tmp_buffer, "%d", &value) == 1)
 		set_display(value, hotk);
 	else {
 		printk(KERN_WARNING "Asus ACPI: Error reading user input\n");
 	}
+	kfree(tmp_buffer);
 
 	return count;
 }

Re: [PATCH] toshiba_acpi 0.18

[John Belmonte]

> @@ -499,9 +500,16 @@ write_led(const char *buffer, unsigned l
>  {
>  	int value;
>  	int led_out = 0;
> +	char *tmp_buffer;
>  
> -	if (sscanf(buffer, "%i", &value) == 1)
> +	tmp_buffer = kmalloc(count + 1, GFP_KERNEL);
> +	if (copy_from_user(tmp_buffer, buffer, count))
> +		return -EFAULT;
> +	else
> +		tmp_buffer[count] = '\0';
> +	if (sscanf(tmp_buffer, "%i", &value) == 1)
>  		led_out = value ? 1 : 0;
> +	kfree(tmp_buffer);

Note that if copy_from_user fails, this code leaks memory.

-John


-- 
http:// if  ile.org/


-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click

Re: [PATCH] toshiba_acpi 0.18

[Karol Kozimor]

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

Thus wrote John Belmonte:
> Note that if copy_from_user fails, this code leaks memory.

Nah, I somehow assumed we don't have to worry after -EFAULT, my bad.
Updated patches attached.
Best regards,

-- 
Karol 'sziwan' Kozimor
sziwan-DETuoxkZsSqrDJvtcaxF/A@public.gmane.org

[-- Attachment #2: copy_from_user-2.4.diff --]
[-- Type: text/plain, Size: 2880 bytes --]

--- ../../kernels/linux-2.4.25/drivers/acpi/asus_acpi.c	2004-02-18 14:36:31.000000000 +0100
+++ ./asus_acpi.c	2004-03-24 12:07:08.000000000 +0100
@@ -40,6 +40,7 @@
 #include <linux/proc_fs.h>
 #include <acpi/acpi_drivers.h>
 #include <acpi/acpi_bus.h>
+#include <asm/uaccess.h>
 
 #define ASUS_ACPI_VERSION "0.27"
 
@@ -499,9 +500,17 @@ write_led(const char *buffer, unsigned l
 {
 	int value;
 	int led_out = 0;
+	char *tmp_buffer;
 
-	if (sscanf(buffer, "%i", &value) == 1)
+	tmp_buffer = kmalloc(count + 1, GFP_KERNEL);
+	if (copy_from_user(tmp_buffer, buffer, count)) {
+		kfree(tmp_buffer);
+		return -EFAULT;
+	} else
+		tmp_buffer[count] = '\0';
+	if (sscanf(tmp_buffer, "%i", &value) == 1)
 		led_out = value ? 1 : 0;
+	kfree(tmp_buffer);
 
 	hotk->status =
 	    (led_out) ? (hotk->status | ledmask) : (hotk->status & ~ledmask);
@@ -656,9 +665,18 @@ proc_write_lcd(struct file *file, const 
 {
 	int value;
 	struct asus_hotk *hotk = (struct asus_hotk *) data;
+	char *tmp_buffer;
 	
-	if (sscanf(buffer, "%i", &value) == 1)
+	tmp_buffer = kmalloc(count + 1, GFP_KERNEL);
+	if (copy_from_user(tmp_buffer, buffer, count)) {
+		kfree(tmp_buffer);
+		return -EFAULT;
+	} else
+		tmp_buffer[count] = '\0';
+	if (sscanf(tmp_buffer, "%i", &value) == 1)
 		set_lcd_state(hotk, value);
+	kfree(tmp_buffer);
+
 	return count;
 }
 
@@ -723,14 +741,22 @@ proc_write_brn(struct file *file, const 
 {
 	int value;
 	struct asus_hotk *hotk = (struct asus_hotk *) data;
+	char *tmp_buffer;
 
-	if (sscanf(buffer, "%d", &value) == 1) {
+	tmp_buffer = kmalloc(count + 1, GFP_KERNEL);
+	if (copy_from_user(tmp_buffer, buffer, count)) {
+		kfree(tmp_buffer);
+		return -EFAULT;
+	} else
+		tmp_buffer[count] = '\0';
+	if (sscanf(tmp_buffer, "%d", &value) == 1) {
 		value = (0 < value) ? ((15 < value) ? 15 : value) : 0;
 			/* 0 <= value <= 15 */
 		set_brightness(value, hotk);
 	} else {
 		printk(KERN_WARNING "Asus ACPI: Error reading user input\n");
 	}
+	kfree(tmp_buffer);
 
 	return count;
 }
@@ -772,12 +798,20 @@ proc_write_disp(struct file *file, const
 {
 	int value;
 	struct asus_hotk *hotk = (struct asus_hotk *) data;
+	char *tmp_buffer;
 
-	if (sscanf(buffer, "%d", &value) == 1)
+	tmp_buffer = kmalloc(count + 1, GFP_KERNEL);
+	if (copy_from_user(tmp_buffer, buffer, count)) {
+		kfree(tmp_buffer);
+		return -EFAULT;
+	} else
+		tmp_buffer[count] = '\0';
+	if (sscanf(tmp_buffer, "%d", &value) == 1)
 		set_display(value, hotk);
 	else {
 		printk(KERN_WARNING "Asus ACPI: Error reading user input\n");
 	}
+	kfree(tmp_buffer);
 
 	return count;
 }
@@ -1006,7 +1040,6 @@ static int __init asus_hotk_get_info(str
 }
 
 
-
 static int __init asus_hotk_check(struct asus_hotk *hotk)
 {
 	int result = 0;
@@ -1029,7 +1062,6 @@ static int __init asus_hotk_check(struct
 }
 
 
-
 static int __init asus_hotk_add(struct acpi_device *device)
 {
 	struct asus_hotk *hotk = NULL;

[-- Attachment #3: copy_from_user-2.6.diff --]
[-- Type: text/plain, Size: 3778 bytes --]

--- ../../kernels/linux-2.6.5/drivers/acpi/asus_acpi.c	2004-03-21 16:48:28.000000000 +0100
+++ ./asus_acpi.c	2004-03-24 12:09:51.000000000 +0100
@@ -40,6 +40,7 @@
 #include <linux/proc_fs.h>
 #include <acpi/acpi_drivers.h>
 #include <acpi/acpi_bus.h>
+#include <asm/uaccess.h>
 
 #define ASUS_ACPI_VERSION "0.27"
 
@@ -494,14 +495,22 @@ read_led(struct asus_hotk *hotk, const c
 
 /* FIXME: kill extraneous args so it can be called independently */
 static int
-write_led(const char *buffer, unsigned long count, struct asus_hotk *hotk, 
+write_led(const char __user *buffer, unsigned long count, struct asus_hotk *hotk,
           char *ledname, int ledmask, int invert)
 {
 	int value;
 	int led_out = 0;
+	char *tmp_buffer;
 
-	if (sscanf(buffer, "%i", &value) == 1)
+	tmp_buffer = kmalloc(count + 1, GFP_KERNEL);
+	if (copy_from_user(tmp_buffer, buffer, count)) {
+		kfree(tmp_buffer);
+		return -EFAULT;
+	} else
+		tmp_buffer[count] = '\0';
+	if (sscanf(tmp_buffer, "%i", &value) == 1)
 		led_out = value ? 1 : 0;
+	kfree(tmp_buffer);
 
 	hotk->status =
 	    (led_out) ? (hotk->status | ledmask) : (hotk->status & ~ledmask);
@@ -651,14 +660,23 @@ proc_read_lcd(char *page, char **start, 
 
 
 static int
-proc_write_lcd(struct file *file, const char *buffer,
+proc_write_lcd(struct file *file, const char __user *buffer,
 	       unsigned long count, void *data)
 {
 	int value;
 	struct asus_hotk *hotk = (struct asus_hotk *) data;
+	char *tmp_buffer;
 	
-	if (sscanf(buffer, "%i", &value) == 1)
+	tmp_buffer = kmalloc(count + 1, GFP_KERNEL);
+	if (copy_from_user(tmp_buffer, buffer, count)) {
+		kfree(tmp_buffer);
+		return -EFAULT;
+	} else
+		tmp_buffer[count] = '\0';
+	if (sscanf(tmp_buffer, "%i", &value) == 1)
 		set_lcd_state(hotk, value);
+	kfree(tmp_buffer);
+
 	return count;
 }
 
@@ -718,19 +736,27 @@ proc_read_brn(char *page, char **start, 
 }
 
 static int
-proc_write_brn(struct file *file, const char *buffer,
+proc_write_brn(struct file *file, const char __user *buffer,
 	       unsigned long count, void *data)
 {
 	int value;
 	struct asus_hotk *hotk = (struct asus_hotk *) data;
+	char *tmp_buffer;
 
-	if (sscanf(buffer, "%d", &value) == 1) {
+	tmp_buffer = kmalloc(count + 1, GFP_KERNEL);
+	if (copy_from_user(tmp_buffer, buffer, count)) {
+		kfree(tmp_buffer);
+		return -EFAULT;
+	} else
+		tmp_buffer[count] = '\0';
+	if (sscanf(tmp_buffer, "%d", &value) == 1) {
 		value = (0 < value) ? ((15 < value) ? 15 : value) : 0;
 			/* 0 <= value <= 15 */
 		set_brightness(value, hotk);
 	} else {
 		printk(KERN_WARNING "Asus ACPI: Error reading user input\n");
 	}
+	kfree(tmp_buffer);
 
 	return count;
 }
@@ -767,17 +793,25 @@ proc_read_disp(char *page, char **start,
  * simultaneously, so be warned. See the acpi4asus README for more info.
  */
 static int
-proc_write_disp(struct file *file, const char *buffer,
+proc_write_disp(struct file *file, const char __user *buffer,
 	       unsigned long count, void *data)
 {
 	int value;
 	struct asus_hotk *hotk = (struct asus_hotk *) data;
+	char *tmp_buffer;
 
-	if (sscanf(buffer, "%d", &value) == 1)
+	tmp_buffer = kmalloc(count + 1, GFP_KERNEL);
+	if (copy_from_user(tmp_buffer, buffer, count)) {
+		kfree(tmp_buffer);
+		return -EFAULT;
+	} else
+		tmp_buffer[count] = '\0';
+	if (sscanf(tmp_buffer, "%d", &value) == 1)
 		set_display(value, hotk);
 	else {
 		printk(KERN_WARNING "Asus ACPI: Error reading user input\n");
 	}
+	kfree(tmp_buffer);
 
 	return count;
 }
@@ -1006,7 +1040,6 @@ static int __init asus_hotk_get_info(str
 }
 
 
-
 static int __init asus_hotk_check(struct asus_hotk *hotk)
 {
 	int result = 0;
@@ -1029,7 +1062,6 @@ static int __init asus_hotk_check(struct
 }
 
 
-
 static int __init asus_hotk_add(struct acpi_device *device)
 {
 	struct asus_hotk *hotk = NULL;

Re: toshiba_acpi 0.18

[Sergey Vlasov]

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

On Sun, 14 Mar 2004 01:02:58 -0500 John Belmonte wrote:

>  static int
> -dispatch_write(struct file* file, const char* buffer, unsigned long count,
> -	ProcItem* item)
> +dispatch_write(struct file* file, __user const char* buffer,
> +	unsigned long count, ProcItem* item)
>  {
> -	return item->write_func(buffer, count);
> +	int result;
> +	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 (copy_from_user(tmp_buffer, buffer, count)) {
> +		result = -EFAULT;
> +	}
> +	else {
> +		tmp_buffer[count] = 0;
> +		result = item->write_func(tmp_buffer, count);
> +	}
> +	kfree(tmp_buffer);
> +	return result;
>  }

This is still not enough.  count comes from userspace and can be
arbitrarily large, and this function does not even check the return
value from kmalloc()...

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Re: toshiba_acpi 0.18

[John Belmonte]

Sergey Vlasov wrote:
> On Sun, 14 Mar 2004 01:02:58 -0500 John Belmonte wrote:
> 
>> static int
>>-dispatch_write(struct file* file, const char* buffer, unsigned long count,
>>-	ProcItem* item)
>>+dispatch_write(struct file* file, __user const char* buffer,
>>+	unsigned long count, ProcItem* item)
>> {
>>-	return item->write_func(buffer, count);
>>+	int result;
>>+	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 (copy_from_user(tmp_buffer, buffer, count)) {
>>+		result = -EFAULT;
>>+	}
>>+	else {
>>+		tmp_buffer[count] = 0;
>>+		result = item->write_func(tmp_buffer, count);
>>+	}
>>+	kfree(tmp_buffer);
>>+	return result;
>> }
> 
> 
> This is still not enough.  count comes from userspace and can be
> arbitrarily large, and this function does not even check the return
> value from kmalloc()...

The "count" arg is passed by value, so there is no issue.

You are right about not checking the kmalloc result for an error.  I'll 
fix that, but it's not enough to warrant a new release on its own.  The 
code before the patch had the same problem, so this is not a newly 
created bug, and not related to the problem being addressed.

-John


-- 
http:// if  ile.org/


-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click