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