Avoid compile warning/error triggered by copy_from_user()

hid-picolcd Applied Bruno Prémont 2 participants 4 messages May 4, 2011 – May 5, 2011

Subsystems (2)

HID Picolcd HID driver

lore

Share & tools

lore

Bruno Prémont

author maintainer

2011-05-04 19:08 UTC #0 lore raw

With CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y compilation of PicoLCD driver fails on copy_from_user(), without it a warning is generated:

CC [M] drivers/hid/hid-picolcd.o
In file included from /usr/src/linux-2.6/arch/x86/include/asm/uaccess.h:571,

                 from /usr/src/linux-2.6/arch/x86/include/asm/sections.h:5,
                 from /usr/src/linux-2.6/arch/x86/include/asm/hw_irq.h:26,
                 from /usr/src/linux-2.6/include/linux/irq.h:359,
                 from /usr/src/linux-2.6/arch/x86/include/asm/hardirq.h:5,
                 from /usr/src/linux-2.6/include/linux/hardirq.h:7,
                 from /usr/src/linux-2.6/include/linux/interrupt.h:12,
                 from /usr/src/linux-2.6/include/linux/usb.h:15,
                 from /usr/src/linux-2.6/drivers/hid/hid-picolcd.c:25:

In function 'copy_from_user',
inlined from 'picolcd_debug_eeprom_write' at drivers/hid/hid-picolcd.c:1592:
arch/x86/include/asm/uaccess_32.h:212: error: call to 'copy_from_user_overflow' declared with attribute error: copy_from_user() buffer size is not provably correct

gcc-4.4.5 is not able to track size calculation when it is stored into a variable, thus tell copy_from_user() maximum size via min(*max-size*, *effective-size*) explicitly and inline how much to copy at most.

Signed-off-by Bruno Prémont <[email protected]>

drivers/hid/hid-picolcd.c +2 -2

