Re: + fs-binfmts-better-handling-of-binfmt-loops.patch added to -mm tree

Re: + fs-binfmts-better-handling-of-binfmt-loops.patch added to -mm tree

[Oleg Nesterov]

> From: Zach Levis <zml@linux.vnet.ibm.com>
> Subject: fs/binfmts: better handling of binfmt loops
>
> With these changes, when a binfmt loop is encountered, the ELOOP will
> propagate back to the 0 depth.  At this point the argv and argc values
> will be reset to what they were originally and an attempt is made to
> continue with the following binfmt handlers.

I must admit, I do not really understand why do we want to recover
after pr_err(). Perhaps the changelog could say a bit more.

> --- a/fs/exec.c~fs-binfmts-better-handling-of-binfmt-loops
> +++ a/fs/exec.c
> @@ -1403,13 +1403,40 @@ int search_binary_handler(struct linux_b
>  			if (!try_module_get(fmt->module))
>  				continue;
>  			read_unlock(&binfmt_lock);
> +			bprm->previous_binfmts[1] = bprm->previous_binfmts[0];
> +			bprm->previous_binfmts[0] = fmt;
> +
>  			bprm->recursion_depth = depth + 1;
>  			retval = fn(bprm);
>  			bprm->recursion_depth = depth;
> +			if (retval == -ELOOP && depth == 0) { /* cur, previous */
> +				pr_err("Too much recursion with binfmts (0:%s, -1:%s) in file %s, skipping (base %s).\n",
> +						bprm->previous_binfmts[0]->name,
> +						bprm->previous_binfmts[1]->name,
> +						bprm->filename,
> +						fmt->name);
> +
> +				/* Put argv back in its place */
> +				while (bprm->argc > 0) {
> +					retval = remove_arg_zero(bprm);
> +					if (retval)
> +						return retval;
> +				}

But why do we need this?

Afaics we only need to restore bprm->p to the old value before the
1st do_execve_common()->copy_strings(argv) and nothing else, no ?
free_bprm()->free_arg_pages() will do the necessary cleanup in any
case.

> +
> +				copy_strings(bprm->argc_orig, *((struct user_arg_ptr *) bprm->argv_orig), bprm);

Perhaps it would be more clean to add "struct user_arg_ptr;"
into binfmts.h and avoid the typecast.

And I do not think we should ignore the possible error from
copy_strings(). Even if we know that it succeeded before, another
thread can, say, unmap this memory in between.

> +				bprm->argc = bprm->argc_orig;

Or we can simply do count() again. compared to copy_strings() this
is cheap.

Oleg.

Re: + fs-binfmts-better-handling-of-binfmt-loops.patch added to -mm tree

[Oleg Nesterov]

On 07/31, Oleg Nesterov wrote:
>
> > From: Zach Levis <zml@linux.vnet.ibm.com>
> > Subject: fs/binfmts: better handling of binfmt loops
> >
> > With these changes, when a binfmt loop is encountered, the ELOOP will
> > propagate back to the 0 depth.  At this point the argv and argc values
> > will be reset to what they were originally and an attempt is made to
> > continue with the following binfmt handlers.
>
> I must admit, I do not really understand why do we want to recover
> after pr_err(). Perhaps the changelog could say a bit more.

And still can't. Probably I missed something, but it seems that
this tries to "fix" the wrong /proc/sys/fs/binfmt_misc/register...

> > --- a/fs/exec.c~fs-binfmts-better-handling-of-binfmt-loops
> > +++ a/fs/exec.c
> > @@ -1403,13 +1403,40 @@ int search_binary_handler(struct linux_b
> >  			if (!try_module_get(fmt->module))
> >  				continue;
> >  			read_unlock(&binfmt_lock);
> > +			bprm->previous_binfmts[1] = bprm->previous_binfmts[0];
> > +			bprm->previous_binfmts[0] = fmt;
> > +
> >  			bprm->recursion_depth = depth + 1;
> >  			retval = fn(bprm);
> >  			bprm->recursion_depth = depth;
> > +			if (retval == -ELOOP && depth == 0) { /* cur, previous */
> > +				pr_err("Too much recursion with binfmts (0:%s, -1:%s) in file %s, skipping (base %s).\n",
> > +						bprm->previous_binfmts[0]->name,
> > +						bprm->previous_binfmts[1]->name,
> > +						bprm->filename,
> > +						fmt->name);
> > +
> > +				/* Put argv back in its place */
> > +				while (bprm->argc > 0) {
> > +					retval = remove_arg_zero(bprm);
> > +					if (retval)
> > +						return retval;
> > +				}
>
> But why do we need this?
>
> Afaics we only need to restore bprm->p to the old value before the
> 1st do_execve_common()->copy_strings(argv) and nothing else, no ?
> free_bprm()->free_arg_pages() will do the necessary cleanup in any
> case.
>
> > +
> > +				copy_strings(bprm->argc_orig, *((struct user_arg_ptr *) bprm->argv_orig), bprm);
>
> Perhaps it would be more clean to add "struct user_arg_ptr;"
> into binfmts.h and avoid the typecast.
>
> And I do not think we should ignore the possible error from
> copy_strings(). Even if we know that it succeeded before, another
> thread can, say, unmap this memory in between.

And since we do copy_strings() again we probably need acct_arg_size()
after remove_arg_zero() loop, although this is not that important.

And with this patch "depth == 0" check(s) look even worse, imho we
need to cleanup this code first. And proc_exec_connector() looks
simply wrong. I'll try to make a patch.

But once again, I can be easily wrong, so please correct me.

Oleg.

Re: + fs-binfmts-better-handling-of-binfmt-loops.patch added to -mm tree

[Zach Levis]

Quoting Oleg Nesterov <oleg@redhat.com>:

> On 07/31, Oleg Nesterov wrote:
>>
>> > From: Zach Levis <zml@linux.vnet.ibm.com>
>> > Subject: fs/binfmts: better handling of binfmt loops
>> >
>> > With these changes, when a binfmt loop is encountered, the ELOOP will
>> > propagate back to the 0 depth.  At this point the argv and argc values
>> > will be reset to what they were originally and an attempt is made to
>> > continue with the following binfmt handlers.
>>
>> I must admit, I do not really understand why do we want to recover
>> after pr_err(). Perhaps the changelog could say a bit more.
>
> And still can't. Probably I missed something, but it seems that
> this tries to "fix" the wrong /proc/sys/fs/binfmt_misc/register...
>
So an example of what this would be used for (going into commit  
message of a v2 with your earlier suggestions):
A qemu is configured to run 64-bit ELFs on an otherwise 32-bit system.  
The system's owner switches to running with 64-bit executables, but  
forgets to disable the binfmt_misc option that redirects 64bit ELFs to  
qemu. Since the qemu executable is a 64-bit ELF now, binfmt_misc keeps  
on matching it with the qemu rule, preventing the execution of any  
64-bit binary.

With this patch, an error is printed and search_binary_handler()  
continues on to the next handler, allowing the original executable to  
run normally so the user can (hopefully) fix their misconfiguration  
more easily.

>> > +
>> > +				copy_strings(bprm->argc_orig, *((struct user_arg_ptr *)  
>> bprm->argv_orig), bprm);
>>
>> Perhaps it would be more clean to add "struct user_arg_ptr;"
>> into binfmts.h and avoid the typecast.

I was kinda trying to avoid exposing the struct, but yeah, that's better.
>>
>> And I do not think we should ignore the possible error from
>> copy_strings(). Even if we know that it succeeded before, another
>> thread can, say, unmap this memory in between.
>
> And since we do copy_strings() again we probably need acct_arg_size()
> after remove_arg_zero() loop, although this is not that important.
I'm not sure if that's even necessary. It looks like there's  
copy_strings()->get_arg_page()->acct_arg_size() that's already called.
>
> And with this patch "depth == 0" check(s) look even worse, imho we
> need to cleanup this code first. And proc_exec_connector() looks
> simply wrong. I'll try to make a patch.
>
> But once again, I can be easily wrong, so please correct me.
>
> Oleg.

Re: + fs-binfmts-better-handling-of-binfmt-loops.patch added to -mm tree

[Oleg Nesterov]

On 08/01, Zach Levis wrote:
>
> So an example of what this would be used for (going into commit message 
> of a v2 with your earlier suggestions):

Ah, so you are going to send v2, great.

May I ask you to wait a little bit? Once again, I believe that
search_binary_handler() needs a cleanup + minor fix. I'll try
to send the patch today.

> With this patch, an error is printed

I agree, it makes sense to print an error with names.

> and search_binary_handler()
> continues on to the next handler, allowing the original executable to
> run normally so the user can (hopefully) fix their misconfiguration more
> easily.

Still not sure this makes sense, but I can't judge and I won't argue.

>>> And I do not think we should ignore the possible error from
>>> copy_strings(). Even if we know that it succeeded before, another
>>> thread can, say, unmap this memory in between.
>>
>> And since we do copy_strings() again we probably need acct_arg_size()
>> after remove_arg_zero() loop, although this is not that important.
> I'm not sure if that's even necessary.

Yes, I was wrong, thanks for correcting me. We don't need this.

> It looks like there's
> copy_strings()->get_arg_page()->acct_arg_size() that's already called.

This doesn't matter, this won't unaccount the memory. But I was
wrong anyway, we do not need to unaccount because vma won't grow.

Oleg.

Re: + fs-binfmts-better-handling-of-binfmt-loops.patch added to -mm tree

[Zach Levis]

Quoting Oleg Nesterov <oleg@redhat.com>:

> On 08/01, Zach Levis wrote:
>>
>> So an example of what this would be used for (going into commit message
>> of a v2 with your earlier suggestions):
>
> Ah, so you are going to send v2, great.
>
> May I ask you to wait a little bit? Once again, I believe that
> search_binary_handler() needs a cleanup + minor fix. I'll try
> to send the patch today.

ok, I'll base v2 off of your patch

+ fs-binfmts-better-handling-of-binfmt-loops.patch added to -mm tree

[akpm]

Subject: + fs-binfmts-better-handling-of-binfmt-loops.patch added to -mm tree
To: zml@linux.vnet.ibm.com,viro@zeniv.linux.org.uk,zach@zachsthings.com
From: akpm@linux-foundation.org
Date: Tue, 30 Jul 2013 14:06:38 -0700


The patch titled
     Subject: fs/binfmts: better handling of binfmt loops
has been added to the -mm tree.  Its filename is
     fs-binfmts-better-handling-of-binfmt-loops.patch

This patch should soon appear at
    http://ozlabs.org/~akpm/mmots/broken-out/fs-binfmts-better-handling-of-binfmt-loops.patch
and later at
    http://ozlabs.org/~akpm/mmotm/broken-out/fs-binfmts-better-handling-of-binfmt-loops.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

The -mm tree is included into linux-next and is updated
there every 3-4 working days

------------------------------------------------------
From: Zach Levis <zml@linux.vnet.ibm.com>
Subject: fs/binfmts: better handling of binfmt loops

With these changes, when a binfmt loop is encountered, the ELOOP will
propagate back to the 0 depth.  At this point the argv and argc values
will be reset to what they were originally and an attempt is made to
continue with the following binfmt handlers.

Caveats:
- binfmt_misc is skipped as a whole. To allow for a more generic
  solution that works for any binfmt without a lot of duplicated code
  and added complexity, the error detection code is applied on the
  binfmt level.
- This is a fallback solution. It attempts to restore the state to allow
  executing most binaries without side effects, but it may not work for
  everything and should not be depended on for regular usage.

My (rough, but functional) test scripts for this issue are available at:
    https://gist.github.com/zml2008/6075418

Signed-off-by: Zach Levis <zach@zachsthings.com>
Signed-off-by: Zach Levis <zml@linux.vnet.ibm.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/exec.c               |   30 +++++++++++++++++++++++++++++-
 include/linux/binfmts.h |    5 ++++-
 2 files changed, 33 insertions(+), 2 deletions(-)

diff -puN fs/exec.c~fs-binfmts-better-handling-of-binfmt-loops fs/exec.c
--- a/fs/exec.c~fs-binfmts-better-handling-of-binfmt-loops
+++ a/fs/exec.c
@@ -1403,13 +1403,40 @@ int search_binary_handler(struct linux_b
 			if (!try_module_get(fmt->module))
 				continue;
 			read_unlock(&binfmt_lock);
+			bprm->previous_binfmts[1] = bprm->previous_binfmts[0];
+			bprm->previous_binfmts[0] = fmt;
+
 			bprm->recursion_depth = depth + 1;
 			retval = fn(bprm);
 			bprm->recursion_depth = depth;
+			if (retval == -ELOOP && depth == 0) { /* cur, previous */
+				pr_err("Too much recursion with binfmts (0:%s, -1:%s) in file %s, skipping (base %s).\n",
+						bprm->previous_binfmts[0]->name,
+						bprm->previous_binfmts[1]->name,
+						bprm->filename,
+						fmt->name);
+
+				/* Put argv back in its place */
+				while (bprm->argc > 0) {
+					retval = remove_arg_zero(bprm);
+					if (retval)
+						return retval;
+				}
+
+				copy_strings(bprm->argc_orig, *((struct user_arg_ptr *) bprm->argv_orig), bprm);
+				bprm->argc = bprm->argc_orig;
+				retval = -ENOEXEC;
+				continue;
+			}
+
 			if (retval >= 0) {
 				if (depth == 0) {
 					trace_sched_process_exec(current, old_pid, bprm);
 					ptrace_event(PTRACE_EVENT_EXEC, old_vpid);
+					/* Successful execution, now null out the cached argv
+					 * (we don't want to access it later)
+					 * */
+					bprm->argv_orig = NULL;
 				}
 				put_binfmt(fmt);
 				allow_write_access(bprm->file);
@@ -1516,7 +1543,7 @@ static int do_execve_common(const char *
 	if (retval)
 		goto out_file;
 
-	bprm->argc = count(argv, MAX_ARG_STRINGS);
+	bprm->argc_orig = bprm->argc = count(argv, MAX_ARG_STRINGS);
 	if ((retval = bprm->argc) < 0)
 		goto out;
 
@@ -1540,6 +1567,7 @@ static int do_execve_common(const char *
 	retval = copy_strings(bprm->argc, argv, bprm);
 	if (retval < 0)
 		goto out;
+	bprm->argv_orig = &argv;
 
 	retval = search_binary_handler(bprm);
 	if (retval < 0)
diff -puN include/linux/binfmts.h~fs-binfmts-better-handling-of-binfmt-loops include/linux/binfmts.h
--- a/include/linux/binfmts.h~fs-binfmts-better-handling-of-binfmt-loops
+++ a/include/linux/binfmts.h
@@ -32,11 +32,14 @@ struct linux_binprm {
 	unsigned int taso:1;
 #endif
 	unsigned int recursion_depth;
+	/* current binfmt at 0 and previous binfmt at 1 */
+	struct linux_binfmt *previous_binfmts[2];
 	struct file * file;
 	struct cred *cred;	/* new credentials */
 	int unsafe;		/* how unsafe this exec is (mask of LSM_UNSAFE_*) */
 	unsigned int per_clear;	/* bits to clear in current->personality */
-	int argc, envc;
+	int argc, argc_orig, envc;
+	void *argv_orig;
 	const char * filename;	/* Name of binary as seen by procps */
 	const char * interp;	/* Name of the binary really executed. Most
 				   of the time same as filename, but could be
_

Patches currently in -mm which might be from zml@linux.vnet.ibm.com are

fs-binfmts-add-a-name-field-to-the-binfmt-struct.patch
fs-binfmts-better-handling-of-binfmt-loops.patch
fs-binfmts-whitespace-fixes-with-scripts-cleanfile.patch