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

[Florian Westphal]

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;
> }
> 
> This drops the fake rtable that br_nf_pre_routing_finish() attached. If
> tunnel_dst is NULL or dst_hold_safe() fails, the skb continues with no
> dst.
> 
> The delete side runs under RTNL, and nothing orders it against the
> softirq reader:
> 
> net/bridge/br_vlan_tunnel.c:vlan_tunnel_info_release() {
>     ...
> 	WRITE_ONCE(vlan->tinfo.tunnel_id, 0);
> 	RCU_INIT_POINTER(vlan->tinfo.tunnel_dst, NULL);
> 	dst_release(&tdst->dst);
> }
> 
> 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?

No.  This is the wrong place.  We don't work around bugs in bridge by
adding random skb->dst == NULL tests in random xt or nft parts.

If this bug is real, it has to fixed in either bridge itself or
br_netfilter.  Calling inet stack from bridge was never a good idea, and
the onus is on br_netfilter to satisfy ip stack preconditions.

inet netfilter land should not have to cater / cope with bogus skbs
originating in bridge layer.