Re: [PATCH v3 10/16] exec: Remove do_execve_file

From: ebiederm@xmission.com (Eric W. Biederman)
To: Luis Chamberlain <mcgrof@kernel.org>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	linux-kernel@vger.kernel.org, David Miller <davem@davemloft.net>,
	Greg Kroah-Hartman <greg@kroah.com>,
	Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
	Kees Cook <keescook@chromium.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Al Viro <viro@zeniv.linux.org.uk>, bpf <bpf@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Masahiro Yamada <yamada.masahiro@socionext.com>,
	Gary Lin <GLin@suse.com>, Bruno Meneguele <bmeneg@redhat.com>,
	LSM List <linux-security-module@vger.kernel.org>,
	Casey Schaufler <casey@schaufler-ca.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Christian Brauner <christian.brauner@ubuntu.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH v3 10/16] exec: Remove do_execve_file
Date: Wed, 08 Jul 2020 08:08:09 -0500	[thread overview]
Message-ID: <87y2nugnnq.fsf@x220.int.ebiederm.org> (raw)
In-Reply-To: <20200708124148.GP13911@42.do-not-panic.com> (Luis Chamberlain's message of "Wed, 8 Jul 2020 12:41:48 +0000")

Luis Chamberlain <mcgrof@kernel.org> writes:

> On Wed, Jul 08, 2020 at 06:35:25AM +0000, Luis Chamberlain wrote:
>> On Thu, Jul 02, 2020 at 11:41:34AM -0500, Eric W. Biederman wrote:
>> > Now that the last callser has been removed remove this code from exec.
>> > 
>> > For anyone thinking of resurrecing do_execve_file please note that
>> > the code was buggy in several fundamental ways.
>> > 
>> > - It did not ensure the file it was passed was read-only and that
>> >   deny_write_access had been called on it.  Which subtlely breaks
>> >   invaniants in exec.
>> > 
>> > - The caller of do_execve_file was expected to hold and put a
>> >   reference to the file, but an extra reference for use by exec was
>> >   not taken so that when exec put it's reference to the file an
>> >   underflow occured on the file reference count.
>> 
>> Maybe its my growing love with testing, but I'm going to have to partly
>> blame here that we added a new API without any respective testing.
>> Granted, I recall this this patch set could have used more wider review
>> and a bit more patience... but just mentioning this so we try to avoid
>> new api-without-testing with more reason in the future.
>> 
>> But more importantly, *how* could we have caught this? Or how can we
>> catch this sort of stuff better in the future?
>
> Of all the issues you pointed out with do_execve_file(), since upon
> review the assumption *by design* was that LSMs/etc would pick up issues
> with the file *prior* to processing, I think that this file reference
> count issue comes to my attention as the more serious issue which I
> wish we could address *first* before this crusade.
>
> So I have to ask, has anyone *really tried* to give a crack at fixing
> this refcount issue in a smaller way first? Alexei?
>
> I'm not opposed to the removal of do_execve_file(), however if there
> is a reproducible crash / issue with the existing user, this sledge
> hammer seems a bit overkill for older kernels.

It does not matter for older kernels because there is exactly one user.
That one user is just a place holder keeping the code alive until a real
user comes along.

For older kernels the solution is to just mark the bpfilter code broken
in Kconfig and refuse to compile it.  That is the trivial backportable
fix if anyone wants one.

Eric