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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <6C79597B-CBC0-46FB-A700-BD04FFE2C5FF@googlemail.com>
Date:	Tue, 7 Jun 2011 08:49:53 +0200
From:	Mathias Krause <minipli@...glemail.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	linux-kernel@...r.kernel.org, stable@...nel.org,
	Al Viro <viro@...iv.linux.org.uk>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] init: use KERNEL_DS when trying to start init process

On 07.06.2011, 01:12 Andrew Morton wrote:
> On Mon, 30 May 2011 18:17:08 +0200
> Mathias Krause <minipli@...glemail.com> 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@...iv.linux.org.uk>
> AuthorDate: Wed Apr 26 14:04:08 2006 -0400
> Commit:     Al Viro <viro@...iv.linux.org.uk>
> 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().


Mathias--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/