Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: 0djh0dCjjpd5F+KtjQWdBV2uk3/gIMmff24yuAQk/zhTrEtLwByv7CsQYkALQNpkudtguEW737
 m2cNwOIMKYaw==
IronPort-SDR: sATOhYz40y+cGfnFT7Xq9uZ7pU51qOr3ByTfyP0CDW+NLGVwYqP0LgB5G6AQIpMkH1P0K31VB3
 MXJNmRB4VsVw==
Subject: Re: [PATCH] netlink: add buffer boundary checking
To:     Eric Dumazet <eric.dumazet@gmail.com>,
        Mark Salyzyn <salyzyn@android.com>,
        linux-kernel@vger.kernel.org
Cc:     kernel-team@android.com, netdev@vger.kernel.org,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, Thomas Graf <tgraf@suug.ch>
References: <20200723182136.2550163-1-salyzyn@android.com>
 <09cd1829-8e41-bef5-ba5e-1c446c166778@gmail.com>
From:   Jacob Keller <jacob.e.keller@intel.com>
Organization: Intel Corporation
Message-ID: <8bd7695c-0012-83e9-8a5a-94a40d91d6f6@intel.com>
Date:   Fri, 24 Jul 2020 14:14:25 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.0.1
MIME-Version: 1.0
In-Reply-To: <09cd1829-8e41-bef5-ba5e-1c446c166778@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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.