[PATCH v2 nf] netfilter: nf_queue: don't assume sk is full socket

[PATCH v2 nf] netfilter: nf_queue: don't assume sk is full socket

[Florian Westphal]

There is no guarantee that state->sk refers to a full socket.

If refcount transitions to 0, sock_put calls sk_free which then ends up
with garbage fields.

I'd like to thank Oleksandr Natalenko and Jiri Benc for considerable
debug work and pointing out state->sk oddities.

Fixes: ca6fb0651883 ("tcp: attach SYNACK messages to request sockets instead of listener")
Tested-by: Oleksandr Natalenko <oleksandr@redhat.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 v2: fix build failure with CONFIG_INET=n.
 No difference for CONFIG_INET=y build.

 net/netfilter/nf_queue.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index 6d12afabfe8a..5ab0680db445 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -46,6 +46,15 @@ void nf_unregister_queue_handler(void)
 }
 EXPORT_SYMBOL(nf_unregister_queue_handler);
 
+static void nf_queue_sock_put(struct sock *sk)
+{
+#ifdef CONFIG_INET
+	sock_gen_put(sk);
+#else
+	sock_put(sk);
+#endif
+}
+
 static void nf_queue_entry_release_refs(struct nf_queue_entry *entry)
 {
 	struct nf_hook_state *state = &entry->state;
@@ -54,7 +63,7 @@ static void nf_queue_entry_release_refs(struct nf_queue_entry *entry)
 	dev_put(state->in);
 	dev_put(state->out);
 	if (state->sk)
-		sock_put(state->sk);
+		nf_queue_sock_put(state->sk);
 
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
 	dev_put(entry->physin);
-- 
2.34.1

Re: [PATCH v2 nf] netfilter: nf_queue: don't assume sk is full socket

[Eric Dumazet]

On 2/25/22 05:02, Florian Westphal wrote:
> There is no guarantee that state->sk refers to a full socket.
>
> If refcount transitions to 0, sock_put calls sk_free which then ends up
> with garbage fields.
>
> I'd like to thank Oleksandr Natalenko and Jiri Benc for considerable
> debug work and pointing out state->sk oddities.
>
> Fixes: ca6fb0651883 ("tcp: attach SYNACK messages to request sockets instead of listener")
> Tested-by: Oleksandr Natalenko <oleksandr@redhat.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>   v2: fix build failure with CONFIG_INET=n.
>   No difference for CONFIG_INET=y build.
>
>   net/netfilter/nf_queue.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
> index 6d12afabfe8a..5ab0680db445 100644
> --- a/net/netfilter/nf_queue.c
> +++ b/net/netfilter/nf_queue.c
> @@ -46,6 +46,15 @@ void nf_unregister_queue_handler(void)
>   }
>   EXPORT_SYMBOL(nf_unregister_queue_handler);
>   
> +static void nf_queue_sock_put(struct sock *sk)
> +{
> +#ifdef CONFIG_INET
> +	sock_gen_put(sk);
> +#else
> +	sock_put(sk);
> +#endif
> +}
> +
>   static void nf_queue_entry_release_refs(struct nf_queue_entry *entry)
>   {
>   	struct nf_hook_state *state = &entry->state;
> @@ -54,7 +63,7 @@ static void nf_queue_entry_release_refs(struct nf_queue_entry *entry)
>   	dev_put(state->in);
>   	dev_put(state->out);
>   	if (state->sk)
> -		sock_put(state->sk);
> +		nf_queue_sock_put(state->sk);
>   
>   #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
>   	dev_put(entry->physin);


OK but it looks like there might be an orthogonal bug.

The sock_hold() side seems suspect, because there is no guarantee

that sk_refcnt is not already 0.

Not sure how netfilter would react with stats->sk set to NULL ?

diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index 
6d12afabfe8a35069f5355ceb57e8147cf59d187..46da92e3bb436ceaec8f9339fc5be7a9c372ce64 
100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -93,8 +93,9 @@ void nf_queue_entry_get_refs(struct nf_queue_entry *entry)

         dev_hold(state->in);
         dev_hold(state->out);
-       if (state->sk)
-               sock_hold(state->sk);
+
+       if (state->sk && !refcount_inc_not_zero(&state->sk->sk_refcnt))
+               state->sk = NULL;

  #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
         dev_hold(entry->physin);

Re: [PATCH v2 nf] netfilter: nf_queue: don't assume sk is full socket

[Florian Westphal]

Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > +		nf_queue_sock_put(state->sk);
> >   #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
> >   	dev_put(entry->physin);
> 
> 
> OK but it looks like there might be an orthogonal bug.
> 
> The sock_hold() side seems suspect, because there is no guarantee
> 
> that sk_refcnt is not already 0.

Ugh.  Looks like we also need skb_sk_is_prefetched() check and then
take care of skb->sk too if its not owned by skb destructor.

> Not sure how netfilter would react with stats->sk set to NULL ?

Its passed as arg to dst_output() later so I think its fine.