Patchwork net: af_packet: Validate parameter size for PACKET_HDRLEN control message

login
register
mail settings
Submitter Guenter Roeck
Date Feb. 27, 2013, 7:46 p.m.
Message ID <1361994418-1403-1-git-send-email-linux@roeck-us.net>
Download mbox | patch
Permalink /patch/223687/
State Rejected
Delegated to: David Miller
Headers show

Comments

Guenter Roeck - Feb. 27, 2013, 7:46 p.m.
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(+)
David Miller - Feb. 27, 2013, 8:19 p.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Borkmann - Feb. 27, 2013, 8:22 p.m.
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))
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Feb. 27, 2013, 8:26 p.m.
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.
Guenter Roeck - Feb. 27, 2013, 8:33 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Borkmann - Feb. 27, 2013, 9:18 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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