| 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: <xafz4xrgpi5m3wedkbhfx6qoqbbpogryxycrvawwzerge3l4t3@d6r6jbnpiyhs>
Date: Mon, 10 Mar 2025 16:24:38 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: Michal Luczaj <mhal@...x.co>
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 Fri, Mar 07, 2025 at 10:49:52AM +0100, Michal Luczaj wrote:
>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:
Yes, that's a good idea, I had something similar in mind, but reusing
unsent_bytes() sounds great to me.
The only problem I see is that in the driver in the guest, the packets
are put in the virtqueue and the variable is decremented only when the
host sends us an interrupt to say that it has copied the packets and
then the guest can free the buffer. Is this okay to consider this as
sending?
I think so, though it's honestly not clear to me if instead by sending
we should consider when the driver copies the bytes into the virtqueue,
but that doesn't mean they were really sent. We should compare it to
what the network devices or AF_UNIX do.
>
>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;
This is not implemented by all transports, so we should handle it in
some way (check the pointer or implement in all transports).
>+
>+ 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);
mmm, great, so on shutdown we never supported SO_LINGER, right?
> }
> }
>
>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.
Did you test with loopback or virtio-vsock with a VM?
BTW this approach LGTM!
Thanks,
Stefano
Powered by blists - more mailing lists