Subject: [RFC] nasty corner case in unix_dgram_sendmsg()

From: Al Viro <[email-masked]>
Date: Mon, 25 Feb 2019 03:51:21 +0000

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