diff mbox series

[nf,1/1] netfilter: nf_queue: hold bridge skb->dev while queued

Message ID ca7ee343bbcb44905e1f5b853df2f3a5b7d40548.1778493188.git.royenheart@gmail.com
State Superseded, archived
Headers show
Series [nf,1/1] netfilter: nf_queue: hold bridge skb->dev while queued | expand

Commit Message

Ren Wei May 12, 2026, 7:57 a.m. UTC 
From: Haoze Xie <royenheart@gmail.com>

br_pass_frame_up() rewrites skb->dev from the ingress port to the bridge
master before queueing bridge LOCAL_IN packets. NFQUEUE only holds
references on state.in/out and bridge physdevs, so a queued bridge
packet can retain a freed bridge master in skb->dev until reinjection.

When the verdict is reinjected later, br_netif_receive_skb() re-enters
the receive path with skb->dev still pointing at the freed bridge master,
triggering a use-after-free.

Store skb->dev in the queue entry for bridge builds, hold a reference on
it for the queue lifetime, and use the saved device when dropping queued
packets during NETDEV_DOWN handling.

Fixes: ac2863445686 ("netfilter: bridge: add nf_afinfo to enable queuing to userspace")
Cc: stable@kernel.org
Reported-by: Yuan Tan <yuantan098@gmail.com>
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Reported-by: Xin Liu <bird@lzu.edu.cn>
Tested-by: Haoze Xie <royenheart@gmail.com>
Signed-off-by: Haoze Xie <royenheart@gmail.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
---
 include/net/netfilter/nf_queue.h | 1 +
 net/netfilter/nf_queue.c         | 5 +++++
 net/netfilter/nfnetlink_queue.c  | 3 +++
 3 files changed, 9 insertions(+)

Comments

