| Message ID | 1361994418-1403-1-git-send-email-linux@roeck-us.net |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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.
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
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
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))
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(+)