* [PATCH] net: af_packet: Validate parameter size for PACKET_HDRLEN control message
@ 2013-02-27 19:46 Guenter Roeck
2013-02-27 20:19 ` David Miller
2013-02-27 20:22 ` Daniel Borkmann
0 siblings, 2 replies; 6+ messages in thread
From: Guenter Roeck @ 2013-02-27 19:46 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Guenter Roeck
Building af_packet may fail with
In function ‘copy_from_user’,
inlined from ‘packet_getsockopt’ at
net/packet/af_packet.c:3215:21:
arch/x86/include/asm/uaccess_32.h:211:26: error: call to
‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
buffer size is not provably correct
if built with W=1 due to a missing parameter size validation.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
net/packet/af_packet.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index c7bfeff..1976b23 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -3210,6 +3210,8 @@ static int packet_getsockopt(struct socket *sock, int level, int optname,
val = po->tp_version;
break;
case PACKET_HDRLEN:
+ if (len < sizeof(int))
+ return -EINVAL;
if (len > sizeof(int))
len = sizeof(int);
if (copy_from_user(&val, optval, len))
--
1.7.9.7
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] net: af_packet: Validate parameter size for PACKET_HDRLEN control message
2013-02-27 19:46 [PATCH] net: af_packet: Validate parameter size for PACKET_HDRLEN control message Guenter Roeck
@ 2013-02-27 20:19 ` David Miller
2013-02-27 20:22 ` Daniel Borkmann
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2013-02-27 20:19 UTC (permalink / raw)
To: linux; +Cc: netdev
The first thing this function does is test whether len < 0, therefore
your change is unnecessary.
If the user gives us something between 0 and sizeof(int), that's
their problem, and they'll get a partial int copied back into
userspace as a result instead of the complete integer.
Please don't blindly silence warnings like this, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: af_packet: Validate parameter size for PACKET_HDRLEN control message
2013-02-27 19:46 [PATCH] net: af_packet: Validate parameter size for PACKET_HDRLEN control message Guenter Roeck
2013-02-27 20:19 ` David Miller
@ 2013-02-27 20:22 ` Daniel Borkmann
2013-02-27 20:26 ` David Miller
1 sibling, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2013-02-27 20:22 UTC (permalink / raw)
To: Guenter Roeck; +Cc: netdev, David S. Miller
On 02/27/2013 08:46 PM, Guenter Roeck wrote:
> Building af_packet may fail with
>
> In function ‘copy_from_user’,
> inlined from ‘packet_getsockopt’ at
> net/packet/af_packet.c:3215:21:
> arch/x86/include/asm/uaccess_32.h:211:26: error: call to
> ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
> buffer size is not provably correct
>
> if built with W=1 due to a missing parameter size validation.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> net/packet/af_packet.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index c7bfeff..1976b23 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -3210,6 +3210,8 @@ static int packet_getsockopt(struct socket *sock, int level, int optname,
> val = po->tp_version;
> break;
> case PACKET_HDRLEN:
> + if (len < sizeof(int))
> + return -EINVAL;
I think this could break some user space applications here, those who e.g. only pass
an uint16_t to packet_getsockopt with PACKET_HDRLEN.
> if (len > sizeof(int))
> len = sizeof(int);
> if (copy_from_user(&val, optval, len))
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: af_packet: Validate parameter size for PACKET_HDRLEN control message
2013-02-27 20:22 ` Daniel Borkmann
@ 2013-02-27 20:26 ` David Miller
2013-02-27 20:33 ` Guenter Roeck
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2013-02-27 20:26 UTC (permalink / raw)
To: dborkman; +Cc: linux, netdev
From: Daniel Borkmann <dborkman@redhat.com>
Date: Wed, 27 Feb 2013 21:22:17 +0100
> On 02/27/2013 08:46 PM, Guenter Roeck wrote:
>> Building af_packet may fail with
>>
>> In function ‘copy_from_user’,
>> inlined from ‘packet_getsockopt’ at
>> net/packet/af_packet.c:3215:21:
>> arch/x86/include/asm/uaccess_32.h:211:26: error: call to
>> ‘copy_from_user_overflow’ declared with attribute error:
>> copy_from_user()
>> buffer size is not provably correct
>>
>> if built with W=1 due to a missing parameter size validation.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> net/packet/af_packet.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
>> index c7bfeff..1976b23 100644
>> --- a/net/packet/af_packet.c
>> +++ b/net/packet/af_packet.c
>> @@ -3210,6 +3210,8 @@ static int packet_getsockopt(struct socket
>> *sock, int level, int optname,
>> val = po->tp_version;
>> break;
>> case PACKET_HDRLEN:
>> + if (len < sizeof(int))
>> + return -EINVAL;
>
> I think this could break some user space applications here, those who
> e.g. only pass
> an uint16_t to packet_getsockopt with PACKET_HDRLEN.
Well, their shit is broken on big endian then.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: af_packet: Validate parameter size for PACKET_HDRLEN control message
2013-02-27 20:26 ` David Miller
@ 2013-02-27 20:33 ` Guenter Roeck
2013-02-27 21:18 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2013-02-27 20:33 UTC (permalink / raw)
To: David Miller; +Cc: dborkman, netdev
On Wed, Feb 27, 2013 at 03:26:30PM -0500, David Miller wrote:
> From: Daniel Borkmann <dborkman@redhat.com>
> Date: Wed, 27 Feb 2013 21:22:17 +0100
>
> > On 02/27/2013 08:46 PM, Guenter Roeck wrote:
> >> Building af_packet may fail with
> >>
> >> In function ‘copy_from_user’,
> >> inlined from ‘packet_getsockopt’ at
> >> net/packet/af_packet.c:3215:21:
> >> arch/x86/include/asm/uaccess_32.h:211:26: error: call to
> >> ‘copy_from_user_overflow’ declared with attribute error:
> >> copy_from_user()
> >> buffer size is not provably correct
> >>
> >> if built with W=1 due to a missing parameter size validation.
> >>
> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >> ---
> >> net/packet/af_packet.c | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> >> index c7bfeff..1976b23 100644
> >> --- a/net/packet/af_packet.c
> >> +++ b/net/packet/af_packet.c
> >> @@ -3210,6 +3210,8 @@ static int packet_getsockopt(struct socket
> >> *sock, int level, int optname,
> >> val = po->tp_version;
> >> break;
> >> case PACKET_HDRLEN:
> >> + if (len < sizeof(int))
> >> + return -EINVAL;
> >
> > I think this could break some user space applications here, those who
> > e.g. only pass
> > an uint16_t to packet_getsockopt with PACKET_HDRLEN.
>
> Well, their shit is broken on big endian then.
There must be something else going on anyway ... yes, my patch fixes the
warning/error, but copy_from_user should only bail out if the copy size
can be larger than the provided buffer (unless I misunderstand the code
in copy_from_user). And the second check should take care of that.
Anyway, point taken. I'll waste my time elsewhere :).
Thanks,
Guenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: af_packet: Validate parameter size for PACKET_HDRLEN control message
2013-02-27 20:33 ` Guenter Roeck
@ 2013-02-27 21:18 ` Daniel Borkmann
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2013-02-27 21:18 UTC (permalink / raw)
To: Guenter Roeck; +Cc: David Miller, netdev
On 02/27/2013 09:33 PM, Guenter Roeck wrote:
> On Wed, Feb 27, 2013 at 03:26:30PM -0500, David Miller wrote:
>> From: Daniel Borkmann <dborkman@redhat.com>
>> Date: Wed, 27 Feb 2013 21:22:17 +0100
>>
>>> On 02/27/2013 08:46 PM, Guenter Roeck wrote:
>>>> Building af_packet may fail with
>>>>
>>>> In function ‘copy_from_user’,
>>>> inlined from ‘packet_getsockopt’ at
>>>> net/packet/af_packet.c:3215:21:
>>>> arch/x86/include/asm/uaccess_32.h:211:26: error: call to
>>>> ‘copy_from_user_overflow’ declared with attribute error:
>>>> copy_from_user()
>>>> buffer size is not provably correct
>>>>
>>>> if built with W=1 due to a missing parameter size validation.
>>>>
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>> ---
>>>> net/packet/af_packet.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
>>>> index c7bfeff..1976b23 100644
>>>> --- a/net/packet/af_packet.c
>>>> +++ b/net/packet/af_packet.c
>>>> @@ -3210,6 +3210,8 @@ static int packet_getsockopt(struct socket
>>>> *sock, int level, int optname,
>>>> val = po->tp_version;
>>>> break;
>>>> case PACKET_HDRLEN:
>>>> + if (len < sizeof(int))
>>>> + return -EINVAL;
>>>
>>> I think this could break some user space applications here, those who
>>> e.g. only pass
>>> an uint16_t to packet_getsockopt with PACKET_HDRLEN.
>>
>> Well, their shit is broken on big endian then.
>
> There must be something else going on anyway ... yes, my patch fixes the
> warning/error, but copy_from_user should only bail out if the copy size
> can be larger than the provided buffer (unless I misunderstand the code
> in copy_from_user). And the second check should take care of that.
Fair enough, from what I read the implementation on x86_64 uses gcc's
__builtin_object_size(<X>, 0) [1]. Since the <to> (<X>) argument is known
at compile time (val:int), __builtin_object_size() will return sizeof(int)-1,
the number of bytes from val start to the end of the object val pointer
points to. Since our length that we pass can be [0, sizeof(int)] the
compiler cannot prove it, if the copy_from_user() buffer size is correct.
Thus, "buffer size is not provably correct". Applications not passing int
to this getsockopt(2) are screwed up then anyway.
[1] http://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-02-27 21:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-02-27 19:46 [PATCH] net: af_packet: Validate parameter size for PACKET_HDRLEN control message Guenter Roeck
2013-02-27 20:19 ` David Miller
2013-02-27 20:22 ` Daniel Borkmann
2013-02-27 20:26 ` David Miller
2013-02-27 20:33 ` Guenter Roeck
2013-02-27 21:18 ` Daniel Borkmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).