[net] af_unix: Guard against other == sk in unix_dgram_sendmsg

The unix_dgram_sendmsg routine use the following test

if (unlikely(unix_peer(other) != sk && unix_recvq_full(other))) {

to determine if sk and other are in an n:1 association (either
established via connect or by using sendto to send messages to an
unrelated socket identified by address). This isn't correct as the
specified address could have been bound to the sending socket itself or
because this socket could have been connected to itself by the time of
the unix_peer_get but disconnected before the unix_state_lock(other). In
both cases, the if-block would be entered despite other == sk which
might either block the sender unintentionally or lead to trying to unlock
the same spin lock twice for a non-blocking send. Add a other != sk
check to guard against this.

Fixes: 7d267278a9ec ("unix: avoid use-after-free in ep_remove_wait_queue")
Reported-By: Philipp Hahn <pmhahn@pmhahn.de>
Signed-off-by: Rainer Weikusat <rweikusat@mobileactivedefense.com>
---

Hello Rainer,

Am 11.02.2016 um 20:37 schrieb Rainer Weikusat:
> The unix_dgram_sendmsg routine use the following test
> 
> if (unlikely(unix_peer(other) != sk && unix_recvq_full(other))) {
> 
> to determine if sk and other are in an n:1 association (either
> established via connect or by using sendto to send messages to an
> unrelated socket identified by address). This isn't correct as the
> specified address could have been bound to the sending socket itself or
> because this socket could have been connected to itself by the time of
> the unix_peer_get but disconnected before the unix_state_lock(other). In
> both cases, the if-block would be entered despite other == sk which
> might either block the sender unintentionally or lead to trying to unlock
> the same spin lock twice for a non-blocking send. Add a other != sk
> check to guard against this.
> 
> Fixes: 7d267278a9ec ("unix: avoid use-after-free in ep_remove_wait_queue")
> Reported-By: Philipp Hahn <pmhahn@pmhahn.de>
> Signed-off-by: Rainer Weikusat <rweikusat@mobileactivedefense.com>
> ---
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 29be035..f1ca279 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1781,7 +1781,12 @@ restart_locked:
>  			goto out_unlock;
>  	}
>  
> -	if (unlikely(unix_peer(other) != sk && unix_recvq_full(other))) {
> +	/* other == sk && unix_peer(other) != sk if
> +	 * - unix_peer(sk) == NULL, destination address bound to sk
> +	 * - unix_peer(sk) == sk by time of get but disconnected before lock
> +	 */
> +	if (other != sk &&
> +	    unlikely(unix_peer(other) != sk && unix_recvq_full(other))) {
>  		if (timeo) {
>  			timeo = unix_wait_for_peer(other, timeo);
>  
> 

After applying that patch at least my machine running the samba test no
longer crashes. So you might add
Tested-by: Philipp Hahn <pmhahn@pmhahn.de>

Thanks for looking it that issues.

Philipp

Philipp Hahn <pmhahn@pmhahn.de> writes:

> Hello Rainer,
>
> Am 11.02.2016 um 20:37 schrieb Rainer Weikusat:
>> The unix_dgram_sendmsg routine use the following test
>> 
>> if (unlikely(unix_peer(other) != sk && unix_recvq_full(other))) {

[...]

>> This isn't correct as the> specified address could have been bound to
>> the sending socket itself

[...]

> After applying that patch at least my machine running the samba test no
> longer crashes.

There's a possible gotcha in there: Send-to-self used to be limited by
the queue limit. But the rationale for that (IIRC) was that someone
could keep using newly created sockets to queue ever more data to a
single, unrelated receiver. I don't think this should apply when
receiving and sending sockets are identical. But that's just my
opinion. The other option would be to avoid the unix_state_double_lock
for sk == other. I'd be willing to change this accordingly if someone
thinks the queue limit should apply to send-to-self.

On Fri, 2016-02-12 at 13:25 +0000, Rainer Weikusat wrote:
> Philipp Hahn <pmhahn@pmhahn.de> writes:
> 
> > Hello Rainer,
> > 
> > Am 11.02.2016 um 20:37 schrieb Rainer Weikusat:
> > > The unix_dgram_sendmsg routine use the following test
> > > 
> > > if (unlikely(unix_peer(other) != sk && unix_recvq_full(other))) {
> 
> [...]
> 
> > > This isn't correct as the> specified address could have been bound to
> > > the sending socket itself
> 
> [...]
> 
> > After applying that patch at least my machine running the samba test no
> > longer crashes.
> 
> There's a possible gotcha in there: Send-to-self used to be limited by
> the queue limit. But the rationale for that (IIRC) was that someone
> could keep using newly created sockets to queue ever more data to a
> single, unrelated receiver. I don't think this should apply when
> receiving and sending sockets are identical. But that's just my
> opinion. The other option would be to avoid the unix_state_double_lock
> for sk == other.

Given that unix_state_double_lock() already handles sk == other, I'm
not sure why you think it needs to be avoided.

> I'd be willing to change this accordingly if someone
> thinks the queue limit should apply to send-to-self.

If we don't check the queue limit here, does anything else prevent the
queue growing to the point it's a DoS?

Ben.

Ben Hutchings <ben@decadent.org.uk> writes:
> On Fri, 2016-02-12 at 13:25 +0000, Rainer Weikusat wrote:
>> Philipp Hahn <pmhahn@pmhahn.de> writes:
>> > Hello Rainer,
>> > 
>> > Am 11.02.2016 um 20:37 schrieb Rainer Weikusat:
>> > > The unix_dgram_sendmsg routine use the following test
>> > > 
>> > > if (unlikely(unix_peer(other) != sk && unix_recvq_full(other))) {
>> 
>> [...]
>> 
>> > > This isn't correct as the> specified address could have been bound to
>> > > the sending socket itself
>> 
>> [...]
>> 
>> > After applying that patch at least my machine running the samba test no
>> > longer crashes.
>> 
>> There's a possible gotcha in there: Send-to-self used to be limited by
>> the queue limit. But the rationale for that (IIRC) was that someone
>> could keep using newly created sockets to queue ever more data to a
>> single, unrelated receiver. I don't think this should apply when
>> receiving and sending sockets are identical. But that's just my
>> opinion. The other option would be to avoid the unix_state_double_lock
>> for sk == other.
>
> Given that unix_state_double_lock() already handles sk == other, I'm
> not sure why you think it needs to be avoided.

Because the whole complication of restarting the operation after locking
both sk and other because other had to be unlocked before calling
unix_state_double_lock is useless for this case: As other == sk, there's
no reason to drop the lock on it which guarantees that the result of all
the earlier checks is still valid: If the -EAGAIN condition is not true,
execution can just continue.

>> I'd be willing to change this accordingly if someone
>> thinks the queue limit should apply to send-to-self.
>
> If we don't check the queue limit here, does anything else prevent the
> queue growing to the point it's a DoS?

The max_dgram_qlen limit exists specifically to prevent someone sending
'a lot' of messages to a socket unrelated to it by repeatedly creating a
socket, sending as many messages as the send buffer size will allow,
closing the socket, creating a new socket, ..., cf

http://netdev.vger.kernel.narkive.com/tcZIFJeC/get-rid-of-proc-sys-net-unix-max-dgram-qlen#post4
(first copy I found)

This 'attack' will obviously not work very well when sending and
receiving socket are identical.

On Fri, 2016-02-12 at 20:17 +0000, Rainer Weikusat wrote:
> Ben Hutchings <ben@decadent.org.uk> writes:
> > On Fri, 2016-02-12 at 13:25 +0000, Rainer Weikusat wrote:
> > > Philipp Hahn <pmhahn@pmhahn.de> writes:
> > > > Hello Rainer,
> > > > 
> > > > Am 11.02.2016 um 20:37 schrieb Rainer Weikusat:
> > > > > The unix_dgram_sendmsg routine use the following test
> > > > > 
> > > > > if (unlikely(unix_peer(other) != sk && unix_recvq_full(other))) {
> > > 
> > > [...]
> > > 
> > > > > This isn't correct as the> specified address could have been bound to
> > > > > the sending socket itself
> > > 
> > > [...]
> > > 
> > > > After applying that patch at least my machine running the samba test no
> > > > longer crashes.
> > > 
> > > There's a possible gotcha in there: Send-to-self used to be limited by
> > > the queue limit. But the rationale for that (IIRC) was that someone
> > > could keep using newly created sockets to queue ever more data to a
> > > single, unrelated receiver. I don't think this should apply when
> > > receiving and sending sockets are identical. But that's just my
> > > opinion. The other option would be to avoid the unix_state_double_lock
> > > for sk == other.
> > 
> > Given that unix_state_double_lock() already handles sk == other, I'm
> > not sure why you think it needs to be avoided.
> 
> Because the whole complication of restarting the operation after locking
> both sk and other because other had to be unlocked before calling
> unix_state_double_lock is useless for this case: As other == sk, there's
> no reason to drop the lock on it which guarantees that the result of all
> the earlier checks is still valid: If the -EAGAIN condition is not true,
> execution can just continue.

Well of course it's useless, but it's also harmless.  If we really
wanted to optimise this we could also skip unlocking if other < sk.

> > > I'd be willing to change this accordingly if someone
> > > thinks the queue limit should apply to send-to-self.
> > 
> > If we don't check the queue limit here, does anything else prevent the
> > queue growing to the point it's a DoS?
> 
> The max_dgram_qlen limit exists specifically to prevent someone sending
> 'a lot' of messages to a socket unrelated to it by repeatedly creating a
> socket, sending as many messages as the send buffer size will allow,
> closing the socket, creating a new socket, ..., cf
> 
> http://netdev.vger.kernel.narkive.com/tcZIFJeC/get-rid-of-proc-sys-net-unix-max-dgram-qlen#post4
> (first copy I found)
> 
> This 'attack' will obviously not work very well when sending and
> receiving socket are identical.

It looked to me like the queue length was the only limit here, as I was
looking in vain for a charge to the receiving socket's memory.
However, to answer my own question, AF_UNIX skbs are always charged to
the sending socket (which is the same thing in this case, but still
affects where the buffer limit is applied).

Ben.

Ben Hutchings <ben@decadent.org.uk> writes:
> On Fri, 2016-02-12 at 20:17 +0000, Rainer Weikusat wrote:

[...]

>>>> I don't think this should apply when
>>>> receiving and sending sockets are identical. But that's just my
>>>> opinion. The other option would be to avoid the unix_state_double_lock
>>>> for sk == other.
>>> 
>>> Given that unix_state_double_lock() already handles sk == other, I'm
>>> not sure why you think it needs to be avoided.
>> 
>> Because the whole complication of restarting the operation after locking
>> both sk and other because other had to be unlocked before calling
>> unix_state_double_lock is useless for this case:

[...]

> Well of course it's useless, but it's also harmless.  

As is adding a

for (i = 0; i < 1000000; ++i);

between any two statements. And this isn't even entirely true as the
pointless double-lock will then require "did we pointlessly
doube-lock" checks elsewhere. I think it should be possible to do this
in a simpler way by not pointlessly double-locking (this may be
wrong but it's worth a try).

> If we really wanted to optimise this we could also skip unlocking if
> other < sk.

I wouldn't want to hardcode assumptions about the unix_state_double_lock
algorithm in functions using it.

From: Rainer Weikusat <rweikusat@mobileactivedefense.com>
Date: Thu, 11 Feb 2016 19:37:27 +0000

> The unix_dgram_sendmsg routine use the following test
> 
> if (unlikely(unix_peer(other) != sk && unix_recvq_full(other))) {
> 
> to determine if sk and other are in an n:1 association (either
> established via connect or by using sendto to send messages to an
> unrelated socket identified by address). This isn't correct as the
> specified address could have been bound to the sending socket itself or
> because this socket could have been connected to itself by the time of
> the unix_peer_get but disconnected before the unix_state_lock(other). In
> both cases, the if-block would be entered despite other == sk which
> might either block the sender unintentionally or lead to trying to unlock
> the same spin lock twice for a non-blocking send. Add a other != sk
> check to guard against this.
> 
> Fixes: 7d267278a9ec ("unix: avoid use-after-free in ep_remove_wait_queue")
> Reported-By: Philipp Hahn <pmhahn@pmhahn.de>
> Signed-off-by: Rainer Weikusat <rweikusat@mobileactivedefense.com>

Also applied and queued up for -stable, thanks.