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@xxxxxxxxxx>
Signed-off-by: Florian Westphal <fw@xxxxxxxxx>
---
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);
