netdev - Re: [PATCH -next v2] unix stream: Fix use-after-free crashes

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: <1315362912.3400.51.camel@edumazet-laptop>
Date:	Wed, 07 Sep 2011 04:35:12 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Tim Chen <tim.c.chen@...ux.intel.com>
Cc:	"Yan, Zheng" <zheng.z.yan@...el.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"sfr@...b.auug.org.au" <sfr@...b.auug.org.au>,
	"jirislaby@...il.com" <jirislaby@...il.com>,
	"sedat.dilek@...il.com" <sedat.dilek@...il.com>, alex.shi@...el.com
Subject: Re: [PATCH -next v2] unix stream: Fix use-after-free crashes

Le mardi 06 septembre 2011 à 15:08 -0700, Tim Chen a écrit :
> On Tue, 2011-09-06 at 22:19 +0200, Eric Dumazet wrote:
> 
> > 
> > unless scm_ref really means scm_noref ?
> > 
> > I really hate this patch. I mean it. 
> > 
> > I read it 10 times, spent 2 hours and still dont understand it.
> > 
> 
> Eric,
> 
> I've tried another patch to fix my original one.  I've used a boolean
> ref_avail to indicate if there is an outstanding ref to scm not yet
> encoded into the skb.  Hopefully the logic is clearer in this new patch.
> 
> > 
> > As I said, we should revert the buggy patch, and rewrite a performance
> > fix from scratch, with not a single get_pid()/put_pid() in fast path.
> > 
> > read()/write() on AF_UNIX sockets should not use a single
> > get_pid()/put_pid().
> > 
> > This is a serious regression we should fix at 100%, not 50% or even 75%,
> > adding serious bugs.
> 
> That will be ideal if there is another way to fix it 100%, other than reverting
> commit 7361c36c.  Probably if there is some way we know beforehand that 
> both sender and receiver share the same pid, which is quite common, a
> lot of these pid code can be bypassed. 
> 

Let me restate : Its should be obvious to fix the performance hit for
good.

If namespaces are not used (CONFIG_PID_NS is not set), we can use the
old code, prior to commit 7361c36c : store pid/uid/gid in skb->cb[]

But more generally, when a write() is done on AF_UNIX socket, we pass a
NULL siocb->scm to unix_{dgram|stream}_sendmsg()

if (NULL == siocb->scm)
	siocb->scm = &tmp_scm;

There is no need in this case to copy in each skb->cb, pointers to
struct pid and struct cred with their atomic reference being changed in
the sender and receiver.

We try to remove _all_ atomic ops on refcounts not only because atomic
ops are expensive by themselves, but also because of the cache line ping
pongs. 


> Tim
> 
> 
> Signed-off-by: Tim Chen <tim.c.chen@...ux.intel.com>
> ---
> 

When a patch is wrong, you can admit it and ask for a revert, instead of
obfuscating the code so much that even a netdev guy like me doesnt
understand it anymore.

We speak of a very recent patch in net-next, not yet published to Linus
tree. There is no shame to revert it right now and work on a new patch.

I want to be able to track future bugs on this code, and your patch and
their fixes made functions too hard to read.

If you dont want to work on it, I'll do it myself.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html