| Message ID | 1413924669-26732-1-git-send-email-sasha.levin@oracle.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Sasha Levin <sasha.levin@oracle.com> Date: Tue, 21 Oct 2014 16:51:09 -0400 > netlink uses empty data to seperate different levels. However, we still > try to copy that data from a NULL ptr using memcpy, which is an undefined > behaviour. > > Signed-off-by: Sasha Levin <sasha.levin@oracle.com> This isn't a POSIX C library, this it the Linux kernel, and as such we can make sure none of our memcpy() implementations try to access any bytes if the given length is NULL. And to be quite honest, there is no benefit whatsoever nor even the possibility of using that "undefined behavior" flexibility to do anthing. This is because every memcpy() implementation must be sure not to access past the end of either source or destination buffer. And the one and only way to do that is to respect the length. I'm not applying this, because the basis for which this is claimed to be a bug fix is quite bogus in my opinion. Sorry. -- 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 10/21/2014 09:39 PM, David Miller wrote: > From: Sasha Levin <sasha.levin@oracle.com> > Date: Tue, 21 Oct 2014 16:51:09 -0400 > >> > netlink uses empty data to seperate different levels. However, we still >> > try to copy that data from a NULL ptr using memcpy, which is an undefined >> > behaviour. >> > >> > Signed-off-by: Sasha Levin <sasha.levin@oracle.com> > This isn't a POSIX C library, this it the Linux kernel, and as such > we can make sure none of our memcpy() implementations try to access > any bytes if the given length is NULL. We can make *our* implementations work around that undefined behaviour if we want, but right now our implementations is to call GCC's builtin memcpy(), which follows the standards and doesn't allow you to call it with NULL 'from' ptr. The fact that it doesn't die and behaves properly is just "luck". Thanks, Sasha -- 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: Sasha Levin <sasha.levin@oracle.com> Date: Tue, 21 Oct 2014 22:19:36 -0400 > On 10/21/2014 09:39 PM, David Miller wrote: >> From: Sasha Levin <sasha.levin@oracle.com> >> Date: Tue, 21 Oct 2014 16:51:09 -0400 >> >>> > netlink uses empty data to seperate different levels. However, we still >>> > try to copy that data from a NULL ptr using memcpy, which is an undefined >>> > behaviour. >>> > >>> > Signed-off-by: Sasha Levin <sasha.levin@oracle.com> >> This isn't a POSIX C library, this it the Linux kernel, and as such >> we can make sure none of our memcpy() implementations try to access >> any bytes if the given length is NULL. > > We can make *our* implementations work around that undefined behaviour if we > want, but right now our implementations is to call GCC's builtin memcpy(), > which follows the standards and doesn't allow you to call it with NULL 'from' > ptr. > > The fact that it doesn't die and behaves properly is just "luck". If GCC's internal memcpy() starts accessing past 'len', I'm going to report the bug rather than code around it. That causes a page fault, you _can't_ do it. -- 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: Sasha Levin > netlink uses empty data to seperate different levels. However, we still > try to copy that data from a NULL ptr using memcpy, which is an undefined > behaviour. > > Signed-off-by: Sasha Levin <sasha.levin@oracle.com> > --- > lib/nlattr.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/lib/nlattr.c b/lib/nlattr.c > index 9c3e85f..6512b43 100644 > --- a/lib/nlattr.c > +++ b/lib/nlattr.c > @@ -430,7 +430,8 @@ void __nla_put(struct sk_buff *skb, int attrtype, int attrlen, > struct nlattr *nla; > > nla = __nla_reserve(skb, attrtype, attrlen); > - memcpy(nla_data(nla), data, attrlen); > + if (data) > + memcpy(nla_data(nla), data, attrlen); Were it even appropriate to add a conditional here, the correct check would be against 'attrlen', not 'data'. David > } > EXPORT_SYMBOL(__nla_put); > > -- > 1.7.10.4 > > -- > 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 -- 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 10/22/2014 02:15 AM, David Miller wrote: > From: Sasha Levin <sasha.levin@oracle.com> > Date: Tue, 21 Oct 2014 22:19:36 -0400 > >> On 10/21/2014 09:39 PM, David Miller wrote: >>> From: Sasha Levin <sasha.levin@oracle.com> >>> Date: Tue, 21 Oct 2014 16:51:09 -0400 >>> >>>>> netlink uses empty data to seperate different levels. However, we still >>>>> try to copy that data from a NULL ptr using memcpy, which is an undefined >>>>> behaviour. >>>>> >>>>> Signed-off-by: Sasha Levin <sasha.levin@oracle.com> >>> This isn't a POSIX C library, this it the Linux kernel, and as such >>> we can make sure none of our memcpy() implementations try to access >>> any bytes if the given length is NULL. >> >> We can make *our* implementations work around that undefined behaviour if we >> want, but right now our implementations is to call GCC's builtin memcpy(), >> which follows the standards and doesn't allow you to call it with NULL 'from' >> ptr. >> >> The fact that it doesn't die and behaves properly is just "luck". > > If GCC's internal memcpy() starts accessing past 'len', I'm going > to report the bug rather than code around it. How so? GCC states clearly that you should *never* pass a NULL pointer there: "The pointers passed to memmove (and similar functions in <string.h>) must be non-null even when nbytes==0" (https://gcc.gnu.org/gcc-4.9/porting_to.html). Even if it doesn't dereference it, it can break somehow in a subtle way. Leaving the kernel code assuming that gcc (or any other compiler) would always behave the same in a situation that shouldn't occur. Thanks, Sasha -- 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: Sasha Levin <sasha.levin@oracle.com> Date: Sun, 26 Oct 2014 19:32:42 -0400 > How so? GCC states clearly that you should *never* pass a NULL > pointer there: > > "The pointers passed to memmove (and similar functions in <string.h>) must > be non-null even when nbytes==0" (https://gcc.gnu.org/gcc-4.9/porting_to.html). > > Even if it doesn't dereference it, it can break somehow in a subtle way. Leaving > the kernel code assuming that gcc (or any other compiler) would always behave > the same in a situation that shouldn't occur. Show me a legal way in which one could legally dereference the pointer when length is zero, and I'll entertain this patch. -- 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 10/26/2014 10:03 PM, David Miller wrote: > From: Sasha Levin <sasha.levin@oracle.com> > Date: Sun, 26 Oct 2014 19:32:42 -0400 > >> How so? GCC states clearly that you should *never* pass a NULL >> pointer there: >> >> "The pointers passed to memmove (and similar functions in <string.h>) must >> be non-null even when nbytes==0" (https://gcc.gnu.org/gcc-4.9/porting_to.html). >> >> Even if it doesn't dereference it, it can break somehow in a subtle way. Leaving >> the kernel code assuming that gcc (or any other compiler) would always behave >> the same in a situation that shouldn't occur. > > Show me a legal way in which one could legally dereference the pointer > when length is zero, and I'll entertain this patch. The moment you've triggered an undefined behaviour you have GCC license to dereference anything it wants. GCC would be well within it's rights dereferencing a NULL "from". They even state it clearly in that GCC 4.9 porting guide I've linked above: """ Calling copy(p, NULL, 0) can therefore deference a null pointer and crash. The example above needs to be fixed to avoid the invalid memmove call, for example: if (nbytes != 0) memmove (dest, src, nbytes); """ Thanks, Sasha -- 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 10/27/2014 05:42 PM, Sasha Levin wrote: > On 10/26/2014 10:03 PM, David Miller wrote: >> From: Sasha Levin <sasha.levin@oracle.com> >> Date: Sun, 26 Oct 2014 19:32:42 -0400 >> >>> How so? GCC states clearly that you should *never* pass a NULL >>> pointer there: >>> >>> "The pointers passed to memmove (and similar functions in <string.h>) must >>> be non-null even when nbytes==0" (https://gcc.gnu.org/gcc-4.9/porting_to.html). >>> >>> Even if it doesn't dereference it, it can break somehow in a subtle way. Leaving >>> the kernel code assuming that gcc (or any other compiler) would always behave >>> the same in a situation that shouldn't occur. >> >> Show me a legal way in which one could legally dereference the pointer >> when length is zero, and I'll entertain this patch. > > The moment you've triggered an undefined behaviour you have GCC license to > dereference anything it wants. GCC would be well within it's rights > dereferencing a NULL "from". > > They even state it clearly in that GCC 4.9 porting guide I've linked above: > > """ > Calling copy(p, NULL, 0) can therefore deference a null pointer and crash. > > The example above needs to be fixed to avoid the invalid memmove call, for example: > > > if (nbytes != 0) > memmove (dest, src, nbytes); > """ > In example from link null ptr deref could happen because GCC will optimize away null pointer check after memmove(): int copy (int* dest, int* src, size_t nbytes) { memmove (dest, src, nbytes); if (src != NULL) <---- GCC will eliminate this check because src can't be null. return *src; <-- NULL ptr deref return 0; } Even though GCC and C standard treats such code ( memmove(dest, NULL, 0); ) as invalid, it probably will not crash in linux kernel case, because that kind of optimization disabled via -fno-delete-null-pointer-checks option. > > Thanks, > Sasha > -- 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/lib/nlattr.c b/lib/nlattr.c index 9c3e85f..6512b43 100644 --- a/lib/nlattr.c +++ b/lib/nlattr.c @@ -430,7 +430,8 @@ void __nla_put(struct sk_buff *skb, int attrtype, int attrlen, struct nlattr *nla; nla = __nla_reserve(skb, attrtype, attrlen); - memcpy(nla_data(nla), data, attrlen); + if (data) + memcpy(nla_data(nla), data, attrlen); } EXPORT_SYMBOL(__nla_put);
netlink uses empty data to seperate different levels. However, we still try to copy that data from a NULL ptr using memcpy, which is an undefined behaviour. Signed-off-by: Sasha Levin <sasha.levin@oracle.com> --- lib/nlattr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)