netdev - [RFC] nasty corner case in unix_dgram

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]

Date:   Mon, 25 Feb 2019 03:51:21 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     netdev@...r.kernel.org
Subject: [RFC] nasty corner case in unix_dgram_sendmsg()

	Consider the following scenario: sendmsg() with explicit ->msg_name
on unconnected SOCK_DGRAM AF_UNIX socket finds the recepient just about to
die.  We go through
        sk_locked = 0;
        unix_state_lock(other);
restart_locked:
        err = -EPERM;
        if (!unix_may_send(sk, other))
                goto out_unlock;
OK, since other->peer is already NULL
        if (unlikely(sock_flag(other, SOCK_DEAD))) {
Yes, it is.
                /*
                 *      Check with 1003.1g - what should
                 *      datagram error
                 */
                unix_state_unlock(other);
no locks held now...
                sock_put(other);
... and there goes the last reference to other.  We get preempted (to make
the window wider - the race would still exist without preempt, but it
would be much harder to hit).

Memory that used to hold *other gets reused for another AF_UNIX socket,
which gets bound to the same address *and* another thread does connect()
to that address on our socket.  Now unix_peer(sk) is equal to other.
Our thread gets to run again, and
                if (!sk_locked)
                        unix_state_lock(sk);
grabs sk->lock
                err = 0;
                if (unix_peer(sk) == other) {
... yes, it is.  Not the same object, though
                        unix_peer(sk) = NULL;
... and it gets disconnected
                        unix_dgram_peer_wake_disconnect_wakeup(sk, other);

                        unix_state_unlock(sk);

                        unix_dgram_disconnected(sk, other);
... with receive queue purged.

AFAICS, that's bogus.  And easily prevented - all we need here is do
the first sock_put() *after* the "have we just found the peer dead?"
logics, avoiding the memory reuse.

Objections?

PS: unix_dgram_sendmsg() is really much too subtle for its own good -
AFAICS, it *does* avoid blocking operations under sk->lock, but proof
is considerably more complex than one would like it to be...  And
I'm still not convinced that no codepath in it could end up doing
something unpleasant to SOCK_SEQPACKET sockets ;-/