Date:	Fri, 12 Feb 2016 20:47:17 +0000
From:	Ben Hutchings <ben@...adent.org.uk>
To:	Rainer Weikusat <rweikusat@...ileactivedefense.com>
Cc:	Philipp Hahn <pmhahn@...ahn.de>,
	Hannes Frederic Sowa <hannes@...essinduktion.org>,
	Sasha Levin <sasha.levin@...cle.com>,
	"David S. Miller" <davem@...emloft.net>,
	linux-kernel@...r.kernel.org, Karolin Seeger <kseeger@...ba.org>,
	Jason Baron <jbaron@...mai.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Arvid Requate <requate@...vention.de>,
	Stefan Gohmann <gohmann@...vention.de>, netdev@...r.kernel.org
Subject: 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 <ben@...adent.org.uk> writes:
> > On Fri, 2016-02-12 at 13:25 +0000, Rainer Weikusat wrote:
> > > Philipp Hahn <pmhahn@...ahn.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
I say we take off; nuke the site from orbit.  It's the only way to be sure.
Download attachment "signature.asc" of type "application/pgp-signature" (812 bytes)

Powered by blists - more mailing lists