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