Q: a bogus set_fs(USER_DS) in setup_frame/setup_rt_frame ?

Q: a bogus set_fs(USER_DS) in setup_frame/setup_rt_frame ?

[Oleg Nesterov]

I am really puzzled by set_fs(USER_DS) in setup_frame/setup_rt_frame.

How is it possible that current->addr_limit != USER_DS ? If this _is_
possible, how can can we trust the result of access_ok() above?

Thanks in advance,

Oleg.

[patch] i386: remove unnecessary code

[Ingo Molnar]

* Oleg Nesterov <oleg@tv-sign.ru> wrote:

> I am really puzzled by set_fs(USER_DS) in setup_frame/setup_rt_frame.
> 
> How is it possible that current->addr_limit != USER_DS ? If this _is_ 
> possible, how can can we trust the result of access_ok() above?

hm, this is _ancient_ code (possibly dating back to the pharaohs). If we 
are in KERNEL_DS then we call do_signal() then we are most likely a 
kernel thread and regs->esp points to the kernel stack ... the result of 
which would be a quite spectacular crash anyway.

Patch below.

	Ingo

----------------------------->
Subject: [patch] i386: remove unnecessary code
From: Ingo Molnar <mingo@elte.hu>

Oleg Nesterov pointed out that the set_fs() calls in setup_frame()
and setup_rt_frame() were superfluous.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/i386/kernel/signal.c |    2 --
 1 file changed, 2 deletions(-)

Index: linux/arch/i386/kernel/signal.c
===================================================================
--- linux.orig/arch/i386/kernel/signal.c
+++ linux/arch/i386/kernel/signal.c
@@ -380,7 +380,6 @@ static int setup_frame(int sig, struct k
 	regs->edx = (unsigned long) 0;
 	regs->ecx = (unsigned long) 0;
 
-	set_fs(USER_DS);
 	regs->xds = __USER_DS;
 	regs->xes = __USER_DS;
 	regs->xss = __USER_DS;
@@ -474,7 +473,6 @@ static int setup_rt_frame(int sig, struc
 	regs->edx = (unsigned long) &frame->info;
 	regs->ecx = (unsigned long) &frame->uc;
 
-	set_fs(USER_DS);
 	regs->xds = __USER_DS;
 	regs->xes = __USER_DS;
 	regs->xss = __USER_DS;

Re: Q: a bogus set_fs(USER_DS) in setup_frame/setup_rt_frame ?

[Linus Torvalds]

On Tue, 17 Jul 2007, Oleg Nesterov wrote:
>
> I am really puzzled by set_fs(USER_DS) in setup_frame/setup_rt_frame.
> 
> How is it possible that current->addr_limit != USER_DS ? If this _is_
> possible, how can can we trust the result of access_ok() above?

Heh. I think it's entirely historical.

Please realize that the whole reason that function is called "set_fs()" is 
that it literally used to set the %fs segment register, not 
"->addr_limit".

So I think the "set_fs(USER_DS)" is there _only_ to match the other

        regs->xds = __USER_DS;
        regs->xes = __USER_DS;
        regs->xss = __USER_DS;
        regs->xcs = __USER_CS;

things, and never mattered. And now it matters even less, and has been 
copied to all other architectures where it is just totally insane.

		Linus

Re: Q: a bogus set_fs(USER_DS) in setup_frame/setup_rt_frame ?

[Oleg Nesterov]

On 07/17, Linus Torvalds wrote:
> 
> On Tue, 17 Jul 2007, Oleg Nesterov wrote:
> >
> > I am really puzzled by set_fs(USER_DS) in setup_frame/setup_rt_frame.
> > 
> > How is it possible that current->addr_limit != USER_DS ? If this _is_
> > possible, how can can we trust the result of access_ok() above?
> 
> Heh. I think it's entirely historical.
> 
> Please realize that the whole reason that function is called "set_fs()" is 
> that it literally used to set the %fs segment register, not 
> "->addr_limit".
> 
> So I think the "set_fs(USER_DS)" is there _only_ to match the other
> 
>         regs->xds = __USER_DS;
>         regs->xes = __USER_DS;
>         regs->xss = __USER_DS;
>         regs->xcs = __USER_CS;
> 
> things, and never mattered.

Thanks!

> And now it matters even less, and has been 
> copied to all other architectures where it is just totally insane.

Yes.

Also, sparc does something strange with do_sigaltstack(). It first copies
stack_t to the local variable, then sets KERNEL_DS to access it from
do_sigaltstack().

IOW, what's wrong with the patch below? Why should we ignore errors other
than -EFAULT?

Oleg.

--- arch/sparc64/kernel/signal.c~	2007-05-21 13:57:49.000000000 +0400
+++ arch/sparc64/kernel/signal.c	2007-07-17 21:04:32.000000000 +0400
@@ -291,7 +291,6 @@ void do_rt_sigreturn(struct pt_regs *reg
 	__siginfo_fpu_t __user *fpu_save;
 	mm_segment_t old_fs;
 	sigset_t set;
-	stack_t st;
 	int err;
 
 	/* Always make any pending restarted system calls return -EINTR */
@@ -327,20 +326,13 @@ void do_rt_sigreturn(struct pt_regs *reg
 		err |= restore_fpu_state(regs, &sf->fpu_state);
 
 	err |= __copy_from_user(&set, &sf->mask, sizeof(sigset_t));
-	err |= __copy_from_user(&st, &sf->stack, sizeof(stack_t));
-	
+	err |= do_sigaltstack(&st->stack, NULL, (unsigned long)sf);
+
 	if (err)
 		goto segv;
-		
+
 	regs->tpc = tpc;
 	regs->tnpc = tnpc;
-	
-	/* It is more difficult to avoid calling this function than to
-	   call it and ignore errors.  */
-	old_fs = get_fs();
-	set_fs(KERNEL_DS);
-	do_sigaltstack((const stack_t __user *) &st, NULL, (unsigned long)sf);
-	set_fs(old_fs);
 
 	sigdelsetmask(&set, ~_BLOCKABLE);
 	spin_lock_irq(&current->sighand->siglock);

Re: Q: a bogus set_fs(USER_DS) in setup_frame/setup_rt_frame ?

[David Miller]

From: Oleg Nesterov <oleg@tv-sign.ru>
Date: Tue, 17 Jul 2007 21:15:35 +0400

> Also, sparc does something strange with do_sigaltstack(). It first copies
> stack_t to the local variable, then sets KERNEL_DS to access it from
> do_sigaltstack().
> 
> IOW, what's wrong with the patch below? Why should we ignore errors other
> than -EFAULT?

Nothing wrong with it.

The code is this way because it was simply copied over from
the signal32.c implementation which has to translate around
the 32-bit types into the 64-bit ones the kernel wants.

But since the 64-bit side doesn't need the translations,
it doesn't need to do the funny do_sigaltstack() call sequence
either.

I'll apply your patch, thanks!