| Message ID | 20250206-vsock-linger-nullderef-v2-1-f8a1f19146f8@rbox.co (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | vsock: null-ptr-deref when SO_LINGER enabled | expand |
On Thu, Feb 06, 2025 at 12:06:47AM +0100, Michal Luczaj wrote: >During socket release, sock_orphan() is called without considering that it >sets sk->sk_wq to NULL. Later, if SO_LINGER is enabled, this leads to a >null pointer dereferenced in virtio_transport_wait_close(). > >Orphan the socket only after transport release. > >While there, reflow the other comment. > >Partially reverts the 'Fixes:' commit. > >KASAN: null-ptr-deref in range [0x0000000000000018-0x000000000000001f] > lock_acquire+0x19e/0x500 > _raw_spin_lock_irqsave+0x47/0x70 > add_wait_queue+0x46/0x230 > virtio_transport_release+0x4e7/0x7f0 > __vsock_release+0xfd/0x490 > vsock_release+0x90/0x120 > __sock_release+0xa3/0x250 > sock_close+0x14/0x20 > __fput+0x35e/0xa90 > __x64_sys_close+0x78/0xd0 > do_syscall_64+0x93/0x1b0 > entry_SYSCALL_64_after_hwframe+0x76/0x7e > >Reported-by: syzbot+9d55b199192a4be7d02c@syzkaller.appspotmail.com >Closes: https://syzkaller.appspot.com/bug?extid=9d55b199192a4be7d02c >Fixes: fcdd2242c023 ("vsock: Keep the binding until socket destruction") >Tested-by: Luigi Leonardi <leonardi@redhat.com> >Signed-off-by: Michal Luczaj <mhal@rbox.co> >--- > net/vmw_vsock/af_vsock.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > >diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >index 075695173648d3a4ecbd04e908130efdbb393b41..85d20891b771a25b8172a163983054a2557f98c1 100644 >--- a/net/vmw_vsock/af_vsock.c >+++ b/net/vmw_vsock/af_vsock.c >@@ -817,20 +817,25 @@ static void __vsock_release(struct sock *sk, int level) > vsk = vsock_sk(sk); > pending = NULL; /* Compiler warning. */ > >- /* When "level" is SINGLE_DEPTH_NESTING, use the nested >- * version to avoid the warning "possible recursive locking >- * detected". When "level" is 0, lock_sock_nested(sk, level) >- * is the same as lock_sock(sk). >+ /* When "level" is SINGLE_DEPTH_NESTING, use the nested version to avoid >+ * the warning "possible recursive locking detected". When "level" is 0, >+ * lock_sock_nested(sk, level) is the same as lock_sock(sk). This comment is formatted “weird” because recently in commit 135ffc7becc8 ("bpf, vsock: Invoke proto::close on close()") we reduced the indentation without touching the comment. Since this is a fix we may have to backport into stable branches without that commit, I would avoid touching it to avoid unnecessary conflicts. The rest LGTM! Thanks for the quick fix and sorry for the late reply but FOSDEM-flu caught me. Thanks, Stefano > */ > lock_sock_nested(sk, level); > >- sock_orphan(sk); >+ /* Indicate to vsock_remove_sock() that the socket is being released and >+ * can be removed from the bound_table. Unlike transport reassignment >+ * case, where the socket must remain bound despite vsock_remove_sock() >+ * being called from the transport release() callback. >+ */ >+ sock_set_flag(sk, SOCK_DEAD); > > if (vsk->transport) > vsk->transport->release(vsk); > else if (sock_type_connectible(sk->sk_type)) > vsock_remove_sock(vsk); > >+ sock_orphan(sk); > sk->sk_shutdown = SHUTDOWN_MASK; > > skb_queue_purge(&sk->sk_receive_queue); > >-- >2.48.1 >
Hi Michal, On Thu, Feb 06, 2025 at 12:06:47AM +0100, Michal Luczaj wrote: >During socket release, sock_orphan() is called without considering that it >sets sk->sk_wq to NULL. Later, if SO_LINGER is enabled, this leads to a >null pointer dereferenced in virtio_transport_wait_close(). > >Orphan the socket only after transport release. > >While there, reflow the other comment. May I ask you why? > >Partially reverts the 'Fixes:' commit. > >KASAN: null-ptr-deref in range [0x0000000000000018-0x000000000000001f] > lock_acquire+0x19e/0x500 > _raw_spin_lock_irqsave+0x47/0x70 > add_wait_queue+0x46/0x230 > virtio_transport_release+0x4e7/0x7f0 > __vsock_release+0xfd/0x490 > vsock_release+0x90/0x120 > __sock_release+0xa3/0x250 > sock_close+0x14/0x20 > __fput+0x35e/0xa90 > __x64_sys_close+0x78/0xd0 > do_syscall_64+0x93/0x1b0 > entry_SYSCALL_64_after_hwframe+0x76/0x7e > >Reported-by: syzbot+9d55b199192a4be7d02c@syzkaller.appspotmail.com >Closes: https://syzkaller.appspot.com/bug?extid=9d55b199192a4be7d02c >Fixes: fcdd2242c023 ("vsock: Keep the binding until socket destruction") >Tested-by: Luigi Leonardi <leonardi@redhat.com> >Signed-off-by: Michal Luczaj <mhal@rbox.co> >--- > net/vmw_vsock/af_vsock.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > >diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >index 075695173648d3a4ecbd04e908130efdbb393b41..85d20891b771a25b8172a163983054a2557f98c1 100644 >--- a/net/vmw_vsock/af_vsock.c >+++ b/net/vmw_vsock/af_vsock.c >@@ -817,20 +817,25 @@ static void __vsock_release(struct sock *sk, int level) > vsk = vsock_sk(sk); > pending = NULL; /* Compiler warning. */ > >- /* When "level" is SINGLE_DEPTH_NESTING, use the nested >- * version to avoid the warning "possible recursive locking >- * detected". When "level" is 0, lock_sock_nested(sk, level) >- * is the same as lock_sock(sk). >+ /* When "level" is SINGLE_DEPTH_NESTING, use the nested version to avoid >+ * the warning "possible recursive locking detected". When "level" is 0, >+ * lock_sock_nested(sk, level) is the same as lock_sock(sk). > */ > lock_sock_nested(sk, level); > >- sock_orphan(sk); >+ /* Indicate to vsock_remove_sock() that the socket is being released and >+ * can be removed from the bound_table. Unlike transport reassignment >+ * case, where the socket must remain bound despite vsock_remove_sock() >+ * being called from the transport release() callback. >+ */ >+ sock_set_flag(sk, SOCK_DEAD); > > if (vsk->transport) > vsk->transport->release(vsk); > else if (sock_type_connectible(sk->sk_type)) > vsock_remove_sock(vsk); > >+ sock_orphan(sk); > sk->sk_shutdown = SHUTDOWN_MASK; > > skb_queue_purge(&sk->sk_receive_queue); > >-- >2.48.1 > Code LGTM! I probably wouldn't have changed that comment because of possible conflicts. Reviewed-by: Luigi Leonardi <leonardi@redhat.com> Thank you, Luigi
On 2/10/25 11:24, Luigi Leonardi wrote: > Hi Michal, > > On Thu, Feb 06, 2025 at 12:06:47AM +0100, Michal Luczaj wrote: >> During socket release, sock_orphan() is called without considering that it >> sets sk->sk_wq to NULL. Later, if SO_LINGER is enabled, this leads to a >> null pointer dereferenced in virtio_transport_wait_close(). >> >> Orphan the socket only after transport release. >> >> While there, reflow the other comment. > May I ask you why? For aesthetic purposes only :) > ... > Code LGTM! > > I probably wouldn't have changed that comment because of possible > conflicts. > > Reviewed-by: Luigi Leonardi <leonardi@redhat.com> Got it, dropping the old comment reflow, thanks. Michal
On 2/10/25 11:15, Stefano Garzarella wrote: > On Thu, Feb 06, 2025 at 12:06:47AM +0100, Michal Luczaj wrote: >> ... >> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >> index 075695173648d3a4ecbd04e908130efdbb393b41..85d20891b771a25b8172a163983054a2557f98c1 100644 >> --- a/net/vmw_vsock/af_vsock.c >> +++ b/net/vmw_vsock/af_vsock.c >> @@ -817,20 +817,25 @@ static void __vsock_release(struct sock *sk, int level) >> vsk = vsock_sk(sk); >> pending = NULL; /* Compiler warning. */ >> >> - /* When "level" is SINGLE_DEPTH_NESTING, use the nested >> - * version to avoid the warning "possible recursive locking >> - * detected". When "level" is 0, lock_sock_nested(sk, level) >> - * is the same as lock_sock(sk). >> + /* When "level" is SINGLE_DEPTH_NESTING, use the nested version to avoid >> + * the warning "possible recursive locking detected". When "level" is 0, >> + * lock_sock_nested(sk, level) is the same as lock_sock(sk). > > This comment is formatted “weird” because recently in commit > 135ffc7becc8 ("bpf, vsock: Invoke proto::close on close()") we reduced > the indentation without touching the comment. > > Since this is a fix we may have to backport into stable branches without > that commit, I would avoid touching it to avoid unnecessary conflicts. > ... I've checked that 135ffc7becc8 was already backported (6.1, 6.6, 6.12) and thought it's safe to do, but I understand your concern, so sending v3 without touching the other comment. Thanks, Michal
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index 075695173648d3a4ecbd04e908130efdbb393b41..85d20891b771a25b8172a163983054a2557f98c1 100644 --- a/net/vmw_vsock/af_vsock.c +++ b/net/vmw_vsock/af_vsock.c @@ -817,20 +817,25 @@ static void __vsock_release(struct sock *sk, int level) vsk = vsock_sk(sk); pending = NULL; /* Compiler warning. */ - /* When "level" is SINGLE_DEPTH_NESTING, use the nested - * version to avoid the warning "possible recursive locking - * detected". When "level" is 0, lock_sock_nested(sk, level) - * is the same as lock_sock(sk). + /* When "level" is SINGLE_DEPTH_NESTING, use the nested version to avoid + * the warning "possible recursive locking detected". When "level" is 0, + * lock_sock_nested(sk, level) is the same as lock_sock(sk). */ lock_sock_nested(sk, level); - sock_orphan(sk); + /* Indicate to vsock_remove_sock() that the socket is being released and + * can be removed from the bound_table. Unlike transport reassignment + * case, where the socket must remain bound despite vsock_remove_sock() + * being called from the transport release() callback. + */ + sock_set_flag(sk, SOCK_DEAD); if (vsk->transport) vsk->transport->release(vsk); else if (sock_type_connectible(sk->sk_type)) vsock_remove_sock(vsk); + sock_orphan(sk); sk->sk_shutdown = SHUTDOWN_MASK; skb_queue_purge(&sk->sk_receive_queue);
During socket release, sock_orphan() is called without considering that it sets sk->sk_wq to NULL. Later, if SO_LINGER is enabled, this leads to a null pointer dereferenced in virtio_transport_wait_close(). Orphan the socket only after transport release. While there, reflow the other comment. Partially reverts the 'Fixes:' commit. KASAN: null-ptr-deref in range [0x0000000000000018-0x000000000000001f] lock_acquire+0x19e/0x500 _raw_spin_lock_irqsave+0x47/0x70 add_wait_queue+0x46/0x230 virtio_transport_release+0x4e7/0x7f0 __vsock_release+0xfd/0x490 vsock_release+0x90/0x120 __sock_release+0xa3/0x250 sock_close+0x14/0x20 __fput+0x35e/0xa90 __x64_sys_close+0x78/0xd0 do_syscall_64+0x93/0x1b0 entry_SYSCALL_64_after_hwframe+0x76/0x7e Reported-by: syzbot+9d55b199192a4be7d02c@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=9d55b199192a4be7d02c Fixes: fcdd2242c023 ("vsock: Keep the binding until socket destruction") Tested-by: Luigi Leonardi <leonardi@redhat.com> Signed-off-by: Michal Luczaj <mhal@rbox.co> --- net/vmw_vsock/af_vsock.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)