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

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

[Kyle Zeng]

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;
+			}
+
 			/* the neighbour function below overwrites the complete
 			 * MAC header, so we save the Ethernet source address and
 			 * protocol number.

base-commit: d89e1d186ec9271bf78e62771779e9fc03bf0fc3
-- 
2.56.0

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Florian Westphal]

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

Please do.  I'd like to understand if that needs to be addressed
from nfqueue side instead, or if we can avoid neigh layer to
begin with.

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

[Florian Westphal]

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).

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

[Kyle Zeng]

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.

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

[Florian Westphal]

Kyle Zeng <kylebot@openai.com> wrote:
> On Sat, Nov 07, 2026 at 08:25:41AM +0100, Florian Westphal wrote:
> > > +			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;
> > > +			}
> > > +
[..]
> 
> 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.

Then please consider sending a v2 that explains this.

/* The neighbour queue must keep skb->dev alive. dst->dev is either
 * the bridge or a vlan on top of bridge
 * (bridge-nf-pass-vlan-input-dev=1).
 */

might just be enough.  Thanks.