[hid-picolcd] Avoid compile warning/error triggered by copy_from_user()

May 04th, 2011 - 03:10 pm ET by Bruno Prémont | Report spam
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 <bonbons@linux-vserver.org>

diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
index b2f56a1..9d8710f 100644
a/drivers/hid/hid-picolcd.c
+++ b/drivers/hid/hid-picolcd.c
@@ -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));
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
email Follow the discussionReplies 3 repliesReplies Make a reply

Similar topics

Replies

#1 Jiri Kosina
May 05th, 2011 - 05:40 am ET | Report spam
On Wed, 4 May 2011, Bruno Prémont wrote:

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

diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
index b2f56a1..9d8710f 100644
a/drivers/hid/hid-picolcd.c
+++ b/drivers/hid/hid-picolcd.c
@@ -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?

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




Jiri Kosina
SUSE Labs
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Replies Reply to this message
#2 Bruno Prémont
May 05th, 2011 - 06:10 am ET | Report spam
Hi Jiri,

On Thu, 5 May 2011 11:27:54 wrote:
On Wed, 4 May 2011, Bruno Prémont wrote:

> 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
>
> diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
> index b2f56a1..9d8710f 100644
> a/drivers/hid/hid-picolcd.c
> +++ b/drivers/hid/hid-picolcd.c
> @@ -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.



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.

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.

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


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



To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Replies Reply to this message
#3 Jiri Kosina
May 05th, 2011 - 07:50 am ET | Report spam
On Thu, 5 May 2011, Bruno Prémont wrote:

> > 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
> >
> > diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
> > index b2f56a1..9d8710f 100644
> > a/drivers/hid/hid-picolcd.c
> > +++ b/drivers/hid/hid-picolcd.c
> > @@ -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.

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.

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

> 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).



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

Applied, thanks.

Jiri Kosina
SUSE Labs
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
email Follow the discussion Replies Reply to this message
Help Create a new topicReplies Make a reply
Search Make your own search