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

Message-ID: <20190226062817.GA17962@ZenIV.linux.org.uk>
Date:   Tue, 26 Feb 2019 06:28:17 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Rainer Weikusat <rweikusat@...ileactivedefense.com>,
        Jason Baron <jbaron@...mai.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [RFC] nasty corner case in unix_dgram_sendmsg()

On Mon, Feb 25, 2019 at 03:51:21AM +0000, Al Viro wrote:

> 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...

Several questions about the whole peer_wake mechanism:

1) why do we even bother with that for SOCK_SEQPACKET?  AFAICS, there
we do *not* hit unix_wait_for_peer() on send - it's conditional upon
        if (other != sk &&
            unlikely(unix_peer(other) != sk && unix_recvq_full(other))) {
after we'd checked that other is non-NULL and not dead.  For seqpacket
we start with both ends of connection having unix_peer() pointing to
each other and only changed to NULL when one of the ends gets closed.
The socket we'd passed to sendmsg is obviously not dead yet, and we'd
verified that other isn't dead either.  So unix_peer(other) must be
equal to sk and we don't go into that if.

In unix_dgram_poll() we might get to unix_dgram_peer_wake_me() for
seqpacket, but only in case when other is dead.  In that case
unix_dgram_peer_wake_me() will insert our peer_wake into queue,
see SOCK_DEAD, remove peer_wake from queue and return false.
AFAICS, that serves no purpose whatsoever...

2) the logics in sendmsg is very odd:
	* if we'd detected n:1 send *and* found that we need to
wait, do so (not using the peer_wake - other's peer_wait is not
going away).  No questions there.
	* if we'd detected n:1 send *and* found that we need to
wait, but don't have any remaining timeout, we lock both ends
and check if unix_peer(sk) is (now) not equal to other, either
because it never had or because we'd been hit by connect(2) while
we'd been doing the locking.  In that case we fail with -EAGAIN.
Fair enough, but
	* if after relocking we see that unix_peer(sk) now
is equal to other, we arrange for wakeup forwarding from other's
peer_wait *and* if that has (likely) succeeded we fail with -EAGAIN.
Huh?  What's the point?  The only thing that depends upon that
wakeup forwarding is poll, and _that_ will set the forwarding up
on its own.
	* if we'd failed (either because other is dead now or
because its queue is not full), we go back to restart_locked.
If it's dead, we'll sod off with ECONNREFUSED, if it's not
full anymore, we'll send the damn thing.

All of that comes at the cost of considerable headache in
unix_dgram_sendmsg() - flag-conditional locking, etc.  Why do
we bother?  What's wrong with simple "n:1 case detected, queue
full, no timeout left, return -EAGAIN and be done with that"?

IDGI...  Am I missing something subtle going on here?

I understand what peer_wake is for, and the poll side of things
is fine; it's sendmsg one that looks weird.  What's going on
there?