linux-kernel - Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe

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: <20120309144239.GD13346@moon>
Date:	Fri, 9 Mar 2012 18:42:39 +0400
From:	Cyrill Gorcunov <gorcunov@...nvz.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Matt Helsley <matthltc@...ibm.com>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Pavel Emelyanov <xemul@...allels.com>,
	Kees Cook <keescook@...omium.org>, Tejun Heo <tj@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file v3

On Fri, Mar 09, 2012 at 03:26:20PM +0100, Oleg Nesterov wrote:
> >
> > To be sure it's not increased somewhere else before
> > down_write taken.
> 
> Who can do this? Only another CLONE_VM thread. And _only_ if we
> already have the bug in mm_exe accounting logic. And only if that
> thread does something to trigger the bug in the small window
> between.

ok, agreed.

> > > into removed_exe_file_vma().
> >
> > This one looks like a good idea for me -- it's cheap and
> > not a hot path.
> 
> But not in this patch, please.
> 

Sure.

> > > But imho your WARN looks like "OK, I checked it lockless but I
> > > am not sure this is correct".
> >
> > Oleg, I bet if someone will be changing num_exe_file_vmas overall
> > idea -- this prctl code will be fixed at last moment (if ever) only
> > because it's very specific, so I wanted to not miss such moment
> > and add some check that the rest of the kernel is in a good state.
> > This test is cheap but may prevent potential problem if one day
> > mm::exe_file concept will be reworked.
> 
> The test is cheap indeed. If you mean performance-wise.
> 
> But it looks confusing, imho. I do not care about a couple of CPU
> cycles. The code should be optimized for the reading in the first
> place, not for executing ;) Imho, of course.
> 
> And once again. Following your logic you need another WARN_ON()
> right after we drop mmap_sem. Why? To be sure it's not increased
> somewhere else _after_ down_write taken. And another one after
> fput.
> 
> Sure, bugs are possible. And yes, in theory this WARN_ON() can
> catch some problem. But there is tradeoff. Given that you need
> another thread to trigger the (potential) bug and the window is
> tiny, how high do you estimate the probability it can help?
> 
> > Sure I can simply drop this WARN_ON ;)
> 
> Oh, keep it if you like it ;)
> 
> Yes I hate it, but you are the author and this is almost cosmetic.

OK, Oleg, can't argue, you've convinced me ;) I'll drop this WARN_ON.
Would it be enough for your Reviewed-by tag? /me hides

	Cyrill
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/