Re: + fully-honor-vdso_enabled.patch added to -mm tree

Re: + fully-honor-vdso_enabled.patch added to -mm tree

[Oleg Nesterov]

John Reiser wrote:
>
> --- a/arch/i386/kernel/sysenter.c~fully-honor-vdso_enabled
> +++ a/arch/i386/kernel/sysenter.c
> @@ -22,6 +22,8 @@
>  #include <asm/msr.h>
>  #include <asm/pgtable.h>
>  #include <asm/unistd.h>
> +#include <asm/a.out.h>
> +#include <asm/mman.h>
>
>  /*
>   * Should the kernel map a VDSO page into processes and pass its
> @@ -105,10 +107,25 @@ int arch_setup_additional_pages(struct l
>  {
>  	struct mm_struct *mm = current->mm;
>  	unsigned long addr;
> +	unsigned long flags;
>  	int ret;
>
> +	switch (vdso_enabled) {
> +	case 0:  /* none */
> +		return 0;

This means we don't initialize mm->context.vdso and ->sysenter_return.

Is it ok? For example, setup_rt_frame() uses VDSO_SYM(&__kernel_rt_sigreturn),
sysenter_past_esp pushes ->sysenter_return on stack.

Note also that load_elf_binary does

	arch_setup_additional_pages()
	create_elf_tables()

, looks like application can crash after exec if vdso_enabled changes from 0
to 1 in between.

Could you please explain if I missed something?

Oleg.

Re: + fully-honor-vdso_enabled.patch added to -mm tree

[Paul Mundt]

On Thu, Mar 01, 2007 at 08:52:07PM +0300, Oleg Nesterov wrote:
> > --- a/arch/i386/kernel/sysenter.c~fully-honor-vdso_enabled
> > +++ a/arch/i386/kernel/sysenter.c
> > @@ -22,6 +22,8 @@
> >  #include <asm/msr.h>
> >  #include <asm/pgtable.h>
> >  #include <asm/unistd.h>
> > +#include <asm/a.out.h>
> > +#include <asm/mman.h>
> >
> >  /*
> >   * Should the kernel map a VDSO page into processes and pass its
> > @@ -105,10 +107,25 @@ int arch_setup_additional_pages(struct l
> >  {
> >  	struct mm_struct *mm = current->mm;
> >  	unsigned long addr;
> > +	unsigned long flags;
> >  	int ret;
> >
> > +	switch (vdso_enabled) {
> > +	case 0:  /* none */
> > +		return 0;
> 
> This means we don't initialize mm->context.vdso and ->sysenter_return.
> 
> Is it ok? For example, setup_rt_frame() uses VDSO_SYM(&__kernel_rt_sigreturn),
> sysenter_past_esp pushes ->sysenter_return on stack.
> 
The setup_rt_frame() case is fairly straightforward, both PPC and SH
already check to make sure there's a valid context before trying to use
VDSO_SYM(), I'm not sure why x86 doesn't.

Though I wonder if there's any point in checking binfmt->hasvdso here?
There shouldn't be a valid mm->context.vdso in the !hasvdso case..

Someone else will have to comment on ->sysenter_return.

Signed-off-by: Paul Mundt <lethal@linux-sh.org>

--

 arch/i386/kernel/signal.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/i386/kernel/signal.c b/arch/i386/kernel/signal.c
index 4f99e87..f778d34 100644
--- a/arch/i386/kernel/signal.c
+++ b/arch/i386/kernel/signal.c
@@ -350,7 +350,7 @@ static int setup_frame(int sig, struct k_sigaction *ka,
 			goto give_sigsegv;
 	}
 
-	if (current->binfmt->hasvdso)
+	if (current->binfmt->hasvdso && current->mm->context.vdso)
 		restorer = (void *)VDSO_SYM(&__kernel_sigreturn);
 	else
 		restorer = (void *)&frame->retcode;

Re: + fully-honor-vdso_enabled.patch added to -mm tree

[Oleg Nesterov]

