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

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

[Fang Xieyan]

match_policy_out() accesses dst->xfrm without checking whether
skb_dst() returned NULL. If no dst entry is attached to the skb,
this dereference is invalid.

Check dst before accessing dst->xfrm and return -1 when no dst entry
is present. This follows the existing handling of a dst entry
without XFRM state and preserves the XT_POLICY_MATCH_NONE semantics
in policy_mt().

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.
Reviewed-by: Weibin Liu <liuwb@xiaopeng.com>
Signed-off-by: Fang Xieyan <fangxy@xiaopeng.com>
---
 net/netfilter/xt_policy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/xt_policy.c b/net/netfilter/xt_policy.c
index ff54e3a8581e..6bbaf92f0cfc 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;
 
 	for (i = 0; dst && dst->xfrm;
-- 
2.50.1

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

[Florian Westphal]

Fang Xieyan <fangxy@xiaopeng.com> wrote:
> match_policy_out() accesses dst->xfrm without checking whether
> skb_dst() returned NULL. If no dst entry is attached to the skb,
> this dereference is invalid.

If there is no dst we'd have crashed already:

        return nf_hook(NFPROTO_IPV4, NF_INET_LOCAL_OUT,
                       net, sk, skb, NULL, skb_dst_dev(skb),
                       dst_output);

likewise, okter in ip stack:

        if (lwtunnel_xmit_redirect(dst->lwtstate)) {

there are several other places that don't do NULL checks here.
So either all of those need them or none at all.

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

[Fang Xieyan]

Hi Florian,

Thanks for the explanation. Agreed, I'll drop this patch.

Thanks,
Fang Xieyan

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

[netdev-bot+sashiko]

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

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

[Florian Westphal]

netdev-bot+sashiko@kernel.org <netdev-bot+sashiko@kernel.org> 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.