Re: [PATCH] init: use KERNEL_DS when trying to start init process

From: Mathias Krause
Date: Tue Jun 07 2011 - 02:50:05 EST

On 07.06.2011, 01:12 Andrew Morton wrote:
> On Mon, 30 May 2011 18:17:08 +0200
> Mathias Krause <minipli@xxxxxxxxxxxxxx> wrote:
>> The bug is, that search_binary_handler() sets the address limit to
>> USER_DS but doesn't reset it on error which will make all further
>> attempts fail with -EFAULT because argv[0] is a pointer to kernel
>> memory, not userland.
>> The bug can easily be reproduced by starting a 32 bit kernel with a 64
>> bit executable as /init and a 32 bit version as /sbin/init within an
>> initramfs. The hardcoded defaults should make /init fail because of the
>> unsupported file format but should make /sbin/init succeed. This doesn't
>> happen because the string "/sbin/init" lives in kernel memory and is no
>> longer allowed because of the modified address limit to USER_DS after
> Geeze, you're kicking over some ancient rocks there.
> Possibly the bug was added by
> commit 473ae30bc7b1dda5c5791c773f95e9424ddfead9
> Author: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> AuthorDate: Wed Apr 26 14:04:08 2006 -0400
> Commit: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> CommitDate: Tue Jun 20 05:25:21 2006 -0400
> [PATCH] execve argument logging
> and will be fixed with
> --- a/fs/exec.c~a
> +++ a/fs/exec.c
> @@ -1357,14 +1357,14 @@ int search_binary_handler(struct linux_b
> if (retval)
> return retval;
> - /* kernel module loader fixup */
> - /* so we don't try to load run modprobe in kernel space. */
> - set_fs(USER_DS);
> -
> retval = audit_bprm(bprm);
> if (retval)
> return retval;
> + /* kernel module loader fixup */
> + /* so we don't try to load run modprobe in kernel space. */
> + set_fs(USER_DS);
> +
> retval = -ENOENT;
> for (try=0; try<2; try++) {
> read_lock(&binfmt_lock);
> _

This fixes nothing because search_binary_handler() won't return that early in
my case but will try the binfmt list to find an appropriate handler. That for,
removing the set_fs() *might* be yet another solution to the problem but I
wasn't brave enough to do that change because I could not foresee all related
consequences -- didn't want to exchange a bug fix for a security hole.

Just changing the address limit in run_init_process() was the straight forward
fix with the least possible implications to the rest of the execve path.

> but I'm finding lots of mysterious things in there.
> Like, what does this comment:
> /* so we don't try to load run modprobe in kernel space. */
> set_fs(USER_DS);
> mean?

I found this comment confusing, too. But since the usermode helper is started
as separate kernel thread I thought this might be a security measure to prevent
leaking kernel memory to userland?!

> It's all truly ancient code and I suspect the set_fs() simply isn't
> needed any more - the calling process doesn't parent modprobe. And
> request_module() should take care of the mm_segment, not its callers.
> Also, search_binary_handler() appears to *always* return with USER_DS?
> Is that a secret part of its interface? Or should it be
> unconditionally restoring KERNEL_DS?

Wouldn't that introduce a security bug, when a userland triggered execve()
fails to execute and returns to userland? Having that process run with
KERNEL_DS afterwards isn't wanted, is it? Or is the address limit restored
by some other means?

> I tried to work out how that set_fs() got there, in the historical git
> tree but it's part of 14592fa9:
> 73 files changed, 963 insertions(+), 798 deletions(-)
> which is pretty useless (what's up with that?)
> So I dunno, I'm stumped. I'm suspecting that the right fix here is to
> just remove that call to set_fs(USER_DS) but I'm having trouble working
> out what all this cruft is trying to do.

Me is having trouble too, that for the simple solution with run_init_process().

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at