netdev - Re: [PATCH nf] netfilter: nf_reject: init skb->dev for reset packet

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]

Date: Wed, 5 Jun 2024 17:38:00 -0400
From: Willem de Bruijn <willemb@...gle.com>
To: Pablo Neira Ayuso <pablo@...filter.org>
Cc: Florian Westphal <fw@...len.de>, Christoph Paasch <cpaasch@...le.com>, 
	Netfilter <netfilter-devel@...r.kernel.org>, Paolo Abeni <pabeni@...hat.com>, 
	netdev@...r.kernel.org, daniel@...earbox.net, 
	Stanislav Fomichev <sdf@...gle.com>
Subject: Re: [PATCH nf] netfilter: nf_reject: init skb->dev for reset packet

On Wed, Jun 5, 2024 at 3:45 PM Pablo Neira Ayuso <pablo@...filter.org> wrote:
>
> On Wed, Jun 05, 2024 at 09:08:33PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@...filter.org> wrote:
> >
> > [ CC Willem ]
> >
> > > On Wed, Jun 05, 2024 at 08:14:50PM +0200, Florian Westphal wrote:
> > > > Christoph Paasch <cpaasch@...le.com> wrote:
> > > > > > Reported-by: Christoph Paasch <cpaasch@...le.com>
> > > > > > Suggested-by: Paolo Abeni <pabeni@...hat.com>
> > > > > > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/494
> > > > > > Signed-off-by: Florian Westphal <fw@...len.de>
> > > > >
> > > > > I just gave this one a shot in my syzkaller instances and am still hitting the issue.
> > > >
> > > > No, different bug, this patch is correct.
> > > >
> > > > I refuse to touch the flow dissector.
> > >
> > > I see callers of ip_local_out() in the tree which do not set skb->dev.
> > >
> > > I don't understand this:
> > >
> > > bool __skb_flow_dissect(const struct net *net,
> > >                         const struct sk_buff *skb,
> > >                         struct flow_dissector *flow_dissector,
> > >                         void *target_container, const void *data,
> > >                         __be16 proto, int nhoff, int hlen, unsigned int flags)
> > > {
> > > [...]
> > >         WARN_ON_ONCE(!net);
> > >         if (net) {
> > >
> > > it was added by 9b52e3f267a6 ("flow_dissector: handle no-skb use case")
> > >
> > > Is this WARN_ON_ONCE() bogus?
> >
> > When this was added (handle dissection from bpf prog, per netns), the correct
> > solution would have been to pass 'struct net' explicitly via skb_get_hash()
> > and all variants.  As that was likely deemed to be too much code churn it
> > tries to infer struct net via skb->{dev,sk}.

It has been a while, but I think we just did not anticipate skb's with
neither dev nor sk set.

Digging through the layers from skb_hash to __skb_flow_dissect
now, it does look impractical to add such an explicit API.

> > So there are several options here:
> > 1. remove the WARN_ON_ONCE and be done with it
> > 2. remove the WARN_ON_ONCE and pretend net was init_net
> > 3. also look at skb_dst(skb)->dev if skb->dev is unset, then back to 1)
> >    or 2)
> > 4. stop using skb_get_hash() from netfilter (but there are likely other
> >    callers that might hit this).
> > 5. fix up callers, one by one
> > 6. assign skb->dev inside netfilter if its unset

Is 6 a realistic option?

> >
> > 3 and 2 combined are probably going to be the least invasive.
> >
> > 5 might take some time, we now know two, namely tcp resets generated
> > from netfilter and igmp_send_report().  No idea if there are more.
>
> Quickly browsing, synproxy and tee also calls ip_local_out() too.
>
> icmp_send() which is used, eg. to send destination unreachable too to
> reset.

Since this uses ip_append_data the generated response should have
its skb->sk set.

> There is also __skb_get_hash_symmetric() that could hit this from
> nft_hash?
>
> No idea what more callers need to be adjusted to remove this splat,
> that was a cursory tree review.
>
> And ip_output() sets on skb->dev from postrouting path, then if
> callers are fixed, then skb->dev would be once then again from output path?