[PATCH net v3 0/2] vsock: null-ptr-deref when SO_LINGER enabled

[PATCH net v3 0/2] vsock: null-ptr-deref when SO_LINGER enabled

[Michal Luczaj]

syzbot pointed out that a recent patching of a use-after-free introduced a
null-ptr-deref. This series fixes the problem and adds a test.

Fixes fcdd2242c023 ("vsock: Keep the binding until socket destruction").

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
Changes in v3:
- Don't touch the old comment [Stefano, Luigi]
- Collect tags [Stefano, Luigi]
- Link to v2: https://lore.kernel.org/r/20250206-vsock-linger-nullderef-v2-0-f8a1f19146f8@rbox.co

Changes in v2:
- Collect tags [Luigi]
- Explain the reason for the explicit set_flag(SOCK_DEAD) [Stefano]
- Link to v1: https://lore.kernel.org/r/20250204-vsock-linger-nullderef-v1-0-6eb1760fa93e@rbox.co

---
Michal Luczaj (2):
      vsock: Orphan socket after transport release
      vsock/test: Add test for SO_LINGER null ptr deref

 net/vmw_vsock/af_vsock.c         |  8 +++++++-
 tools/testing/vsock/vsock_test.c | 41 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 1 deletion(-)
---
base-commit: 011b0335903832facca86cd8ed05d7d8d94c9c76
change-id: 20250203-vsock-linger-nullderef-cbe4402ad306

Best regards,
-- 
Michal Luczaj <mhal@rbox.co>

[PATCH net v3 1/2] vsock: Orphan socket after transport release

[Michal Luczaj]

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.

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>
Reviewed-by: Luigi Leonardi <leonardi@redhat.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/af_vsock.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 075695173648d3a4ecbd04e908130efdbb393b41..53a081d49d28ac1c04e7f8057c8a55e7b73cc131 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -824,13 +824,19 @@ static void __vsock_release(struct sock *sk, int level)
 	 */
 	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

[PATCH net v3 2/2] vsock/test: Add test for SO_LINGER null ptr deref

[Michal Luczaj]

Explicitly close() a TCP_ESTABLISHED (connectible) socket with SO_LINGER
enabled.

As for now, test does not verify if close() actually lingers.
On an unpatched machine, may trigger a null pointer dereference.

Tested-by: Luigi Leonardi <leonardi@redhat.com>
Reviewed-by: Luigi Leonardi <leonardi@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 tools/testing/vsock/vsock_test.c | 41 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index dfff8b288265f96b602cb1bfa0e6dce02f114222..d0f6d253ac72d08a957cb81a3c38fcc72bec5a53 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -1788,6 +1788,42 @@ static void test_stream_connect_retry_server(const struct test_opts *opts)
 	close(fd);
 }
 
+static void test_stream_linger_client(const struct test_opts *opts)
+{
+	struct linger optval = {
+		.l_onoff = 1,
+		.l_linger = 1
+	};
+	int fd;
+
+	fd = vsock_stream_connect(opts->peer_cid, opts->peer_port);
+	if (fd < 0) {
+		perror("connect");
+		exit(EXIT_FAILURE);
+	}
+
+	if (setsockopt(fd, SOL_SOCKET, SO_LINGER, &optval, sizeof(optval))) {
+		perror("setsockopt(SO_LINGER)");
+		exit(EXIT_FAILURE);
+	}
+
+	close(fd);
+}
+
+static void test_stream_linger_server(const struct test_opts *opts)
+{
+	int fd;
+
+	fd = vsock_stream_accept(VMADDR_CID_ANY, opts->peer_port, NULL);
+	if (fd < 0) {
+		perror("accept");
+		exit(EXIT_FAILURE);
+	}
+
+	vsock_wait_remote_close(fd);
+	close(fd);
+}
+
 static struct test_case test_cases[] = {
 	{
 		.name = "SOCK_STREAM connection reset",
@@ -1943,6 +1979,11 @@ static struct test_case test_cases[] = {
 		.run_client = test_stream_connect_retry_client,
 		.run_server = test_stream_connect_retry_server,
 	},
+	{
+		.name = "SOCK_STREAM SO_LINGER null-ptr-deref",
+		.run_client = test_stream_linger_client,
+		.run_server = test_stream_linger_server,
+	},
 	{},
 };
 

-- 
2.48.1

Re: [PATCH net v3 1/2] vsock: Orphan socket after transport release

[Stefano Garzarella]

On Mon, Feb 10, 2025 at 01:15:00PM +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.
>
>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>
>Reviewed-by: Luigi Leonardi <leonardi@redhat.com>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> net/vmw_vsock/af_vsock.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index 075695173648d3a4ecbd04e908130efdbb393b41..53a081d49d28ac1c04e7f8057c8a55e7b73cc131 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -824,13 +824,19 @@ static void __vsock_release(struct sock *sk, int level)
> 	 */
> 	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
>

Re: [PATCH net v3 0/2] vsock: null-ptr-deref when SO_LINGER enabled

[Jakub Kicinski]

On Mon, 10 Feb 2025 13:14:59 +0100 Michal Luczaj wrote:
> Fixes fcdd2242c023 ("vsock: Keep the binding until socket destruction").

I don't think it's a good idea to put Fixes tags into the cover letters.
Not sure what purpose it'd serve.

Re: [PATCH net v3 0/2] vsock: null-ptr-deref when SO_LINGER enabled

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 10 Feb 2025 13:14:59 +0100 you wrote:
> syzbot pointed out that a recent patching of a use-after-free introduced a
> null-ptr-deref. This series fixes the problem and adds a test.
> 
> Fixes fcdd2242c023 ("vsock: Keep the binding until socket destruction").
> 
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> 
> [...]

Here is the summary with links:
  - [net,v3,1/2] vsock: Orphan socket after transport release
    https://git.kernel.org/netdev/net/c/78dafe1cf3af
  - [net,v3,2/2] vsock/test: Add test for SO_LINGER null ptr deref
    https://git.kernel.org/netdev/net/c/440c9d488705

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net v3 0/2] vsock: null-ptr-deref when SO_LINGER enabled

