[PATCH nf] netfilter: nf_queue: hold reference on skb->dev

[PATCH nf] netfilter: nf_queue: hold reference on skb->dev

[Pablo Neira Ayuso]

Before NF_BR_LOCAL_IN, skb->dev is mangled in a way that results in
state->in != skb->dev, which can result in UaF when accessing the bridge
device if removed while in the queue.

Reported-by: Ren Wei <n05ec@lzu.edu.cn>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_queue.c        | 6 ++++++
 net/netfilter/nfnetlink_queue.c | 3 +++
 2 files changed, 9 insertions(+)

diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index a6c81c04b3a5..9c6741673842 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -59,8 +59,11 @@ static void nf_queue_sock_put(struct sock *sk)
 static void nf_queue_entry_release_refs(struct nf_queue_entry *entry)
 {
 	struct nf_hook_state *state = &entry->state;
+	struct sk_buff *skb = entry->skb;
 
 	/* Release those devices we held, or Alexey will kill me. */
+	if (skb->dev)
+		dev_put(skb->dev);
 	dev_put(state->in);
 	dev_put(state->out);
 	if (state->sk)
@@ -98,10 +101,13 @@ static void __nf_queue_entry_init_physdevs(struct nf_queue_entry *entry)
 bool nf_queue_entry_get_refs(struct nf_queue_entry *entry)
 {
 	struct nf_hook_state *state = &entry->state;
+	struct sk_buff *skb = entry->skb;
 
 	if (state->sk && !refcount_inc_not_zero(&state->sk->sk_refcnt))
 		return false;
 
+	if (skb->dev)
+		dev_hold(skb->dev);
 	dev_hold(state->in);
 	dev_hold(state->out);
 
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 58304fd1f70f..7408b348da13 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -1212,6 +1212,9 @@ dev_cmp(struct nf_queue_entry *entry, unsigned long ifindex)
 	if (physinif == ifindex || physoutif == ifindex)
 		return 1;
 #endif
+	if (entry->skb->dev)
+		if (entry->skb->dev->ifindex == ifindex)
+			return 1;
 	if (entry->state.in)
 		if (entry->state.in->ifindex == ifindex)
 			return 1;
-- 
2.47.3

Re: [PATCH nf] netfilter: nf_queue: hold reference on skb->dev

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Before NF_BR_LOCAL_IN, skb->dev is mangled in a way that results in
> state->in != skb->dev, which can result in UaF when accessing the bridge
> device if removed while in the queue.
> 
> Reported-by: Ren Wei <n05ec@lzu.edu.cn>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  net/netfilter/nf_queue.c        | 6 ++++++
>  net/netfilter/nfnetlink_queue.c | 3 +++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
> index a6c81c04b3a5..9c6741673842 100644
> --- a/net/netfilter/nf_queue.c
> +++ b/net/netfilter/nf_queue.c
> @@ -59,8 +59,11 @@ static void nf_queue_sock_put(struct sock *sk)
>  static void nf_queue_entry_release_refs(struct nf_queue_entry *entry)
>  {
>  	struct nf_hook_state *state = &entry->state;
> +	struct sk_buff *skb = entry->skb;
>  
>  	/* Release those devices we held, or Alexey will kill me. */
> +	if (skb->dev)
> +		dev_put(skb->dev);

dev_hold/put(NULL) is safe, no need for NULL guard (and no need
to resend).

Thanks for making a patch.  I'll make a patch for the pptp/gre bug in
case reporter remains silent.