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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says the patch is defensive only ("No
  reachable path that passes an skb with a NULL dst to…
- [Low] The patch fixes a reachable NULL-pointer dereference in
  match_policy_out() (net/netfilter/xt_policy.c) but has no Fixes: tag…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but xt_realm has
  the same unchecked skb_dst() dereference.

--

netfilter: xt_policy: add NULL check for skb_dst() in output policy match

This commit adds a NULL check for skb_dst() in match_policy_out() before
dst->xfrm is accessed. When no dst is attached, the function returns -1,
the same as for a dst without XFRM state. The commit message says the
change is defensive and that no reachable path is known.

> Code review identified the unchecked dereference. No reachable path
> that passes an skb with a NULL dst to match_policy_out() has been
> identified in current kernels.

[Severity: Medium]

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?

> Reviewed-by: Weibin Liu <liuwb@xiaopeng.com>
> Signed-off-by: Fang Xieyan <fangxy@xiaopeng.com>

[Severity: Low]

Since the dereference looks reachable, should this patch carry a Fixes:
tag and Cc: stable? Without them, stable tooling won't pick it up.

The unchecked dst->xfrm dereference dates back to c4b885139203
("[NETFILTER]: Add policy match").

The behaviour of continuing with no dst after skb_dst_drop() in
br_handle_egress_vlan_tunnel() came from 58e2071742e3 ("net: bridge: fix
vlan tunnel dst null pointer dereference") and cfc579f9d89a ("net:
bridge: fix vlan tunnel dst refcnt when egressing").

Either one could be a candidate Fixes: target.

> diff --git a/net/netfilter/xt_policy.c b/net/netfilter/xt_policy.c
> index ff54e3a8581e1..6bbaf92f0cfc9 100644
> --- a/net/netfilter/xt_policy.c
> +++ b/net/netfilter/xt_policy.c
> @@ -87,7 +87,7 @@ match_policy_out(const struct sk_buff *skb, const struct xt_policy_info *info,
>  	int strict = info->flags & XT_POLICY_MATCH_STRICT;
>  	int i, pos;
>  
> -	if (dst->xfrm == NULL)
> +	if (!dst || dst->xfrm == NULL)
>  		return -1;

[Severity: High]

This isn't a bug introduced by this patch, but realm_mt() in
net/netfilter/xt_realm.c appears to have the same unchecked skb_dst()
dereference:

net/netfilter/xt_realm.c:realm_mt() {
	const struct xt_realm_info *info = par->matchinfo;
	const struct dst_entry *dst = skb_dst(skb);

	return (info->id == (dst->tclassid & info->mask)) ^ info->invert;
}

realm_mt_reg registers it for NFPROTO_IPV4 in these hooks:

	.hooks		= (1 << NF_INET_POST_ROUTING) | (1 << NF_INET_FORWARD) |
			  (1 << NF_INET_LOCAL_OUT) | (1 << NF_INET_LOCAL_IN),

Bridged IPv4 traffic reaches FORWARD through br_nf_forward_ip() and
POSTROUTING through br_nf_post_routing(). The same
br_handle_egress_vlan_tunnel() and vlan_tunnel_info_release() race
described above can leave that skb with no dst.

Would a "-m realm" rule in FORWARD or POSTROUTING then dereference
NULL->tclassid in softirq context? This patch only covers
match_policy_out(), not this sibling.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261026125313.14810-1-fangxy%40xiaopeng.com