| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <df2d51fd-03e7-477f-8aea-938446f47864@rbox.co>
Date: Fri, 7 Mar 2025 10:49:52 +0100
From: Michal Luczaj <mhal@...x.co>
To: Stefano Garzarella <sgarzare@...hat.com>
Cc: "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
netdev@...r.kernel.org
Subject: Re: [PATCH net 2/2] vsock/test: Add test for SO_LINGER null ptr deref
On 2/10/25 11:18, Stefano Garzarella wrote:
> On Wed, Feb 05, 2025 at 12:20:56PM +0100, Michal Luczaj wrote:
>> On 2/4/25 11:48, Stefano Garzarella wrote:
>>> On Tue, Feb 04, 2025 at 01:29:53AM +0100, Michal Luczaj wrote:
>>>> ...
>>>> +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);
>>>> + }
>>>
>>> Since we are testing SO_LINGER, will also be nice to check if it's
>>> working properly, since one of the fixes proposed could break it.
>>>
>>> To test, we may set a small SO_VM_SOCKETS_BUFFER_SIZE on the receive
>>> side and try to send more than that value, obviously without reading
>>> anything into the receiver, and check that close() here, returns after
>>> the timeout we set in .l_linger.
>>
>> I may be doing something wrong, but (at least for loopback transport) ...
>
> Also with VMs is the same, I think virtio_transport_wait_close() can be
> improved to check if everything is sent, avoiding to wait.
What kind of improvement do you have in mind?
I've tried modifying the loop to make close()/shutdown() linger until
unsent_bytes() == 0. No idea if this is acceptable:
diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
index 9e85424c8343..bd8b88d70423 100644
--- a/include/net/af_vsock.h
+++ b/include/net/af_vsock.h
@@ -221,6 +221,7 @@ void vsock_for_each_connected_socket(struct vsock_transport *transport,
void (*fn)(struct sock *sk));
int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk);
bool vsock_find_cid(unsigned int cid);
+void vsock_linger(struct sock *sk, long timeout);
/**** TAP ****/
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 7e3db87ae433..2cf7571e94da 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1013,6 +1013,25 @@ static int vsock_getname(struct socket *sock,
return err;
}
+void vsock_linger(struct sock *sk, long timeout)
+{
+ if (timeout) {
+ DEFINE_WAIT_FUNC(wait, woken_wake_function);
+ ssize_t (*unsent)(struct vsock_sock *vsk);
+ struct vsock_sock *vsk = vsock_sk(sk);
+
+ add_wait_queue(sk_sleep(sk), &wait);
+ unsent = vsk->transport->unsent_bytes;
+
+ do {
+ if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
+ break;
+ } while (!signal_pending(current) && timeout);
+
+ remove_wait_queue(sk_sleep(sk), &wait);
+ }
+}
+
static int vsock_shutdown(struct socket *sock, int mode)
{
int err;
@@ -1056,6 +1075,8 @@ static int vsock_shutdown(struct socket *sock, int mode)
if (sock_type_connectible(sk->sk_type)) {
sock_reset_flag(sk, SOCK_DONE);
vsock_send_shutdown(sk, mode);
+ if (sock_flag(sk, SOCK_LINGER))
+ vsock_linger(sk, sk->sk_lingertime);
}
}
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 7f7de6d88096..9230b8358ef2 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -1192,23 +1192,6 @@ static void virtio_transport_remove_sock(struct vsock_sock *vsk)
vsock_remove_sock(vsk);
}
-static void virtio_transport_wait_close(struct sock *sk, long timeout)
-{
- if (timeout) {
- DEFINE_WAIT_FUNC(wait, woken_wake_function);
-
- add_wait_queue(sk_sleep(sk), &wait);
-
- do {
- if (sk_wait_event(sk, &timeout,
- sock_flag(sk, SOCK_DONE), &wait))
- break;
- } while (!signal_pending(current) && timeout);
-
- remove_wait_queue(sk_sleep(sk), &wait);
- }
-}
-
static void virtio_transport_cancel_close_work(struct vsock_sock *vsk,
bool cancel_timeout)
{
@@ -1279,7 +1262,7 @@ static bool virtio_transport_close(struct vsock_sock *vsk)
(void)virtio_transport_shutdown(vsk, SHUTDOWN_MASK);
if (sock_flag(sk, SOCK_LINGER) && !(current->flags & PF_EXITING))
- virtio_transport_wait_close(sk, sk->sk_lingertime);
+ vsock_linger(sk, sk->sk_lingertime);
if (sock_flag(sk, SOCK_DONE)) {
return true;
This works, but I find it difficult to test without artificially slowing
the kernel down. It's a race against workers as they quite eagerly do
virtio_transport_consume_skb_sent(), which decrements vvs->bytes_unsent.
I've tried reducing SO_VM_SOCKETS_BUFFER_SIZE as you've suggested, but
send() would just block until peer had available space.
Thanks,
Michal
Powered by blists - more mailing lists