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:

quoted

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.

  Luis