Re: [PATCH net-next 10/14] netfilter: bridge: annotate data-races around br_netfilter and nf_log sysctls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] netfilter: bridge: TOCTOU VLAN header pull/push in
  br_nf_forward_arp()
- [Low] netfilter: bridge: unmarked read and double write in
  brnf_sysctl_call_tables()
--

commit 5e1289f99dadfbb891673725020b1b41b41b6df22c8
Author: Jiayuan Chen <jiayuan.chen@linux.dev>

netfilter: bridge: annotate data-races around br_netfilter and nf_log sysctls

This commit annotates lockless reads of br_netfilter and nf_log sysctls in
the packet path with READ_ONCE() and uses WRITE_ONCE() when normalizing
values in brnf_sysctl_call_tables().

> diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
> index e15355ff215a7..8a47fe4e8eb61 100644
> --- a/net/bridge/br_netfilter_hooks.c
> +++ b/net/bridge/br_netfilter_hooks.c
> @@ -103,14 +103,14 @@ static inline bool is_vlan_ipv6(const struct sk_buff *skb,
>  	struct brnf_net *brnet = net_generic(net, brnf_net_id);
>  
>  	return vlan_proto(skb) == htons(ETH_P_IPV6) &&
> -	       brnet->filter_vlan_tagged;
> +	       READ_ONCE(brnet->filter_vlan_tagged);
>  }
>  
>  static inline bool is_vlan_arp(const struct sk_buff *skb, const struct net *net)
>  {
>  	struct brnf_net *brnet = net_generic(net, brnf_net_id);
>  
> -	return vlan_proto(skb) == htons(ETH_P_ARP) && brnet->filter_vlan_tagged;
> +	return vlan_proto(skb) == htons(ETH_P_ARP) && READ_ONCE(brnet->filter_vlan_tagged);
>  }

[Severity: Medium]
Can forcing a fresh reload of brnet->filter_vlan_tagged on every
is_vlan_arp() call corrupt skb->data and skb->network_header when
bridge-nf-filter-vlan-tagged is toggled from 1 to 0 while an 802.1Q ARP
frame is being bridged?

Unlike br_nf_forward_ip(), the ARP forwarding path leaves skb->protocol as
htons(ETH_P_8021Q) and re-evaluates is_vlan_arp() at each step before
br_nf_forward_finish() unconditionally pushes the VLAN header back:

net/bridge/br_netfilter_hooks.c:br_nf_forward_arp() {
    ...
	if (is_vlan_arp(skb, state->net))
		nf_bridge_pull_encap_header(skb);

	if (unlikely(!pskb_may_pull(skb, sizeof(struct arphdr))))
		return NF_DROP_REASON(skb, SKB_DROP_REASON_PKT_TOO_SMALL, 0);

	if (arp_hdr(skb)->ar_pln != 4) {
		if (is_vlan_arp(skb, state->net))
			nf_bridge_push_encap_header(skb);
		return NF_ACCEPT;
	}
	*d = state->in;
	NF_HOOK(NFPROTO_ARP, NF_ARP_FORWARD, state->net, state->sk, skb,
		state->in, state->out, br_nf_forward_finish);
    ...
}

net/bridge/br_netfilter_hooks.c:br_nf_forward_finish() {
    ...
	if (!IS_ARP(skb) && !is_vlan_arp(skb, net)) {
    ...
	} else {
		in = *((struct net_device **)(skb->cb));
	}
	nf_bridge_push_encap_header(skb);
    ...
}

If br_nf_forward() sees filter_vlan_tagged == 1 and dispatches to
br_nf_forward_arp(), which then reads 0, nf_bridge_pull_encap_header() is
skipped while arp_hdr(skb) is read at the 802.1Q header offset, and
br_nf_forward_finish() still calls nf_bridge_push_encap_header(), pushing 4
bytes (VLAN_HLEN) that were never pulled.  Similarly, if filter_vlan_tagged
changes from 1 to 0 after nf_bridge_pull_encap_header() has pulled the VLAN
header, an ar_pln != 4 frame returns NF_ACCEPT without pushing the header
back, or br_nf_forward_finish() takes the IP branch and drops the packet
because nf_bridge_get_physindev() finds no nf_bridge_info.

[ ... ]

> @@ -1178,7 +1178,7 @@ int brnf_sysctl_call_tables(const struct ctl_table *ctl, int write,
>  	ret = proc_dointvec(ctl, write, buffer, lenp, ppos);
>  
>  	if (write && *(int *)(ctl->data))
> -		*(int *)(ctl->data) = 1;
> +		WRITE_ONCE(*(int *)(ctl->data), 1);
>  	return ret;
>  }

[Severity: Low]
Does *(int *)(ctl->data) in the if condition still perform an unmarked read
after proc_dointvec() has already written directly to ctl->data?

Because proc_dointvec() updates ctl->data in place before normalizing it in
a second step, concurrent sysctl writes still race with the plain read,
non-zero values other than 1 are transiently visible to packet-path
readers, and a concurrent write of 0 can be overwritten with 1.  Passing a
stack copy of struct ctl_table pointing to a local int variable and issuing
a single WRITE_ONCE() when write && !ret would avoid both the unmarked read
and the two-step write.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109025425.328445-1-jiayuan.chen@linux.dev?part=10