netdev - Re: 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-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 13 Jul 2020 14:31:18 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Thomas Habets <thomas@...ets.se>, netdev@...r.kernel.org,
        Ralf Baechle <ralf@...ux-mips.org>
Subject: Re: PATCH ax25: Don't hold skb lock while doing blocking read

patch title has a typo : there is no skb lock.

On 7/9/20 12:49 PM, Thomas Habets wrote:
> 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.

Again, do not mix skb and socket ;)

> 
> 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) {

This would be racy, but maybe we do not care.

>                 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);

Or you could use a smaller change and make this look like net/x25/af_x25.c ?

In any case, this will open gates for syzbot fun I am sure ;)