netdev - PATCH ax25: Don't hold skb lock while doing blocking read

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+kHd+fh2UW4FvxL4v_rd8mQ=avuzHxb=n_ogpVwCMSXVTCeAg@mail.gmail.com>
Date:   Thu, 9 Jul 2020 20:49:41 +0100
From:   Thomas Habets <thomas@...ets.se>
To:     netdev@...r.kernel.org, Ralf Baechle <ralf@...ux-mips.org>
Subject: PATCH ax25: Don't hold skb lock while doing blocking read

Here's a test program that illustrates the problem:
https://github.com/ThomasHabets/radiostuff/blob/master/ax25/axftp/examples/client_lockcheck.cc

Before this patch, this hangs, because the read(2) blocks the
write(2).

I see that calling skb_recv_datagram without this lock is done in
pep_sock_accept() and atalk_recvmsg() and others, which is what makes
me think it's safe to do here too. But I'm far from an expert on skb
lock semantics.

I see some other socket types are also locking during
read. E.g. qrtr_recvmsg. Maybe they need to be fixed too.

Before:
strace -f -eread,write ./examples/client_lockcheck M0THC-9 M0THC-0 M0THC-2
strace: Process 3888 attached
[pid  3888] read(3,  <unfinished ...>
[pid  3887] write(3, "hello world", 11
[hang]

After:
strace -f -eread,write ./examples/client_lockcheck M0THC-9 M0THC-0 M0THC-2
strace: Process 2433 attached
[pid  2433] read(3,  <unfinished ...>
[pid  2432] write(3, "hello world", 11) = 11
[pid  2433] <... read resumed> "yo", 1000) = 2
[pid  2433] write(1, "yo\n", 3yo
)         = 3
[successful exit]


diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index fd91cd34f25e..378ee132e4d0 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -1617,22 +1617,22 @@ static int ax25_recvmsg(struct socket *sock,
struct msghdr *msg, size_t size,
        int copied;
        int err = 0;

-       lock_sock(sk);
        /*
         *      This works for seqpacket too. The receiver has ordered the
         *      queue for us! We do one quick check first though
         */
        if (sk->sk_type == SOCK_SEQPACKET && sk->sk_state != TCP_ESTABLISHED) {
                err =  -ENOTCONN;
-               goto out;
+               goto out_nolock;
        }

        /* Now we can treat all alike */
        skb = skb_recv_datagram(sk, flags & ~MSG_DONTWAIT,
                                flags & MSG_DONTWAIT, &err);
        if (skb == NULL)
-               goto out;
+               goto out_nolock;

+       lock_sock(sk);
        if (!sk_to_ax25(sk)->pidincl)
                skb_pull(skb, 1);               /* Remove PID */

@@ -1677,6 +1677,7 @@ static int ax25_recvmsg(struct socket *sock,
struct msghdr *msg, size_t size,

 out:
        release_sock(sk);
+out_nolock:

        return err;
 }