[PATCH] netlink: don't copy over empty attribute data

[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 <***@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);
}
EXPORT_SYMBOL(__nla_put);

[David Miller]

From: Sasha Levin <***@oracle.com>
Date: Tue, 21 Oct 2014 16:51:09 -0400

Post by 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.

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.

[Sasha Levin]

Post by David Miller
Date: Tue, 21 Oct 2014 16:51:09 -0400

Post by 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.

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

[David Miller]

From: Sasha Levin <***@oracle.com>
Date: Tue, 21 Oct 2014 22:19:36 -0400

Post by Sasha Levin

Post by David Miller
Date: Tue, 21 Oct 2014 16:51:09 -0400

Post by 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.

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.

[David Laight]

From: Sasha Levin

Post by 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.
---
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

Post by Sasha Levin
}
EXPORT_SYMBOL(__nla_put);
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
More majordomo info at http://vger.kernel.org/majordomo-info.html