lkml.org 
[lkml]   [2013]   [Aug]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v4 2/3] fs/binfmts: Better handling of binfmt loops
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.
>


\
 
 \ /
  Last update: 2013-08-15 19:01    [W:0.082 / U:0.776 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site