netdev - Re: [PATCH net] net: zero-initialize skb extensions on allocation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ygnhy2c48jeb.fsf@nvidia.com>
Date:   Mon, 24 May 2021 14:23:56 +0300
From:   Vlad Buslov <vladbu@...dia.com>
To:     Florian Westphal <fw@...len.de>
CC:     <davem@...emloft.net>, <kuba@...nel.org>, <pablo@...filter.org>,
        <mathew.j.martineau@...ux.intel.com>,
        <matthieu.baerts@...sares.net>, <steffen.klassert@...unet.com>,
        <herbert@...dor.apana.org.au>, <saeedm@...lanox.com>,
        <netdev@...r.kernel.org>
Subject: Re: [PATCH net] net: zero-initialize skb extensions on allocation

On Mon 24 May 2021 at 13:01, Florian Westphal <fw@...len.de> wrote:
> Vlad Buslov <vladbu@...dia.com> wrote:
>> Function skb_ext_add() doesn't initialize created skb extension with any
>> value and leaves it up to the user.
>
> That was intentional.
>
> Its unlikely that all extensions are active at the same time on same skb.
>
> This is also the reason why the extension struct uses offset addressing
> to get the extension data rather than the simpler
>
> skb_ext {
> 	struct sec_path sp;
> 	struct nf_bridge_info nfbr;
> 	...
> }
>
> So adding e.g. mptcp extension will only touch 1 cacheline instead of 3
> (or more if more extensions get added in the future).
>
> IOW, i would prefer if tc would add tc_skb_add_ext() or similar and
> zero whats needed there.

Got it. Thanks for the explanation!

>
>> Fix the issue by changing __skb_ext_alloc() function to request
>> zero-initialized memory from kmem cache. Note that skb extension allocation
>> in skb_ext_maybe_cow() is not changed because newly allocated memory is
>> immediately overwritten with content of old skb extension so there is no
>> need to pre-initialize it.
>>
>> Multiple users of skb extension API have already been manually setting
>> newly allocated skb extension memory to zero. Remove such code and rely on
>> skb extension API instead.
>
> Are you sure its safe?

Apparently it is not, since you found a problem with nf_bridge_alloc :)

>
>>  static inline struct nf_bridge_info *nf_bridge_alloc(struct sk_buff *skb)
>>  {
>>  #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
>> -	struct nf_bridge_info *b = skb_ext_add(skb, SKB_EXT_BRIDGE_NF);
>> -
>> -	if (b)
>> -		memset(b, 0, sizeof(*b));
>> -
>> -	return b;
>> +	return skb_ext_add(skb, SKB_EXT_BRIDGE_NF);
>
> So in the (unlikely) case where skb_ext_add did not allocate a new
> extension, the memory is no longer cleared.
>
> If the skb had an nf_bridge_info extension previously that got
> discarded earlier via skb_ext_del() this now leaks the old content.

So what would you suggest: provide a dedicated wrapper for TC skb
extension that will memset resulting extension to zero or refactor my
patch to zero-initialize specific skb extension in skb_ext_add() (only
the extension requested and also when previously discarded extension is
reused)?