Re: [PATCH v4 2/3] fs/binfmts: Better handling of binfmt loops

[Zach L]

On 08/14/2013 10:50 AM, Oleg Nesterov wrote:
> On 08/14, Zach Levis wrote:
>>
>> Example: 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.
>
> Honestly, I dislike this version even more, sorry. The patch becomes
> much more complex, and and it is still not clear to me why do we want
> these complications.
>
It's a larger patch but the majority of the increase is from is
splitting the binfmt initialization code into a separate function to
address the issue you brought up where the state of the binprm was not
entirely restored
>> My (rough, but functional) test scripts for this issue are available at:
>>      https://gist.github.com/zml2008/6075418
>
> Well, suppose that someone tries to read this changelog in 2014 to
> understand the code. Are you sure this link will be still alive?
fairly likely github will be around for a while, but will put the
contents in the commit msg to be safer
>
> It would be better to have everything in the changelog, if possible.
>
>> +static void put_binfmts(struct linux_binprm *bprm, struct linux_binfmt *cur_fmt)
>> +{
>> +    if (bprm->previous_binfmts[1])
>> +        put_binfmt(bprm->previous_binfmts[1]);
>> +    if (bprm->previous_binfmts[0])
>> +        put_binfmt(bprm->previous_binfmts[0]);
>> +    if (cur_fmt)
>> +        put_binfmt(cur_fmt);
>> +}
>
> I didn't actually read this patch, but at first glance this doesn't look
> right. Just suppose that ->load_binary() succeeds at depth = N, this will
> be called N times.
ok, I've moved things around so this is only called once
>
[snip]
>
> And btw, if we want this, then why we only do this if recursion_depth == 0?
> Just condider '#!/path-to-the-binary-which-wants-this-patch".
Unless recursion_depth is 0, there could be a binfmt in between that
would expect its changes to the binprm to remain in effect in lower
handlers, so even with your example
>
> And again, the patch (afaics) translates -ELOOP into -ENOEXEC on failure,
> not good.
it doesn't do that, but init_binprm reset the return value to success,
which is worse. going to be fixed in the next revision
>
> Oleg.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/