Pablo Neira Ayuso May 12, 2026, 10:33 a.m. UTC | #1 
On Tue, May 12, 2026 at 03:57:25PM +0800, Ren Wei wrote:
> From: Haoze Xie <royenheart@gmail.com>
> 
> br_pass_frame_up() rewrites skb->dev from the ingress port to the bridge
> master before queueing bridge LOCAL_IN packets. NFQUEUE only holds
> references on state.in/out and bridge physdevs, so a queued bridge
> packet can retain a freed bridge master in skb->dev until reinjection.
> 
> When the verdict is reinjected later, br_netif_receive_skb() re-enters
> the receive path with skb->dev still pointing at the freed bridge master,
> triggering a use-after-free.
> 
> Store skb->dev in the queue entry for bridge builds, hold a reference on
> it for the queue lifetime, and use the saved device when dropping queued
> packets during NETDEV_DOWN handling.
> 
> Fixes: ac2863445686 ("netfilter: bridge: add nf_afinfo to enable queuing to userspace")
> Cc: stable@kernel.org
> Reported-by: Yuan Tan <yuantan098@gmail.com>
> Reported-by: Yifan Wu <yifanwucs@gmail.com>
> Reported-by: Juefei Pu <tomapufckgml@gmail.com>
> Reported-by: Xin Liu <bird@lzu.edu.cn>
> Tested-by: Haoze Xie <royenheart@gmail.com>
> Signed-off-by: Haoze Xie <royenheart@gmail.com>
> Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
> ---
>  include/net/netfilter/nf_queue.h | 1 +
>  net/netfilter/nf_queue.c         | 5 +++++
>  net/netfilter/nfnetlink_queue.c  | 3 +++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/include/net/netfilter/nf_queue.h b/include/net/netfilter/nf_queue.h
> index d17035d14d96..1e7eb8e85932 100644
> --- a/include/net/netfilter/nf_queue.h
> +++ b/include/net/netfilter/nf_queue.h
> @@ -17,6 +17,7 @@ struct nf_queue_entry {
>  	unsigned int		id;
>  	unsigned int		hook_index;	/* index in hook_entries->hook[] */
>  #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
> +	struct net_device	*skb_dev;

patch is not correct, this is only fixing it for br_netfilter.

>  	struct net_device	*physin;
>  	struct net_device	*physout;
>  #endif

Maybe normalize this special case with this patch instead? I will
propose it to the bridge maintainer.

It is strange that skb->dev != indev.

I have to take a second look, but I don't a usecase where skb->dev is
used in the netfilter tree can could break.
Pablo Neira Ayuso May 12, 2026, 11:03 a.m. UTC | #2 
On Tue, May 12, 2026 at 12:33:37PM +0200, Pablo Neira Ayuso wrote:
> On Tue, May 12, 2026 at 03:57:25PM +0800, Ren Wei wrote:
> > From: Haoze Xie <royenheart@gmail.com>
> > 
> > br_pass_frame_up() rewrites skb->dev from the ingress port to the bridge
> > master before queueing bridge LOCAL_IN packets. NFQUEUE only holds
> > references on state.in/out and bridge physdevs, so a queued bridge
> > packet can retain a freed bridge master in skb->dev until reinjection.
> > 
> > When the verdict is reinjected later, br_netif_receive_skb() re-enters
> > the receive path with skb->dev still pointing at the freed bridge master,
> > triggering a use-after-free.
> > 
> > Store skb->dev in the queue entry for bridge builds, hold a reference on
> > it for the queue lifetime, and use the saved device when dropping queued
> > packets during NETDEV_DOWN handling.
> > 
> > Fixes: ac2863445686 ("netfilter: bridge: add nf_afinfo to enable queuing to userspace")
> > Cc: stable@kernel.org
> > Reported-by: Yuan Tan <yuantan098@gmail.com>
> > Reported-by: Yifan Wu <yifanwucs@gmail.com>
> > Reported-by: Juefei Pu <tomapufckgml@gmail.com>
> > Reported-by: Xin Liu <bird@lzu.edu.cn>
> > Tested-by: Haoze Xie <royenheart@gmail.com>
> > Signed-off-by: Haoze Xie <royenheart@gmail.com>
> > Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
> > ---
> >  include/net/netfilter/nf_queue.h | 1 +
> >  net/netfilter/nf_queue.c         | 5 +++++
> >  net/netfilter/nfnetlink_queue.c  | 3 +++
> >  3 files changed, 9 insertions(+)
> > 
> > diff --git a/include/net/netfilter/nf_queue.h b/include/net/netfilter/nf_queue.h
> > index d17035d14d96..1e7eb8e85932 100644
> > --- a/include/net/netfilter/nf_queue.h
> > +++ b/include/net/netfilter/nf_queue.h
> > @@ -17,6 +17,7 @@ struct nf_queue_entry {
> >  	unsigned int		id;
> >  	unsigned int		hook_index;	/* index in hook_entries->hook[] */
> >  #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
> > +	struct net_device	*skb_dev;
> 
> patch is not correct, this is only fixing it for br_netfilter.
> 
> >  	struct net_device	*physin;
> >  	struct net_device	*physout;
> >  #endif
> 
> Maybe normalize this special case with this patch instead? I will
> propose it to the bridge maintainer.
> 
> It is strange that skb->dev != indev.
> 
> I have to take a second look, but I don't a usecase where skb->dev is
> used in the netfilter tree can could break.

Scratch this proposal, it also breaks.
Pablo Neira Ayuso May 12, 2026, 11:24 a.m. UTC | #3 
On Tue, May 12, 2026 at 03:57:25PM +0800, Ren Wei wrote:
> From: Haoze Xie <royenheart@gmail.com>
> 
> br_pass_frame_up() rewrites skb->dev from the ingress port to the bridge
> master before queueing bridge LOCAL_IN packets. NFQUEUE only holds
> references on state.in/out and bridge physdevs, so a queued bridge
> packet can retain a freed bridge master in skb->dev until reinjection.
> 
> When the verdict is reinjected later, br_netif_receive_skb() re-enters
> the receive path with skb->dev still pointing at the freed bridge master,
> triggering a use-after-free.
> 
> Store skb->dev in the queue entry for bridge builds, hold a reference on
> it for the queue lifetime, and use the saved device when dropping queued
> packets during NETDEV_DOWN handling.

Next attempt: Maybe hold reference on skb->dev...
Florian Westphal May 12, 2026, 11:29 a.m. UTC | #4 
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, May 12, 2026 at 03:57:25PM +0800, Ren Wei wrote:
> > From: Haoze Xie <royenheart@gmail.com>
> > 
> > br_pass_frame_up() rewrites skb->dev from the ingress port to the bridge
> > master before queueing bridge LOCAL_IN packets. NFQUEUE only holds
> > references on state.in/out and bridge physdevs, so a queued bridge
> > packet can retain a freed bridge master in skb->dev until reinjection.
> > 
> > When the verdict is reinjected later, br_netif_receive_skb() re-enters
> > the receive path with skb->dev still pointing at the freed bridge master,
> > triggering a use-after-free.
> > 
> > Store skb->dev in the queue entry for bridge builds, hold a reference on
> > it for the queue lifetime, and use the saved device when dropping queued
> > packets during NETDEV_DOWN handling.
> 
> Next attempt: Maybe hold reference on skb->dev...

> diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
> index a6c81c04b3a5..26a4db5e17d4 100644
> --- a/net/netfilter/nf_queue.c
> +++ b/net/netfilter/nf_queue.c
> @@ -66,6 +66,7 @@ static void nf_queue_entry_release_refs(struct nf_queue_entry *entry)
>  	if (state->sk)
>  		nf_queue_sock_put(state->sk);
>  
> +	dev_put(entry->skb->dev);
>  #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
>  	dev_put(entry->physin);
>  	dev_put(entry->physout);
> @@ -104,6 +105,7 @@ bool nf_queue_entry_get_refs(struct nf_queue_entry *entry)
>  
>  	dev_hold(state->in);
>  	dev_hold(state->out);
> +	dev_hold(entry->skb->dev);

We should also extend

net/netfilter/nfnetlink_queue.c:dev_cmp() to consider skb->dev, if set.

And I think skb->dev can be NULL here in output path.
diff mbox series

Patch

diff --git a/include/net/netfilter/nf_queue.h b/include/net/netfilter/nf_queue.h
index d17035d14d96..1e7eb8e85932 100644
--- a/include/net/netfilter/nf_queue.h
+++ b/include/net/netfilter/nf_queue.h
@@ -17,6 +17,7 @@  struct nf_queue_entry {
 	unsigned int		id;
 	unsigned int		hook_index;	/* index in hook_entries->hook[] */
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
+	struct net_device	*skb_dev;
 	struct net_device	*physin;
 	struct net_device	*physout;
 #endif
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index a6c81c04b3a5..08da3f4700da 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -67,6 +67,7 @@  static void nf_queue_entry_release_refs(struct nf_queue_entry *entry)
 		nf_queue_sock_put(state->sk);
 
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
+	dev_put(entry->skb_dev);
 	dev_put(entry->physin);
 	dev_put(entry->physout);
 #endif
@@ -106,6 +107,7 @@  bool nf_queue_entry_get_refs(struct nf_queue_entry *entry)
 	dev_hold(state->out);
 
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
+	dev_hold(entry->skb_dev);
 	dev_hold(entry->physin);
 	dev_hold(entry->physout);
 #endif
@@ -207,6 +209,9 @@  static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state,
 		.size	= sizeof(*entry) + route_key_size,
 	};
 
+#if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
+	entry->skb_dev = skb->dev;
+#endif
 	__nf_queue_entry_init_physdevs(entry);
 
 	if (!nf_queue_entry_get_refs(entry)) {
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 58304fd1f70f..b64a59bb4ba7 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -1206,6 +1206,9 @@  dev_cmp(struct nf_queue_entry *entry, unsigned long ifindex)
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
 	int physinif, physoutif;
 
+	if (entry->skb_dev && entry->skb_dev->ifindex == ifindex)
+		return 1;
+
 	physinif = nf_bridge_get_physinif(entry->skb);
 	physoutif = nf_bridge_get_physoutif(entry->skb);