index b2f56a1..9d8710f 100644
@@ -1585,11 +1585,11 @@ static ssize_t picolcd_debug_eeprom_write(struct file *f, const char __user *u,
 	memset(raw_data, 0, sizeof(raw_data));
 	raw_data[0] = *off & 0xff;
 	raw_data[1] = (*off >> 8) & 0xff;
-	raw_data[2] = s < 20 ? s : 20;
+	raw_data[2] = min((size_t)20, s);
 	if (*off + raw_data[2] > 0xff)
 		raw_data[2] = 0x100 - *off;
 
-	if (copy_from_user(raw_data+3, u, raw_data[2]))
+	if (copy_from_user(raw_data+3, u, min((u8)20, raw_data[2])))
 		return -EFAULT;
 	resp = picolcd_send_and_wait(data->hdev, REPORT_EE_WRITE, raw_data,
 			sizeof(raw_data));

Jiri Kosina

2011-05-05 09:27 UTC #1 lore raw

Quoted 30 lines · Bruno Prémont

With CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y compilation of PicoLCD driver fails on copy_from_user(), without it a warning is generated:
CC [M] drivers/hid/hid-picolcd.o
In file included from /usr/src/linux-2.6/arch/x86/include/asm/uaccess.h:571,
                 from /usr/src/linux-2.6/arch/x86/include/asm/sections.h:5,
                 from /usr/src/linux-2.6/arch/x86/include/asm/hw_irq.h:26,
                 from /usr/src/linux-2.6/include/linux/irq.h:359,
                 from /usr/src/linux-2.6/arch/x86/include/asm/hardirq.h:5,
                 from /usr/src/linux-2.6/include/linux/hardirq.h:7,
                 from /usr/src/linux-2.6/include/linux/interrupt.h:12,
                 from /usr/src/linux-2.6/include/linux/usb.h:15,
                 from /usr/src/linux-2.6/drivers/hid/hid-picolcd.c:25:
In function 'copy_from_user',
inlined from 'picolcd_debug_eeprom_write' at drivers/hid/hid-picolcd.c:1592:
arch/x86/include/asm/uaccess_32.h:212: error: call to 'copy_from_user_overflow' declared with attribute error: copy_from_user() buffer size is not provably correct
gcc-4.4.5 is not able to track size calculation when it is stored into a variable, thus tell copy_from_user() maximum size via min(*max-size*, *effective-size*) explicitly and inline how much to copy at most.
Signed-off-by Bruno Prémont <[email protected]>
--
drivers/hid/hid-picolcd.c +0 -0
index b2f56a1..9d8710f 100644

drivers/hid/hid-picolcd.c static ssize_t picolcd_debug_eeprom_write(struct file *f, const char __user *u, · line 1592

Quoted 10 lines

drivers/hid/hid-picolcd.c +2 -2

@@ -1585,11 +1585,11 @@ static ssize_t picolcd_debug_eeprom_write(struct file *f, const char __user *u,
 	memset(raw_data, 0, sizeof(raw_data));
 	raw_data[0] = *off & 0xff;
 	raw_data[1] = (*off >> 8) & 0xff;
-	raw_data[2] = s < 20 ? s : 20;
+	raw_data[2] = min((size_t)20, s);
 	if (*off + raw_data[2] > 0xff)
 		raw_data[2] = 0x100 - *off;
 
-	if (copy_from_user(raw_data+3, u, raw_data[2]))
+	if (copy_from_user(raw_data+3, u, min((u8)20, raw_data[2])))

Hmm ... this is quite an obfuscation just for the sake of making gcc happy.

Do other versions of gcc get this right? (i.e. is this gcc bug?)

Don't we have similar problems all over the place in the kernel?

Quoted 4 lines

drivers/hid/hid-picolcd.c +0 -0

 		return -EFAULT;
 	resp = picolcd_send_and_wait(data->hdev, REPORT_EE_WRITE, raw_data,
 			sizeof(raw_data));

Signature · 2 lines

Jiri Kosina
SUSE Labs

Bruno Prémont

author maintainer

2011-05-05 10:04 UTC #2 lore raw

Hi Jiri,

Quoted 40 lines · On Thu, 5 May 2011 11:27:54 wrote:

Quoted 38 lines · Bruno Prémont
With CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y compilation of PicoLCD driver fails on copy_from_user(), without it a warning is generated:
CC [M] drivers/hid/hid-picolcd.o
In file included from /usr/src/linux-2.6/arch/x86/include/asm/uaccess.h:571,
                 from /usr/src/linux-2.6/arch/x86/include/asm/sections.h:5,
                 from /usr/src/linux-2.6/arch/x86/include/asm/hw_irq.h:26,
                 from /usr/src/linux-2.6/include/linux/irq.h:359,
                 from /usr/src/linux-2.6/arch/x86/include/asm/hardirq.h:5,
                 from /usr/src/linux-2.6/include/linux/hardirq.h:7,
                 from /usr/src/linux-2.6/include/linux/interrupt.h:12,
                 from /usr/src/linux-2.6/include/linux/usb.h:15,
                 from /usr/src/linux-2.6/drivers/hid/hid-picolcd.c:25:
In function 'copy_from_user',
inlined from 'picolcd_debug_eeprom_write' at drivers/hid/hid-picolcd.c:1592:
arch/x86/include/asm/uaccess_32.h:212: error: call to 'copy_from_user_overflow' declared with attribute error: copy_from_user() buffer size is not provably correct
gcc-4.4.5 is not able to track size calculation when it is stored into a variable, thus tell copy_from_user() maximum size via min(*max-size*, *effective-size*) explicitly and inline how much to copy at most.
Signed-off-by Bruno Prémont <[email protected]>
--
drivers/hid/hid-picolcd.c +1 -1
index b2f56a1..9d8710f 100644
@@ -1585,11 +1585,11 @@ static ssize_t picolcd_debug_eeprom_write(struct file *f, const char __user *u,
 	memset(raw_data, 0, sizeof(raw_data));
 	raw_data[0] = *off & 0xff;
 	raw_data[1] = (*off >> 8) & 0xff;
-	raw_data[2] = s < 20 ? s : 20;
+	raw_data[2] = min((size_t)20, s);
 	if (*off + raw_data[2] > 0xff)
 		raw_data[2] = 0x100 - *off;
 

Quoted 5 lines

drivers/hid/hid-picolcd.c static ssize_t picolcd_debug_eeprom_write(struct file *f, const char __user *u, · line 1592
Quoted 2 lines
drivers/hid/hid-picolcd.c +1 -1
-	if (copy_from_user(raw_data+3, u, raw_data[2]))
+	if (copy_from_user(raw_data+3, u, min((u8)20, raw_data[2])))
Hmm ... this is quite an obfuscation just for the sake of making gcc happy.

As long as CONFIG_DEBUG_STRICT_USER_COPY_CHECKS is not set it's just making gcc happy, when it is set compilation fails, which is at least annoying.

Quoted 1 line

Do other versions of gcc get this right? (i.e. is this gcc bug?)

Hm, don't remember exactly but older gcc versions triggered the same error. I didn't check with more recent version.

I can try with gcc-4.5.1/gcc-4.5.2 or gcc-4.6.0 but it will take some time to first compile those versions of gcc and then check how they handle this case.

Quoted 1 line

Don't we have similar problems all over the place in the kernel?

I have not seen any similar warnings turning up in my compiles so probably only a limited amount of places around the kernel are affected, if at all (they might have worked around as well). As my configs are rather optimized for the target machines I'm not compiling most of the drivers.
Maybe some of Ingo's/Randy's (and whoever else is building them) randconfig/allyesconfig/allmodconfig show other affected code (not sure where to find their build logs though).

I don't think that many users of copy_from_user() have variable counts of data to copy (most often a struct the sizeof which is then passed).

Bruno

Quoted 5 lines

Quoted 4 lines

drivers/hid/hid-picolcd.c +0 -0

 		return -EFAULT;
 	resp = picolcd_send_and_wait(data->hdev, REPORT_EE_WRITE, raw_data,
 			sizeof(raw_data));

Jiri Kosina

Applied

2011-05-05 11:44 UTC #3 lore raw

Quoted 68 lines · Bruno Prémont

Quoted 38 lines
Quoted 38 lines
With CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y compilation of PicoLCD driver fails on copy_from_user(), without it a warning is generated:
CC [M] drivers/hid/hid-picolcd.o
In file included from /usr/src/linux-2.6/arch/x86/include/asm/uaccess.h:571,
                 from /usr/src/linux-2.6/arch/x86/include/asm/sections.h:5,
                 from /usr/src/linux-2.6/arch/x86/include/asm/hw_irq.h:26,
                 from /usr/src/linux-2.6/include/linux/irq.h:359,
                 from /usr/src/linux-2.6/arch/x86/include/asm/hardirq.h:5,
                 from /usr/src/linux-2.6/include/linux/hardirq.h:7,
                 from /usr/src/linux-2.6/include/linux/interrupt.h:12,
                 from /usr/src/linux-2.6/include/linux/usb.h:15,
                 from /usr/src/linux-2.6/drivers/hid/hid-picolcd.c:25:
In function 'copy_from_user',
inlined from 'picolcd_debug_eeprom_write' at drivers/hid/hid-picolcd.c:1592:
arch/x86/include/asm/uaccess_32.h:212: error: call to 'copy_from_user_overflow' declared with attribute error: copy_from_user() buffer size is not provably correct
gcc-4.4.5 is not able to track size calculation when it is stored into a variable, thus tell copy_from_user() maximum size via min(*max-size*, *effective-size*) explicitly and inline how much to copy at most.
Signed-off-by Bruno Prémont <[email protected]>
--
drivers/hid/hid-picolcd.c +1 -1
index b2f56a1..9d8710f 100644
@@ -1585,11 +1585,11 @@ static ssize_t picolcd_debug_eeprom_write(struct file *f, const char __user *u,
 	memset(raw_data, 0, sizeof(raw_data));
 	raw_data[0] = *off & 0xff;
 	raw_data[1] = (*off >> 8) & 0xff;
-	raw_data[2] = s < 20 ? s : 20;
+	raw_data[2] = min((size_t)20, s);
 	if (*off + raw_data[2] > 0xff)
 		raw_data[2] = 0x100 - *off;
 
Quoted 5 lines
drivers/hid/hid-picolcd.c static ssize_t picolcd_debug_eeprom_write(struct file *f, const char __user *u, · line 1592
Quoted 2 lines
drivers/hid/hid-picolcd.c +1 -1
-	if (copy_from_user(raw_data+3, u, raw_data[2]))
+	if (copy_from_user(raw_data+3, u, min((u8)20, raw_data[2])))
Hmm ... this is quite an obfuscation just for the sake of making gcc happy.
As long as CONFIG_DEBUG_STRICT_USER_COPY_CHECKS is not set it's just making gcc happy, when it is set compilation fails, which is at least annoying.
Quoted 1 line
Do other versions of gcc get this right? (i.e. is this gcc bug?)
Hm, don't remember exactly but older gcc versions triggered the same error. I didn't check with more recent version.
I can try with gcc-4.5.1/gcc-4.5.2 or gcc-4.6.0 but it will take some time to first compile those versions of gcc and then check how they handle this case.
Quoted 1 line
Don't we have similar problems all over the place in the kernel?
I have not seen any similar warnings turning up in my compiles so probably only a limited amount of places around the kernel are affected, if at all (they might have worked around as well). As my configs are rather optimized for the target machines I'm not compiling most of the drivers.
Maybe some of Ingo's/Randy's (and whoever else is building them) randconfig/allyesconfig/allmodconfig show other affected code (not sure where to find their build logs though).

Quoted 2 lines

I don't think that many users of copy_from_user() have variable counts of data to copy (most often a struct the sizeof which is then passed).

Hmm. Not really happy that we have to do such ritual dancing just to satisfy gcc, but whatever.

Applied, thanks.

Signature · 2 lines

Jiri Kosina
SUSE Labs