linux-kernel - Re: [PATCH 0/2] finx argv

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]

Date:	Sun, 17 Mar 2013 15:15:55 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Andi Kleen <andi@...stfloor.org>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Lucas De Marchi <lucas.de.marchi@...il.com>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Paul Mackerras <paulus@...ba.org>, david@...son.dropbear.id.au,
	Kees Cook <keescook@...omium.org>,
	Serge Hallyn <serge.hallyn@...onical.com>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	Feng Hong <hongfeng@...vell.com>,
	Lucas De Marchi <lucas.demarchi@...fusion.mobi>
Subject: Re: [PATCH 0/2] finx argv_split() vs sysctl race

On 03/16, Andi Kleen wrote:
>
> On Sat, Mar 16, 2013 at 10:23:51PM +0100, Oleg Nesterov wrote:
> > >
> > > rcu strings has a helper function to copy the string for sleepy cases.
> >
> > Then you need to pre-allocate, take rcu_read_lock(), copy, and check
> > that it actually fits the pre-allocated buffer. Not sure why the simple
> > rwsem is worse.
>
> The reason I did it originally like that was that some of the sysctls weren't
> as "slow path" as power off.

OK, in this case rwsem is not fine, I agree.

> > > > To me 1/2 looks as a simplification anyway, but I won't argue if we
> > > > decide to add rcu/locking and avoid this patch.
> > >
> > > Ok I'll revisit.
> >
> > OK, but do you agree with 1/2?
>
> It doesn't solve the race alone because when the 0 byte can move it's
> not safe to run kstrndup() in parallel.

I don't think this is possible... To simplify, lets consider
poweroff_cmd[POWEROFF_CMD_PATH_LEN]. proc_dostring() or whatever can
never overwrite the last byte == 0, otherwise it will exceed this array
later to write the final zero.

But this doesn't matter,

> Ok given the n and that it
> force terminates it could only lead to some junk at the end.

Yes, kstrndup(max) is always safe even if the memory is not null
terminated.

> But it seems like a useful small optimization, although I don't know
> if it's used in any non slow paths.

Ah, I didn't mean the patch makes sense because of optimization. My
point was, we can fix the race without making this code worse (in fact
it tries to make the code better but this is subjective).

"fix the race" only means "make it safe" of course, a string in between
is unlikely useful.

> I assume you audited all callers that they comprehend that they need
> to free differently now.

Yes, /bin/grep shows that all callers use argv_free().

Oleg.

--
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/