netdev - Re: [PATCH] af_unix: fix holding spinlock in oob handling

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:   Thu, 12 Aug 2021 09:53:33 +0200
From:   Eric Dumazet <edumazet@...gle.com>
To:     Rao Shoaib <Rao.Shoaib@...cle.com>
Cc:     netdev <netdev@...r.kernel.org>, Jakub Kicinski <kuba@...nel.org>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH] af_unix: fix holding spinlock in oob handling

On Thu, Aug 12, 2021 at 12:07 AM Rao Shoaib <Rao.Shoaib@...cle.com> wrote:
>
> From: Rao Shoaib <rao.shoaib@...cle.com>
>
> syzkaller found that OOB code was holding spinlock
> while calling a function in which it could sleep.
>
> Reported-by: syzbot+8760ca6c1ee783ac4abd@...kaller.appspotmail.com
>
> Fixes: 314001f0bf92 ("af_unix: Add OOB support")
>
> Signed-off-by: Rao Shoaib <rao.shoaib@...cle.com>
> ---

Please do not add these empty lines.

Fixes: ...
Reported-by: ...
Signed-off-by: ...

Also you might take a look at queue_oob()

1)  Setting skb->len tp 1 should not be needed, skb_put() already does that
2) After unix_state_lock(other); we probably need to check status of
the other socket.
3) Some skb_free() calls should have been consume_skb()

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index ec02e70a549b42f6c102253508c48426a13f7bc4..0c27e2976f9d234ca3bb131731375bc51a056846
100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1908,7 +1908,6 @@ static int queue_oob(struct socket *sock, struct
msghdr *msg, struct sock *other
                return err;

        skb_put(skb, 1);
-       skb->len = 1;
        err = skb_copy_datagram_from_iter(skb, 0, &msg->msg_iter, 1);

        if (err) {
@@ -1917,11 +1916,17 @@ static int queue_oob(struct socket *sock,
struct msghdr *msg, struct sock *other
        }

        unix_state_lock(other);
+       if (sock_flag(other, SOCK_DEAD) ||
+           (other->sk_shutdown & RCV_SHUTDOWN)) {
+               unix_state_unlock(other);
+               kfree_skb(skb);
+               return -EPIPE;
+       }
        maybe_add_creds(skb, sock, other);
        skb_get(skb);

        if (ousk->oob_skb)
-               kfree_skb(ousk->oob_skb);
+               consume_skb(ousk->oob_skb);

        ousk->oob_skb = skb;