Re: [PATCH] netfilter: xt_policy: add NULL check for skb_dst() in output policy match

[Florian Westphal]

Florian Westphal <fw@xxxxxxxxx> wrote:

[ CC Nik ]

> netdev-bot+sashiko@xxxxxxxxxx <netdev-bot+sashiko@xxxxxxxxxx> wrote:
> > Is this statement accurate? There seems to be a reachable path through
> > br_netfilter on a bridge port that has vlan_tunnel enabled.
> > 
> > net/bridge/br_vlan_tunnel.c:br_handle_egress_vlan_tunnel() {
> >     ...
> > 	tunnel_id = READ_ONCE(vlan->tinfo.tunnel_id);
> > 	if (!tunnel_id || unlikely(!skb_vlan_tag_present(skb)))
> > 		return 0;
> > 
> > 	skb_dst_drop(skb);
> >     ...
> > 	tunnel_dst = rcu_dereference(vlan->tinfo.tunnel_dst);
> > 	if (tunnel_dst && dst_hold_safe(&tunnel_dst->dst))
> > 		skb_dst_set(skb, &tunnel_dst->dst);
> > 
> > 	return 0;
> > }

Hmm, Nik can you please have a look?  Its strange that we don't return
an error. dst was dropped, tunnel dst disappeared, hwaccel tag was
cleared.

Should this return 0 only if skb_dst_set() was called?

Afaics then callers would toss the skb and we won't pass this
to inet netfilter hooks.

The original code had an unconditional skb_dst_set(),
assuming tunnel_dst is always there.

That changed with
cfc579f9d89a ("net: bridge: fix vlan tunnel dst refcnt when egressing")
fixup.

I'll quote more from report verbatim below for context.

> > So a forwarder can read the old non-zero tunnel_id and then find
> > tunnel_dst NULL, or holding a dst that dst_hold_safe() rejects. The skb
> > then goes on like this:
> > 
> > __br_forward()
> >   br_handle_vlan()
> >     br_handle_egress_vlan_tunnel()   <- dst dropped, none attached
> >   NF_HOOK(NFPROTO_BRIDGE, NF_BR_FORWARD, ...)
> >     br_nf_forward()
> >       br_nf_forward_ip()
> >         NF_HOOK(pf, NF_INET_FORWARD, ...)
> >           ipt_do_table() / ip6t_do_table()
> >             policy_mt()
> >               match_policy_out()     <- dst == NULL
> > 
> > br_nf_post_routing() does the same for POSTROUTING, and
> > policy_mt_check_hooks() allows --dir out in both hooks.
> > 
> > The trigger needs a VLAN-filtering bridge with a vlan_tunnel port,
> > bridge-nf-call-iptables (or ip6tables) enabled, a "-m policy --dir out"
> > rule in FORWARD or POSTROUTING, and a tunnel mapping delete racing with
> > forwarded traffic. Given that, the pre-patch code would oops in softirq
> > context.
> > 
> > Could the commit message describe this path, so the change is not taken
> > as purely defensive?