| Message ID | 20220223201004.30615-1-fw@strlen.de |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nf] netfilter: nf_queue: don't assume sk is full socket | expand |
On Wed, Feb 23, 2022 at 09:10:04PM +0100, 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. Applied, thanks
Hello. On středa 23. února 2022 21:10:04 CET 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. No thank you for spotting the exact issue. > Fixes: ca6fb0651883 ("tcp: attach SYNACK messages to request sockets instead of listener") > Signed-off-by: Florian Westphal <fw@strlen.de> Tested-by: Oleksandr Natalenko <oleksandr@redhat.com> > --- > No reproducer, so there is a chance that the reported crash is caused by > something else. That said, I don't see how sock_put use is safe here. Using your reproducer from [1] I could trigger the issue on unpatched kernel and couldn't trigger the issue on patched kernel with the patch from below applied. [1] https://patchwork.ozlabs.org/project/netfilter-devel/patch/20220224152118.20619-1-fw@strlen.de/ > > net/netfilter/nf_queue.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c > index 6d12afabfe8a..178742a110d9 100644 > --- a/net/netfilter/nf_queue.c > +++ b/net/netfilter/nf_queue.c > @@ -54,7 +54,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); > + sock_gen_put(state->sk); > > #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER) > dev_put(entry->physin); >
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c index 6d12afabfe8a..178742a110d9 100644 --- a/net/netfilter/nf_queue.c +++ b/net/netfilter/nf_queue.c @@ -54,7 +54,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); + sock_gen_put(state->sk); #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER) dev_put(entry->physin);
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") Signed-off-by: Florian Westphal <fw@strlen.de> --- No reproducer, so there is a chance that the reported crash is caused by something else. That said, I don't see how sock_put use is safe here. net/netfilter/nf_queue.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)