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: <CANn89i+50SE0Lnbpj1b1u62CyOfVxH25bneXnc3e=RJB0+jJ9g@mail.gmail.com>
Date: Fri, 7 Jun 2024 11:25:07 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: 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

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