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

[Florian Westphal <fw@strlen.de>]

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