On Mon, May 18, 2020 at 07:34:19PM -0500, Eric W. Biederman wrote:
>
> Recursion in kernel code is generally a bad idea as it can overflow
> the kernel stack. Recursion in exec also hides that the code is
> looping and that the loop changes bprm->file.
>
> Instead of recursing in search_binary_handler have the methods that
> would recurse set bprm->interpreter and return 0. Modify exec_binprm
> to loop when bprm->interpreter is set. Consolidate all of the
> reassignments of bprm->file in that loop to make it clear what is
> going on.
>
> The structure of the new loop in exec_binprm is that all errors return
> immediately, while successful completion (ret == 0 &&
> !bprm->interpreter) just breaks out of the loop and runs what
> exec_bprm has always run upon successful completion.
>
> Fail if the an interpreter is being call after execfd has been set.
> The code has never properly handled an interpreter being called with
> execfd being set and with reassignments of bprm->file and the
> assignment of bprm->executable in generic code it has finally become
> possible to test and fail when if this problematic condition happens.
>
> With the reassignments of bprm->file and the assignment of
> bprm->executable moved into the generic code add a test to see if
> bprm->executable is being reassigned.
>
> In search_binary_handler remove the test for !bprm->file. With all
> reassignments of bprm->file moved to exec_binprm bprm->file can never
> be NULL in search_binary_handler.
>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
Lovely!
Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
I spent some time following the file lifetimes of deny/allow_write_access()
and the fget/fput() paths. It all looks correct to me; it's tricky
(especially bprm->executable) but so very much cleaner than before. :)
The only suggestion I could come up with is more comments (surprise) to
help anyone new to this loop realize what the "common" path is (and
similarly, a compiler hint too):
diff --git a/fs/exec.c b/fs/exec.c
index a9f421ec9e27..738051a698e1 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1790,15 +1790,19 @@ static int exec_binprm(struct linux_binprm *bprm)
/* This allows 4 levels of binfmt rewrites before failing hard. */
for (depth = 0;; depth++) {
struct file *exec;
+
if (depth > 5)
return -ELOOP;
ret = search_binary_handler(bprm);
+ /* Unrecoverable error, give up. */
if (ret < 0)
return ret;
- if (!bprm->interpreter)
+ /* Found final handler, start execution. */
+ if (likely(!bprm->interpreter))
break;
+ /* Found an interpreter, so try again and attempt to run it. */
exec = bprm->file;
bprm->file = bprm->interpreter;
bprm->interpreter = NULL;
--
Kees Cook
