netdev - Re: [PATCH net-next 1/2] net: add and use skb_get_hash

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: <6663159ab88ef_2f27b294c5@willemb.c.googlers.com.notmuch>
Date: Fri, 07 Jun 2024 10:13:46 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Eric Dumazet <edumazet@...gle.com>, 
 Florian Westphal <fw@...len.de>
Cc: netdev@...r.kernel.org, 
 Paolo Abeni <pabeni@...hat.com>, 
 "David S. Miller" <davem@...emloft.net>, 
 Jakub Kicinski <kuba@...nel.org>, 
 netfilter-devel@...r.kernel.org, 
 pablo@...filter.org, 
 willemb@...gle.com, 
 Christoph Paasch <cpaasch@...le.com>
Subject: Re: [PATCH net-next 1/2] net: add and use skb_get_hash_net

Eric Dumazet wrote:
> On Fri, Jun 7, 2024 at 10:36 AM Florian Westphal <fw@...len.de> wrote:
> >
> > Years ago flow dissector gained ability to delegate flow dissection
> > to a bpf program, scoped per netns.
> >
> > Unfortunately, skb_get_hash() only gets an sk_buff argument instead
> > of both net+skb.  This means the flow dissector needs to obtain the
> > netns pointer from somewhere else.
> >
> > The netns is derived from skb->dev, and if that is not available, from
> > skb->sk.  If neither is set, we hit a (benign) WARN_ON_ONCE().
> >
> > Trying both dev and sk covers most cases, but not all, as recently
> > reported by Christoph Paasch.
> >
> > In case of nf-generated tcp reset, both sk and dev are NULL:
> >
> > WARNING: .. net/core/flow_dissector.c:1104
> >  skb_flow_dissect_flow_keys include/linux/skbuff.h:1536 [inline]
> >  skb_get_hash include/linux/skbuff.h:1578 [inline]
> >  nft_trace_init+0x7d/0x120 net/netfilter/nf_tables_trace.c:320
> >  nft_do_chain+0xb26/0xb90 net/netfilter/nf_tables_core.c:268
> >  nft_do_chain_ipv4+0x7a/0xa0 net/netfilter/nft_chain_filter.c:23
> >  nf_hook_slow+0x57/0x160 net/netfilter/core.c:626
> >  __ip_local_out+0x21d/0x260 net/ipv4/ip_output.c:118
> >  ip_local_out+0x26/0x1e0 net/ipv4/ip_output.c:127
> >  nf_send_reset+0x58c/0x700 net/ipv4/netfilter/nf_reject_ipv4.c:308
> >  nft_reject_ipv4_eval+0x53/0x90 net/ipv4/netfilter/nft_reject_ipv4.c:30
> >  [..]
> >
> > syzkaller did something like this:
> > table inet filter {
> >   chain input {
> >     type filter hook input priority filter; policy accept;
> >     meta nftrace set 1                  # calls skb_get_hash
> >     tcp dport 42 reject with tcp reset  # emits skb with NULL skb dev/sk
> >    }
> >    chain output {
> >     type filter hook output priority filter; policy accept;
> >     # empty chain is enough
> >    }
> > }
> >
> > ... then sends a tcp packet to port 42.
> >
> > Initial attempt to simply set skb->dev from nf_reject_ipv4 doesn't cover
> > all cases: skbs generated via ipv4 igmp_send_report trigger similar splat.

Does this mean we have more non-nf callsites to convert?

> >
> > Moreover, Pablo Neira found that nft_hash.c uses __skb_get_hash_symmetric()
> > which would trigger same warn splat for such skbs.
> >
> > Lets allow callers to pass the current netns explicitly.
> > The nf_trace infrastructure is adjusted to use the new helper.
> >
> > __skb_get_hash_symmetric is handled in the next patch.
> >
> > Reported-by: Christoph Paasch <cpaasch@...le.com>
> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/494
> > Signed-off-by: Florian Westphal <fw@...len.de>
> 
> Nice, I had an internal syzbot report about the same issue.
> 
> Reviewed-by: Eric Dumazet <edumazet@...gle.com>

Subject to the documentation warning from the bot

Reviewed-by: Willem de Bruijn <willemb@...gle.com>

Thanks for fixing this, Florian.