Re: [PATCH net v3] net: neigh: Reallocate headroom if necessary in neigh_hh_bridge()

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2158 bytes --]

> On Wed, May 13, 2026 at 06:40:28PM +0200, Lorenzo Bianconi wrote:
> > neigh_hh_bridge() assumes the skb always has sufficient headroom to copy
> > the aligned  L2 header. This assumption can trigger the crash reported
> > below using the following netfilter setup:
> > 
> > $modprobe br_netfilter
> > $sysctl -w net.bridge.bridge-nf-call-iptables=1
> > 
> > $root@OpenWrt:~# nft list ruleset
> > table ip nat {
> >         chain prerouting {
> >                 type nat hook prerouting priority dstnat; policy accept;
> >                 ip daddr 192.168.83.123 dnat to 192.168.83.120
> >         }
> > }
> > 
> > - iperf3 client (192.168.83.119) --> bridge (192.168.83.118) --> iperf3 server (192.168.83.120)
> > 
> > the iperf3 client is sending packet for 192.168.83.123 to the bridge device.
> 
> [...]
> 
> > 
> > Fix the issue reallocating the skb headroom if necessary in neigh_hh_bridge routine.
> > 
> > Fixes: e179e6322ac33 ("netfilter: bridge-netfilter: Fix MAC header handling with IP DNAT")
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> 
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> 
> [...]
> 
> > diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
> > index 0ab1c94db4b9..cea2352900e9 100644
> > --- a/net/bridge/br_netfilter_hooks.c
> > +++ b/net/bridge/br_netfilter_hooks.c
> > @@ -297,7 +297,13 @@ int br_nf_pre_routing_finish_bridge(struct net *net, struct sock *sk, struct sk_
> >  				goto free_skb;
> >  			}
> >  
> > -			neigh_hh_bridge(&neigh->hh, skb);
> > +			ret = neigh_hh_bridge(&neigh->hh, skb);
> > +			if (ret) {
> > +				neigh_release(neigh);
> > +				kfree_skb(skb);
> > +				return ret;
> 
> Personally I would use 'goto free_skb' after releasing the neighbour, to
> be consistent with the other paths that free the packet.

ack, I do not have a strong opinion about it, but in this case we would need to
even move "ret" since the current codebase always returns 0. What do you prefer?

Regards,
Lorenzo

> 
> > +			}
> > +
> >  			skb->dev = br_indev;
> >  
> >  			ret = br_handle_frame_finish(net, sk, skb);

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]