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: Thu, 6 Jun 2024 00:16:22 +0200
From: Pablo Neira Ayuso <pablo@...filter.org>
To: Willem de Bruijn <willemb@...gle.com>
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 05, 2024 at 05:38:00PM -0400, Willem de Bruijn wrote:
> 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?

Postrouting path already sets on skb->dev via ip_output(), if callers
are "fixed" then skb->dev will get set twice.

the netfilter tracing infrastructure only needs a hash identifier for
this packet to be displayed from userspace when tracing rules, how is
the running the custom bpf dissector hook useful in such case?

the most correct solution is to pass struct net explicitly to the
dissector API instead of guessing what net this packet belongs to.

Else, remove this WARN_ON_ONCE which is just a oneliner.

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

thanks for explaining.

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