netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: netdev@vger.kernel.org
Subject: [RFC] nasty corner case in unix_dgram_sendmsg()
Date: Mon, 25 Feb 2019 03:51:21 +0000	[thread overview]
Message-ID: <20190225035121.GH2217@ZenIV.linux.org.uk> (raw)

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

             reply	other threads:[~2019-02-25  3:51 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-25  3:51 Al Viro [this message]
2019-02-26  6:28 ` [RFC] nasty corner case in unix_dgram_sendmsg() Al Viro
2019-02-26  6:38   ` Al Viro
2019-02-26 15:31     ` Rainer Weikusat
2019-02-26 19:03       ` Al Viro
2019-02-26 20:35         ` Jason Baron
2019-02-26 23:59           ` Al Viro
2019-02-27 16:45             ` Jason Baron

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190225035121.GH2217@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).