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

On Fri, 2016-02-12 at 20:17 +0000, Rainer Weikusat wrote:

Ben Hutchings [off-list ref] writes:

quoted

On Fri, 2016-02-12 at 13:25 +0000, Rainer Weikusat wrote:

quoted

Philipp Hahn [off-list ref] writes:

quoted

Hello Rainer,

Am 11.02.2016 um 20:37 schrieb Rainer Weikusat:

quoted

The unix_dgram_sendmsg routine use the following test

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

[...]

quoted

quoted

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

[...]

quoted

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.

quoted

quoted

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
I say we take off; nuke the site from orbit.  It's the only way to be sure.