[PATCH] netlink: add buffer boundary checking

[PATCH] netlink: add buffer boundary checking

[Mark Salyzyn]

Many of the nla_get_* inlines fail to check attribute's length before
copying the content resulting in possible out-of-boundary accesses.
Adjust the inlines to perform nla_len checking, for the most part
using the nla_memcpy function to faciliate since these are not
necessarily performance critical and do not need a likely fast path.

Signed-off-by: Mark Salyzyn <salyzyn@android.com>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: kernel-team@android.com
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Thomas Graf <tgraf@suug.ch>
Fixes: bfa83a9e03cf ("[NETLINK]: Type-safe netlink messages/attributes interface")
---
 include/net/netlink.h | 66 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 54 insertions(+), 12 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index c0411f14fb53..11c0f153be7c 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -1538,7 +1538,11 @@ static inline int nla_put_bitfield32(struct sk_buff *skb, int attrtype,
  */
 static inline u32 nla_get_u32(const struct nlattr *nla)
 {
-	return *(u32 *) nla_data(nla);
+	u32 tmp;
+
+	nla_memcpy(&tmp, nla, sizeof(tmp));
+
+	return tmp;
 }
 
 /**
@@ -1547,7 +1551,11 @@ static inline u32 nla_get_u32(const struct nlattr *nla)
  */
 static inline __be32 nla_get_be32(const struct nlattr *nla)
 {
-	return *(__be32 *) nla_data(nla);
+	__be32 tmp;
+
+	nla_memcpy(&tmp, nla, sizeof(tmp));
+
+	return tmp;
 }
 
 /**
@@ -1556,7 +1564,11 @@ static inline __be32 nla_get_be32(const struct nlattr *nla)
  */
 static inline __le32 nla_get_le32(const struct nlattr *nla)
 {
-	return *(__le32 *) nla_data(nla);
+	__le32 tmp;
+
+	nla_memcpy(&tmp, nla, sizeof(tmp));
+
+	return tmp;
 }
 
 /**
@@ -1565,7 +1577,11 @@ static inline __le32 nla_get_le32(const struct nlattr *nla)
  */
 static inline u16 nla_get_u16(const struct nlattr *nla)
 {
-	return *(u16 *) nla_data(nla);
+	u16 tmp;
+
+	nla_memcpy(&tmp, nla, sizeof(tmp));
+
+	return tmp;
 }
 
 /**
@@ -1574,7 +1590,11 @@ static inline u16 nla_get_u16(const struct nlattr *nla)
  */
 static inline __be16 nla_get_be16(const struct nlattr *nla)
 {
-	return *(__be16 *) nla_data(nla);
+	__be16 tmp;
+
+	nla_memcpy(&tmp, nla, sizeof(tmp));
+
+	return tmp;
 }
 
 /**
@@ -1583,7 +1603,11 @@ static inline __be16 nla_get_be16(const struct nlattr *nla)
  */
 static inline __le16 nla_get_le16(const struct nlattr *nla)
 {
-	return *(__le16 *) nla_data(nla);
+	__le16 tmp;
+
+	nla_memcpy(&tmp, nla, sizeof(tmp));
+
+	return tmp;
 }
 
 /**
@@ -1592,7 +1616,7 @@ static inline __le16 nla_get_le16(const struct nlattr *nla)
  */
 static inline u8 nla_get_u8(const struct nlattr *nla)
 {
-	return *(u8 *) nla_data(nla);
+	return (nla_len(nla) >= sizeof(u8)) ? *(u8 *) nla_data(nla) : 0;
 }
 
 /**
@@ -1627,7 +1651,11 @@ static inline __be64 nla_get_be64(const struct nlattr *nla)
  */
 static inline __le64 nla_get_le64(const struct nlattr *nla)
 {
-	return *(__le64 *) nla_data(nla);
+	__le64 tmp;
+
+	nla_memcpy(&tmp, nla, sizeof(tmp));
+
+	return tmp;
 }
 
 /**
@@ -1636,7 +1664,11 @@ static inline __le64 nla_get_le64(const struct nlattr *nla)
  */
 static inline s32 nla_get_s32(const struct nlattr *nla)
 {
-	return *(s32 *) nla_data(nla);
+	s32 tmp;
+
+	nla_memcpy(&tmp, nla, sizeof(tmp));
+
+	return tmp;
 }
 
 /**
@@ -1645,7 +1677,11 @@ static inline s32 nla_get_s32(const struct nlattr *nla)
  */
 static inline s16 nla_get_s16(const struct nlattr *nla)
 {
-	return *(s16 *) nla_data(nla);
+	s16 tmp;
+
+	nla_memcpy(&tmp, nla, sizeof(tmp));
+
+	return tmp;
 }
 
 /**
@@ -1654,7 +1690,7 @@ static inline s16 nla_get_s16(const struct nlattr *nla)
  */
 static inline s8 nla_get_s8(const struct nlattr *nla)
 {
-	return *(s8 *) nla_data(nla);
+	return (nla_len(nla) >= sizeof(s8)) ? *(s8 *) nla_data(nla) : 0;
 }
 
 /**
@@ -1698,7 +1734,11 @@ static inline unsigned long nla_get_msecs(const struct nlattr *nla)
  */
 static inline __be32 nla_get_in_addr(const struct nlattr *nla)
 {
-	return *(__be32 *) nla_data(nla);
+	__be32 tmp;
+
+	nla_memcpy(&tmp, nla, sizeof(tmp));
+
+	return tmp;
 }
 
 /**
@@ -1710,6 +1750,7 @@ static inline struct in6_addr nla_get_in6_addr(const struct nlattr *nla)
 	struct in6_addr tmp;
 
 	nla_memcpy(&tmp, nla, sizeof(tmp));
+
 	return tmp;
 }
 
@@ -1722,6 +1763,7 @@ static inline struct nla_bitfield32 nla_get_bitfield32(const struct nlattr *nla)
 	struct nla_bitfield32 tmp;
 
 	nla_memcpy(&tmp, nla, sizeof(tmp));
+
 	return tmp;
 }
 
-- 
2.28.0.rc0.105.gf9edc3c819-goog

Re: [PATCH] netlink: add buffer boundary checking

[David Miller]

From: Mark Salyzyn <salyzyn@android.com>
Date: Thu, 23 Jul 2020 11:21:32 -0700

> Many of the nla_get_* inlines fail to check attribute's length before
> copying the content resulting in possible out-of-boundary accesses.
> Adjust the inlines to perform nla_len checking, for the most part
> using the nla_memcpy function to faciliate since these are not
> necessarily performance critical and do not need a likely fast path.
> 
> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: kernel-team@android.com
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Thomas Graf <tgraf@suug.ch>
> Fixes: bfa83a9e03cf ("[NETLINK]: Type-safe netlink messages/attributes interface")

Please, let's avoid stuff like this.

Now it is going to be expensive to move several small attributes,
which is common.  And there's a multiplier when dumping, for example,
thousands of networking devices, routes, or whatever, and all of their
attributes in a dump.

If you can document actual out of bounds accesses, let's fix them.  Usually
contextually the attribute type and size has been validated by the time we
execute these accessors.

I'm not applying this, sorry.

Re: [PATCH] netlink: add buffer boundary checking

[Eric Dumazet]

On 7/23/20 11:21 AM, Mark Salyzyn wrote:
> Many of the nla_get_* inlines fail to check attribute's length before
> copying the content resulting in possible out-of-boundary accesses.
> Adjust the inlines to perform nla_len checking, for the most part
> using the nla_memcpy function to faciliate since these are not
> necessarily performance critical and do not need a likely fast path.
> 
> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: kernel-team@android.com
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Thomas Graf <tgraf@suug.ch>
> Fixes: bfa83a9e03cf ("[NETLINK]: Type-safe netlink messages/attributes interface")
> ---
>  include/net/netlink.h | 66 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 54 insertions(+), 12 deletions(-)
> 
> diff --git a/include/net/netlink.h b/include/net/netlink.h
> index c0411f14fb53..11c0f153be7c 100644
> --- a/include/net/netlink.h
> +++ b/include/net/netlink.h
> @@ -1538,7 +1538,11 @@ static inline int nla_put_bitfield32(struct sk_buff *skb, int attrtype,
>   */
>  static inline u32 nla_get_u32(const struct nlattr *nla)
>  {
> -	return *(u32 *) nla_data(nla);
> +	u32 tmp;
> +
> +	nla_memcpy(&tmp, nla, sizeof(tmp));
> +
> +	return tmp;

I believe this will hide bugs, that syzbot was able to catch.

Instead, you could perhaps introduce a CONFIG_DEBUG_NETLINK option,
and add a WARN_ON_ONCE(nla_len(nla) < sizeof(u32)) so that we can detect bugs in callers.

Re: [PATCH] netlink: add buffer boundary checking

[Mark Salyzyn]

On 7/23/20 12:35 PM, Eric Dumazet wrote:
> I believe this will hide bugs, that syzbot was able to catch.

syzbot failed to catch the problem because of padding u8, u16 and u32 
were all immune because they would go out of bounds into a padded buffer :-(

On 7/23/20 12:19 PM, David Miller wrote:
> Now it is going to be expensive to move several small attributes,
> which is common.  And there's a multiplier when dumping, for example,
> thousands of networking devices, routes, or whatever, and all of their
> attributes in a dump.
So it _is_ performance critical (!)
> If you can document actual out of bounds accesses, let's fix them.  Usually
> contextually the attribute type and size has been validated by the time we
> execute these accessors.

A PoC was written for nl80211_tdls_mgmt supplied no bytes for 
NL80211_ATTR_STATUS_CODE and instrumented code could report back OOB.

I was initially considering pushback because padding bytes were being 
read and considered it harmless. Best way to find out if it is really a 
problem probably was to ask, but as Linus said once 'show me the code' 
and that is just as effective in the asking ;->

My Gut remains to respond WAI unless you'all reading padding is 'bad'

-- Mark

Re: [PATCH] netlink: add buffer boundary checking

[Jacob Keller]

On 7/23/2020 12:35 PM, Eric Dumazet wrote:
> On 7/23/20 11:21 AM, Mark Salyzyn wrote:
>> Many of the nla_get_* inlines fail to check attribute's length before
>> copying the content resulting in possible out-of-boundary accesses.
>> Adjust the inlines to perform nla_len checking, for the most part
>> using the nla_memcpy function to faciliate since these are not
>> necessarily performance critical and do not need a likely fast path.
>>
>> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
>> Cc: netdev@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: kernel-team@android.com
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: Jakub Kicinski <kuba@kernel.org>
>> Cc: Thomas Graf <tgraf@suug.ch>
>> Fixes: bfa83a9e03cf ("[NETLINK]: Type-safe netlink messages/attributes interface")
>> ---
>>  include/net/netlink.h | 66 +++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 54 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/net/netlink.h b/include/net/netlink.h
>> index c0411f14fb53..11c0f153be7c 100644
>> --- a/include/net/netlink.h
>> +++ b/include/net/netlink.h
>> @@ -1538,7 +1538,11 @@ static inline int nla_put_bitfield32(struct sk_buff *skb, int attrtype,
>>   */
>>  static inline u32 nla_get_u32(const struct nlattr *nla)
>>  {
>> -	return *(u32 *) nla_data(nla);
>> +	u32 tmp;
>> +
>> +	nla_memcpy(&tmp, nla, sizeof(tmp));
>> +
>> +	return tmp;
> 
> I believe this will hide bugs, that syzbot was able to catch.
> 
> Instead, you could perhaps introduce a CONFIG_DEBUG_NETLINK option,
> and add a WARN_ON_ONCE(nla_len(nla) < sizeof(u32)) so that we can detect bugs in callers.
> 
> 

I also think this is a better approach.

Re: [PATCH] netlink: add buffer boundary checking

[Mark Salyzyn]

On 7/24/20 2:14 PM, Jacob Keller wrote:
>
> On 7/23/2020 12:35 PM, Eric Dumazet wrote:
>> On 7/23/20 11:21 AM, Mark Salyzyn wrote:
>>> Many of the nla_get_* inlines fail to check attribute's length before
>>> copying the content resulting in possible out-of-boundary accesses.
>>> Adjust the inlines to perform nla_len checking, for the most part
>>> using the nla_memcpy function to faciliate since these are not
>>> necessarily performance critical and do not need a likely fast path.
>>>
>>> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
>>> Cc: netdev@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Cc: kernel-team@android.com
>>> Cc: "David S. Miller" <davem@davemloft.net>
>>> Cc: Jakub Kicinski <kuba@kernel.org>
>>> Cc: Thomas Graf <tgraf@suug.ch>
>>> Fixes: bfa83a9e03cf ("[NETLINK]: Type-safe netlink messages/attributes interface")
>>> ---
>>>   include/net/netlink.h | 66 +++++++++++++++++++++++++++++++++++--------
>>>   1 file changed, 54 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/include/net/netlink.h b/include/net/netlink.h
>>> index c0411f14fb53..11c0f153be7c 100644
>>> --- a/include/net/netlink.h
>>> +++ b/include/net/netlink.h
>>> @@ -1538,7 +1538,11 @@ static inline int nla_put_bitfield32(struct sk_buff *skb, int attrtype,
>>>    */
>>>   static inline u32 nla_get_u32(const struct nlattr *nla)
>>>   {
>>> -	return *(u32 *) nla_data(nla);
>>> +	u32 tmp;
>>> +
>>> +	nla_memcpy(&tmp, nla, sizeof(tmp));
>>> +
>>> +	return tmp;
>> I believe this will hide bugs, that syzbot was able to catch.
>>
>> Instead, you could perhaps introduce a CONFIG_DEBUG_NETLINK option,
>> and add a WARN_ON_ONCE(nla_len(nla) < sizeof(u32)) so that we can detect bugs in callers.
>>
>>
> I also think this is a better approach.

We (another engineer here) are looking into that and will get back to 
everyone.

Sincerely -- Mark Salyzyn