On 03/02, Paul Mundt wrote:
>
> On Thu, Mar 01, 2007 at 08:52:07PM +0300, Oleg Nesterov wrote:
> > >
> > > @@ -105,10 +107,25 @@ int arch_setup_additional_pages(struct l
> > >  {
> > >  	struct mm_struct *mm = current->mm;
> > >  	unsigned long addr;
> > > +	unsigned long flags;
> > >  	int ret;
> > >
> > > +	switch (vdso_enabled) {
> > > +	case 0:  /* none */
> > > +		return 0;
> > 
> > This means we don't initialize mm->context.vdso and ->sysenter_return.
> > 
> > Is it ok? For example, setup_rt_frame() uses VDSO_SYM(&__kernel_rt_sigreturn),
> > sysenter_past_esp pushes ->sysenter_return on stack.
>
> The setup_rt_frame() case is fairly straightforward, both PPC and SH
> already check to make sure there's a valid context before trying to use
> VDSO_SYM(), I'm not sure why x86 doesn't.
>
> Though I wonder if there's any point in checking binfmt->hasvdso here?
> There shouldn't be a valid mm->context.vdso in the !hasvdso case..

setup_rt_frame() is obviously wrong? Surely it must check ->hasvdso like
setup_frame() does! Otherwise, we will have SIGSEGV on SA_SIGINFO if
->load_binary() does not call arch_setup_additional_pages(), no?

If no, what ->hasvdso is?

> Someone else will have to comment on ->sysenter_return.

It is needed for sysexit. If we don't use sysenter (and we shouldn't, because
syscall_page is not mapped), we don't need to initialize it. Note also that
sys_execve() sets TIF_IRET, so we are safe even if sys_execve() was called
using __kernel_vsyscall.

Still, I don't understand why we don't pass NEW_AUX_ENT(AT_SYSINFO) when
vdso_enabled == 0. We don't need linux-gate.so to use __kernel_vsyscall,
we have FIX_VDSO. In that case we should s/PAGE_KERNEL_RO/PAGE_READONLY/
of course. I guess the reason is some magic in glibc.

Oleg.

Re: + fully-honor-vdso_enabled.patch added to -mm tree

[John Reiser]

Oleg Nesterov wrote:
> John Reiser wrote:

>>+	switch (vdso_enabled) {
>>+	case 0:  /* none */
>>+		return 0;
> 
> 
> This means we don't initialize mm->context.vdso and ->sysenter_return.
> 
> Is it ok? For example, setup_rt_frame() uses VDSO_SYM(&__kernel_rt_sigreturn),
> sysenter_past_esp pushes ->sysenter_return on stack.

Paul Mundt has commented on setup_rt_frame() and provided a patch which
bullet-proofs that area.  I will include that patch into the next revision.

The value of ->sysenter_return is interpreted in user space by the
sysexit instruction; nobody else cares what the value is.  The kernel
is not required to provide a good value when vdso_enabled is zero,
because the kernel has not told the process that sysenter is valid
(by setting AT_SYSINFO.)  The kernel requires specific register values
for sysenter+sysexit and these values may change at the whim of the
kernel, so correct code must follow the kernel's protocol.
glibc uses sysenter only when AT_SYSINFO is present.  User code can
screw up even when vdso_enabled is non-zero, by overwriting or re-
mapping the vdso page (clobber memory at the destination of sysexit.)

Both context.vdso and sysenter_return could be set to zero whenever
vdso_enabled is zero; those two values might even be defaulted.
I'll add such a change to the next revision of the patch, if you'll
defend it against claims of "unnecessary code."

> 
> Note also that load_elf_binary does
> 
> 	arch_setup_additional_pages()
> 	create_elf_tables()
> 
> , looks like application can crash after exec if vdso_enabled changes from 0
> to 1 in between.

Correct.  Changing vdso_enabled from 0 to non-zero must be prepared
to lose this race if it is not prevented.  Ordinarily it won't matter
because the administrator will perform such changes at a "quiet" time.

-- 
John Reiser, jreiser@BitWagon.com

Re: + fully-honor-vdso_enabled.patch added to -mm tree

[John Reiser]

Oleg Nesterov wrote:

> Still, I don't understand why we don't pass NEW_AUX_ENT(AT_SYSINFO) when
> vdso_enabled == 0. We don't need linux-gate.so to use __kernel_vsyscall,
> we have FIX_VDSO. In that case we should s/PAGE_KERNEL_RO/PAGE_READONLY/
> of course. I guess the reason is some magic in glibc.

In the past there were problems where user code could not single-step
(or even read the code of) such system calls.  Can these two operations
be performed today with what you propose, particularly if FIX_VDSO is
near 0xfffff000 ?

-- 
John Reiser, jreiser@BitWagon.com

Re: + fully-honor-vdso_enabled.patch added to -mm tree

[Oleg Nesterov]

On 03/02, John Reiser wrote:
>
> Oleg Nesterov wrote:
> > John Reiser wrote:
> 
> >>+	switch (vdso_enabled) {
> >>+	case 0:  /* none */
> >>+		return 0;
> > 
> > 
> > This means we don't initialize mm->context.vdso and ->sysenter_return.
> > 
> > Is it ok? For example, setup_rt_frame() uses VDSO_SYM(&__kernel_rt_sigreturn),
> > sysenter_past_esp pushes ->sysenter_return on stack.
> 
> Paul Mundt has commented on setup_rt_frame() and provided a patch which
> bullet-proofs that area.  I will include that patch into the next revision.

Confused. I still think his patch incomplete. Don't we need the same check
in setup_rt_frame() ?

> The value of ->sysenter_return is interpreted in user space by the
> sysexit instruction; nobody else cares what the value is.  The kernel
> is not required to provide a good value when vdso_enabled is zero,

Yes sure.

> Both context.vdso and sysenter_return could be set to zero whenever
> vdso_enabled is zero; those two values might even be defaulted.
> I'll add such a change to the next revision of the patch, if you'll
> defend it against claims of "unnecessary code."

context.vdso == NULL after mm_alloc(). I don't see a "good" arch dependent
function to clear ->sysenter_return (if we really need this). May be
flush_thread().

> > Note also that load_elf_binary does
> > 
> > 	arch_setup_additional_pages()
> > 	create_elf_tables()
> > 
> > , looks like application can crash after exec if vdso_enabled changes from 0
> > to 1 in between.
> 
> Correct.  Changing vdso_enabled from 0 to non-zero must be prepared
> to lose this race if it is not prevented.  Ordinarily it won't matter
> because the administrator will perform such changes at a "quiet" time.

I agree, this problem is mostly theoretical, but I believe it is hardly
possible to document what the "quiet" time is ;)

How about

	 #define ARCH_DLINFO
	-do if (vdso_enabled) {
	+do if (VDSO_BASE) {
?

Oleg.

Re: + fully-honor-vdso_enabled.patch added to -mm tree

[Chuck Ebbert]

John Reiser wrote:
> The value of ->sysenter_return is interpreted in user space by the
> sysexit instruction; nobody else cares what the value is.  The kernel
> is not required to provide a good value when vdso_enabled is zero,
> because the kernel has not told the process that sysenter is valid
> (by setting AT_SYSINFO.)

Doesn't matter because a malicious user can still execute sysenter.
We do have to deal with that somehow, so we have to put something
safe in there.

> Correct.  Changing vdso_enabled from 0 to non-zero must be prepared
> to lose this race if it is not prevented.  Ordinarily it won't matter
> because the administrator will perform such changes at a "quiet" time.
> 

We have to deal with all the possibilities here, too.

Re: + fully-honor-vdso_enabled.patch added to -mm tree

[Oleg Nesterov]

On 03/02, Chuck Ebbert wrote:
>
> John Reiser wrote:
> > The value of ->sysenter_return is interpreted in user space by the
> > sysexit instruction; nobody else cares what the value is.  The kernel
> > is not required to provide a good value when vdso_enabled is zero,
> > because the kernel has not told the process that sysenter is valid
> > (by setting AT_SYSINFO.)
> 
> Doesn't matter because a malicious user can still execute sysenter.
> We do have to deal with that somehow, so we have to put something
> safe in there.

Yes, but a malicious user can't make any harm to the system, sysexit
jumps to ->sysenter_return when user_mode() is true, right?

(I am asking because I don't know in details what sysexit does).

Oleg.

Re: + fully-honor-vdso_enabled.patch added to -mm tree

[John Reiser]

Chuck Ebbert wrote:
> John Reiser wrote:
> 
>>The value of ->sysenter_return is interpreted in user space by the
>>sysexit instruction; nobody else cares what the value is.  The kernel
>>is not required to provide a good value when vdso_enabled is zero,
>>because the kernel has not told the process that sysenter is valid
>>(by setting AT_SYSINFO.)
> 
> 
> Doesn't matter because a malicious user can still execute sysenter.
> We do have to deal with that somehow, so we have to put something
> safe in there.

All values are safe as far as the kernel is concerned.  The sysexit
instruction is the only consumer of the value.  The sysexit instruction
interprets the value as a _user_ virtual address, and jumps to it,
in _user_ mode.  If user code does not like jumping to a random address
when vdso_enabled is zero, then user code should not use sysenter
when vdso_enabled is zero.  But execution of kernel code is not
affected in any way regardless of the value.

I'd be happy to set ->sysenter_return to 0 (or any other suggested value)
when vdso_enabled is 0, but this would be a politeness only.  There is
no added security risk to the kernel by leaving it unset.

>>Correct.  Changing vdso_enabled from 0 to non-zero must be prepared
>>to lose this race if it is not prevented.  Ordinarily it won't matter
>>because the administrator will perform such changes at a "quiet" time.
>>
> 
> 
> We have to deal with all the possibilities here, too.

Documentation is one method of dealing with it.

-- 
John Reiser, jreiser@BitWagon.com

Re: + fully-honor-vdso_enabled.patch added to -mm tree

[Oleg Nesterov]

On 03/02, John Reiser wrote:
>
> Oleg Nesterov wrote:
> 
> > Still, I don't understand why we don't pass NEW_AUX_ENT(AT_SYSINFO) when
> > vdso_enabled == 0. We don't need linux-gate.so to use __kernel_vsyscall,
> > we have FIX_VDSO. In that case we should s/PAGE_KERNEL_RO/PAGE_READONLY/
> > of course. I guess the reason is some magic in glibc.
> 
> In the past there were problems where user code could not single-step
> (or even read the code of) such system calls.  Can these two operations
> be performed today with what you propose, particularly if FIX_VDSO is
> near 0xfffff000 ?

No, we can't COW the gate page, it lives in swapper_pg_dir. I am not sure
why we could not read it, we have gate_vma for that.

So, the reason is debugger. Thanks.

Oleg.

Re: + fully-honor-vdso_enabled.patch added to -mm tree

[Paul Mundt]

On Sat, Mar 03, 2007 at 01:18:54AM +0300, Oleg Nesterov wrote:
> On 03/02, John Reiser wrote:
> > Paul Mundt has commented on setup_rt_frame() and provided a patch which
> > bullet-proofs that area.  I will include that patch into the next revision.
> 
> Confused. I still think his patch incomplete. Don't we need the same check
> in setup_rt_frame() ?
> 
Yes, you're right. The patch is missing the proper restorer logic in the
setup_rt_frame() case. This should handle both of them, restoring the
previous logic when the VDSO is disabled.

Signed-off-by: Paul Mundt <lethal@linux-sh.org>

--

 arch/i386/kernel/signal.c |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/i386/kernel/signal.c b/arch/i386/kernel/signal.c
index 4f99e87..26c2497 100644
--- a/arch/i386/kernel/signal.c
+++ b/arch/i386/kernel/signal.c
@@ -350,7 +350,7 @@ static int setup_frame(int sig, struct k_sigaction *ka,
 			goto give_sigsegv;
 	}
 
-	if (current->binfmt->hasvdso)
+	if (current->binfmt->hasvdso && current->mm->context.vdso)
 		restorer = (void *)VDSO_SYM(&__kernel_sigreturn);
 	else
 		restorer = (void *)&frame->retcode;
@@ -449,17 +449,20 @@ static int setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
 		goto give_sigsegv;
 
 	/* Set up to return from userspace.  */
-	restorer = (void *)VDSO_SYM(&__kernel_rt_sigreturn);
+	if (current->binfmt->hasvdso && current->mm->context.vdso)
+		restorer = (void *)VDSO_SYM(&__kernel_rt_sigreturn);
+	else
+		restorer = (void *)&frame->retcode;
 	if (ka->sa.sa_flags & SA_RESTORER)
 		restorer = ka->sa.sa_restorer;
 	err |= __put_user(restorer, &frame->pretcode);
-	 
+
 	/*
 	 * This is movl $,%eax ; int $0x80
 	 *
-	 * WE DO NOT USE IT ANY MORE! It's only left here for historical
-	 * reasons and because gdb uses it as a signature to notice
-	 * signal handler stack frames.
+	 * This is not used in the general case! It's left here for
+	 * handling toggling of the VDSO state and because gdb uses it
+	 * as a signature to notice signal handler stack frames.
 	 */
 	err |= __put_user(0xb8, (char __user *)(frame->retcode+0));
 	err |= __put_user(__NR_rt_sigreturn, (int __user *)(frame->retcode+1));

Re: + fully-honor-vdso_enabled.patch added to -mm tree

[Oleg Nesterov]

On 03/05, Paul Mundt wrote:
>
> On Sat, Mar 03, 2007 at 01:18:54AM +0300, Oleg Nesterov wrote:
> > On 03/02, John Reiser wrote:
> > > Paul Mundt has commented on setup_rt_frame() and provided a patch which
> > > bullet-proofs that area.  I will include that patch into the next revision.
> > 
> > Confused. I still think his patch incomplete. Don't we need the same check
> > in setup_rt_frame() ?
> > 
> Yes, you're right. The patch is missing the proper restorer logic in the
> setup_rt_frame() case. This should handle both of them, restoring the
> previous logic when the VDSO is disabled.
> 
> Signed-off-by: Paul Mundt <lethal@linux-sh.org>
> 
> --
> 
>  arch/i386/kernel/signal.c |   15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/i386/kernel/signal.c b/arch/i386/kernel/signal.c
> index 4f99e87..26c2497 100644
> --- a/arch/i386/kernel/signal.c
> +++ b/arch/i386/kernel/signal.c
> @@ -350,7 +350,7 @@ static int setup_frame(int sig, struct k_sigaction *ka,
>  			goto give_sigsegv;
>  	}
>  
> -	if (current->binfmt->hasvdso)
> +	if (current->binfmt->hasvdso && current->mm->context.vdso)

I think this is correct, but a bit strange.

The "->context.vdso != NULL" check relies on the fact that .vdso == NULL
after mm_alloc (because arch_setup_additional_pages() doesn' initialize
it when vdso_enabled == 0, and it has to be != NULL otherwise).

This means that binfmt->hasvdso in essence is not used, at least for i386.
Isn't it better to kill ->hasvdso and just use ->context.vdso ? Every usage
of ->hasvdso should also check ->context.vdso anyway.

Oleg.

Re: + fully-honor-vdso_enabled.patch added to -mm tree

[Paul Mundt]

On Mon, Mar 05, 2007 at 01:54:44PM +0300, Oleg Nesterov wrote:
> On 03/05, Paul Mundt wrote:
> > -	if (current->binfmt->hasvdso)
> > +	if (current->binfmt->hasvdso && current->mm->context.vdso)
> 
> I think this is correct, but a bit strange.
> 
> The "->context.vdso != NULL" check relies on the fact that .vdso == NULL
> after mm_alloc (because arch_setup_additional_pages() doesn' initialize
> it when vdso_enabled == 0, and it has to be != NULL otherwise).
> 
> This means that binfmt->hasvdso in essence is not used, at least for i386.
> Isn't it better to kill ->hasvdso and just use ->context.vdso ? Every usage
> of ->hasvdso should also check ->context.vdso anyway.
> 
That was my thought as well, it looks like it's only needed for x86_64. We
don't look at binfmt->hasvdso on SH at all at least, and it works fine
there.. I left it in in the patch since I figured x86 had a reason for
checking it, but perhaps someone who knows the x86/x86_64 interaction
can comment on this.