Re: [PATCH] netfilter: br_netfilter: reject unsafe neighbour devices for DNAT

[Kyle Zeng <kylebot@openai.com>]

On Sat, Nov 07, 2026 at 08:25:41AM +0100, Florian Westphal wrote:
> Kyle Zeng <kylebot@openai.com> wrote:
> > The IPv4 and IPv6 bridge prerouting callbacks classify DNAT traffic using
> > the logical input device saved in skb->dev. The physical input port can
> > have a different bridge master when prerouting resumes, for example after
> > an NFQUEUE verdict. Netfilter reassembly can also combine the first
> > fragment's bridge information with a different fragment's input device.
> > 
> > br_nf_pre_routing_finish_bridge() replaces skb->dev with the port's
> > current bridge master, but looks up the neighbour using the existing
> > route. This can put an skb with skb->dev pointing to bridge B on an
> > unresolved neighbour of bridge A. Deleting B and resolving the neighbour
> > on A then causes a use-after-free in __dev_queue_xmit().
> > 
> > Before calling the neighbour output method, check that the neighbour
> > keeps skb->dev alive. A neighbour holds its device, and a VLAN device
> > also holds its real device, so allow a VLAN whose real device is the
> > bridge. Release the neighbour and drop the skb on a mismatch.
> > 
> > Doing this in the shared continuation covers both protocol families,
> > including a queue in the remaining bridge prerouting hooks. Leave the
> > cached-header path unchanged since it does not enqueue the skb.
> > 
> > Signed-off-by: Kyle Zeng <kylebot@openai.com>
> > ---
> > I have a reproducer for this issue and can share via DM
> > 
> >  net/bridge/br_netfilter_hooks.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
> > index e15355ff215a..726a2d17ed3a 100644
> > --- a/net/bridge/br_netfilter_hooks.c
> > +++ b/net/bridge/br_netfilter_hooks.c
> > @@ -307,6 +307,18 @@ int br_nf_pre_routing_finish_bridge(struct net *net, struct sock *sk, struct sk_
> >  
> >  			ret = br_handle_frame_finish(net, sk, skb);
> >  		} else {
> > +			struct net_device *dev = neigh->dev;
> > +
> > +			/* The neighbour queue must keep skb->dev alive. A VLAN
> > +			 * device also holds a reference to its real device.
> > +			 */
> > +			if (is_vlan_dev(dev))
> > +				dev = vlan_dev_real_dev(dev);
> > +			if (dev != skb->dev) {
> > +				neigh_release(neigh);
> > +				goto free_skb;
> > +			}
> > +
> 
> I don't understand the vlan check. Why is that needed?
> The call to br_nf_pre_routing_finish_bridge is gated via:
> 
>  405                         if (skb_dst(skb)->dev == dev) {
>  406                                 skb->dev = br_indev;
>  407                                 nf_bridge_update_protocol(skb);
>  408                                 nf_bridge_push_encap_header(skb);
>  409                                 br_nf_hook_thresh(NF_BR_PRE_ROUTING,
>  410                                                   net, sk, skb, skb->dev,
>  411                                                   NULL,
>  412                                                   br_nf_pre_routing_finish_bridge);
> 
> 
> ... so this patch ought to be sufficient?
> 
> --- a/net/bridge/br_netfilter_hooks.c
> +++ b/net/bridge/br_netfilter_hooks.c
> @@ -283,6 +283,10 @@ int br_nf_pre_routing_finish_bridge(struct net *net, struct sock *sk, struct sk_
>         if (!skb->dev)
>                 goto free_skb;
>         dst = skb_dst(skb);
> +
> +       if (dst->dev != skb->dev)
> +               goto free_skb;
> +
>         neigh = dst_neigh_lookup_skb(dst, skb);
> 
> (I can't see how neigh->dev != skb->dev after the extra test).


The VLAN check is intended to preserve existing VLAN DNAT behavior.

bridge-nf-filter-vlan-tagged=1, bridge-nf-pass-vlan-input-dev=1, and br0.100, the earlier test can pass with
dst->dev == dev == br0.100. The continuation then changes skb->dev to br0, so the strict comparison would
drop that valid packet.

A neighbour on br0.100 is safe here because it holds the VLAN device, which holds its underlying bridge. That is why the slow-path check compares the neighbour´s underlying device with skb->dev.