[Michal Luczaj]

On 2/13/25 05:02, Jakub Kicinski wrote:
> On Mon, 10 Feb 2025 13:14:59 +0100 Michal Luczaj wrote:
>> Fixes fcdd2242c023 ("vsock: Keep the binding until socket destruction").
> 
> I don't think it's a good idea to put Fixes tags into the cover letters.
> Not sure what purpose it'd serve.

I was trying to say it's a "follow up" to a very recent (at least in the
vsock context) patch-gone-wrong. But I did not intend to make this a tag;
it's not a "Fixes:" with a colon :)

Anyway, if that puts too much detail into the cover letter, I'll refrain
from doing so.

Re: [PATCH net v3 0/2] vsock: null-ptr-deref when SO_LINGER enabled

[Jakub Kicinski]

On Thu, 13 Feb 2025 11:15:43 +0100 Michal Luczaj wrote:
> On 2/13/25 05:02, Jakub Kicinski wrote:
> > On Mon, 10 Feb 2025 13:14:59 +0100 Michal Luczaj wrote:  
> >> Fixes fcdd2242c023 ("vsock: Keep the binding until socket destruction").  
> > 
> > I don't think it's a good idea to put Fixes tags into the cover letters.
> > Not sure what purpose it'd serve.  
> 
> I was trying to say it's a "follow up" to a very recent (at least in the
> vsock context) patch-gone-wrong. But I did not intend to make this a tag;
> it's not a "Fixes:" with a colon :)
> 
> Anyway, if that puts too much detail into the cover letter, I'll refrain
> from doing so.

Never too much detail :) But if it's informative and for humans I'd
recommend weaving it into the explanation or adding some words around.
Sorry for the nit picking.

Re: [PATCH net v3 0/2] vsock: null-ptr-deref when SO_LINGER enabled

[Michal Luczaj]

On 2/13/25 16:24, Jakub Kicinski wrote:
> On Thu, 13 Feb 2025 11:15:43 +0100 Michal Luczaj wrote:
>> On 2/13/25 05:02, Jakub Kicinski wrote:
>>> On Mon, 10 Feb 2025 13:14:59 +0100 Michal Luczaj wrote:  
>>>> Fixes fcdd2242c023 ("vsock: Keep the binding until socket destruction").  
>>>
>>> I don't think it's a good idea to put Fixes tags into the cover letters.
>>> Not sure what purpose it'd serve.  
>>
>> I was trying to say it's a "follow up" to a very recent (at least in the
>> vsock context) patch-gone-wrong. But I did not intend to make this a tag;
>> it's not a "Fixes:" with a colon :)
>>
>> Anyway, if that puts too much detail into the cover letter, I'll refrain
>> from doing so.
> 
> Never too much detail :) But if it's informative and for humans I'd
> recommend weaving it into the explanation or adding some words around.
> Sorry for the nit picking.

It's ok, I think I get your point. Even simply using a reference[1] would
probably be less confusing for eyes and brains.