siginfo memory leak?

siginfo memory leak?

[Michal Hocko]

Hi,
Aleksa has reported that strace tells a bogus si_errno while debugging
something on s390:
[pid 20799] --- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_MAPERR, si_errno=2510266, si_addr=0x100000000000000}

A quick look into do_sigsegv shows that siginfo is not completely
initialized and it indeed might leak the previous stack content
which will later gets to userspace. So unless I am missing something
we need something like the trivial patch below. I have tried to look
around and it seems that this is not the only place...

x86 do_error_trap doesn't do any initialization at all! It is hard to
tell other places. I have checked some and most of them do some
(partial) initialization.

So my primary question is whether we want to fix all those potential
places one by one or come up with something more systematic (e.g. a
macro to declare on stack siginfo). Btw. I am not even sure partial
initializations are correct and memset should be used unconditioanlly
(e.g. fill_sigtrap_info does do that).
---
diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 791a4146052c..41913fac14e4 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -248,6 +248,7 @@ static noinline void do_sigsegv(struct pt_regs *regs, int si_code)
 	si.si_signo = SIGSEGV;
 	si.si_code = si_code;
 	si.si_addr = (void __user *)(regs->int_parm_long & __FAIL_ADDR_MASK);
+	si.si_errno = 0;
 	force_sig_info(SIGSEGV, &si, current);
 }
 
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index ade185a46b1d..f8b66ddbb47d 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -286,6 +286,7 @@ static void do_error_trap(struct pt_regs *regs, long error_code, char *str,
 
 	if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) !=
 			NOTIFY_STOP) {
+		memset(&info, 0, sizeof(info));
 		conditional_sti(regs);
 		do_trap(trapnr, signr, str, regs, error_code,
 			fill_trap_info(regs, signr, trapnr, &info));
 
-- 
Michal Hocko
SUSE Labs

Re: siginfo memory leak?

[Martin Schwidefsky]

On Mon, 23 May 2016 13:16:30 +0200
Michal Hocko <mhocko@kernel.org> wrote:

> Hi,
> Aleksa has reported that strace tells a bogus si_errno while debugging
> something on s390:
> [pid 20799] --- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_MAPERR, si_errno=2510266, si_addr=0x100000000000000}

That is a bug.
 
> A quick look into do_sigsegv shows that siginfo is not completely
> initialized and it indeed might leak the previous stack content
> which will later gets to userspace. So unless I am missing something
> we need something like the trivial patch below. I have tried to look
> around and it seems that this is not the only place...

Indeed, for s390 four bytes of the kernel stack gets leaked to user space.
That needs fixing.

> x86 do_error_trap doesn't do any initialization at all! It is hard to
> tell other places. I have checked some and most of them do some
> (partial) initialization.
> 
> So my primary question is whether we want to fix all those potential
> places one by one or come up with something more systematic (e.g. a
> macro to declare on stack siginfo). Btw. I am not even sure partial
> initializations are correct and memset should be used unconditioanlly
> (e.g. fill_sigtrap_info does do that).
> ---
> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> index 791a4146052c..41913fac14e4 100644
> --- a/arch/s390/mm/fault.c
> +++ b/arch/s390/mm/fault.c
> @@ -248,6 +248,7 @@ static noinline void do_sigsegv(struct pt_regs *regs, int si_code)
>  	si.si_signo = SIGSEGV;
>  	si.si_code = si_code;
>  	si.si_addr = (void __user *)(regs->int_parm_long & __FAIL_ADDR_MASK);
> +	si.si_errno = 0;
>  	force_sig_info(SIGSEGV, &si, current);
>  }
> 

The other for place where s390 calls force_sig_info are correct.
Only do_sigsegv misses the clear of si_errno.

> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index ade185a46b1d..f8b66ddbb47d 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -286,6 +286,7 @@ static void do_error_trap(struct pt_regs *regs, long error_code, char *str,
> 
>  	if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) !=
>  			NOTIFY_STOP) {
> +		memset(&info, 0, sizeof(info));
>  		conditional_sti(regs);
>  		do_trap(trapnr, signr, str, regs, error_code,
>  			fill_trap_info(regs, signr, trapnr, &info));
> 

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: siginfo memory leak?

[Michal Hocko]

On Mon 23-05-16 14:43:19, Martin Schwidefsky wrote:
> On Mon, 23 May 2016 13:16:30 +0200
[...]
> > diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> > index 791a4146052c..41913fac14e4 100644
> > --- a/arch/s390/mm/fault.c
> > +++ b/arch/s390/mm/fault.c
> > @@ -248,6 +248,7 @@ static noinline void do_sigsegv(struct pt_regs *regs, int si_code)
> >  	si.si_signo = SIGSEGV;
> >  	si.si_code = si_code;
> >  	si.si_addr = (void __user *)(regs->int_parm_long & __FAIL_ADDR_MASK);
> > +	si.si_errno = 0;
> >  	force_sig_info(SIGSEGV, &si, current);
> >  }
> > 
> 
> The other for place where s390 calls force_sig_info are correct.
> Only do_sigsegv misses the clear of si_errno.

I can send a full patch with the proper changelog but I am really
wondering whether we can plug this in a more systematic way. If you
prefer a small s390 specific I will do it right away though. Same
applies to x86 one.
 
> > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> > index ade185a46b1d..f8b66ddbb47d 100644
> > --- a/arch/x86/kernel/traps.c
> > +++ b/arch/x86/kernel/traps.c
> > @@ -286,6 +286,7 @@ static void do_error_trap(struct pt_regs *regs, long error_code, char *str,
> > 
> >  	if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) !=
> >  			NOTIFY_STOP) {
> > +		memset(&info, 0, sizeof(info));
> >  		conditional_sti(regs);
> >  		do_trap(trapnr, signr, str, regs, error_code,
> >  			fill_trap_info(regs, signr, trapnr, &info));
> > 
> 
> -- 
> blue skies,
>    Martin.
> 
> "Reality continues to ruin my life." - Calvin.

-- 
Michal Hocko
SUSE Labs

Re: siginfo memory leak?

[Martin Schwidefsky]

On Mon, 23 May 2016 15:05:38 +0200
Michal Hocko <mhocko@kernel.org> wrote:

> On Mon 23-05-16 14:43:19, Martin Schwidefsky wrote:
> > On Mon, 23 May 2016 13:16:30 +0200
> [...]
> > > diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> > > index 791a4146052c..41913fac14e4 100644
> > > --- a/arch/s390/mm/fault.c
> > > +++ b/arch/s390/mm/fault.c
> > > @@ -248,6 +248,7 @@ static noinline void do_sigsegv(struct pt_regs *regs, int si_code)
> > >  	si.si_signo = SIGSEGV;
> > >  	si.si_code = si_code;
> > >  	si.si_addr = (void __user *)(regs->int_parm_long & __FAIL_ADDR_MASK);
> > > +	si.si_errno = 0;
> > >  	force_sig_info(SIGSEGV, &si, current);
> > >  }
> > > 
> > 
> > The other for place where s390 calls force_sig_info are correct.
> > Only do_sigsegv misses the clear of si_errno.
> 
> I can send a full patch with the proper changelog but I am really
> wondering whether we can plug this in a more systematic way. If you
> prefer a small s390 specific I will do it right away though. Same
> applies to x86 one.

Why not fix the bug with a small patch and then provide the "big"
solution? A potential information leak is not good ..

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: siginfo memory leak?

[Michal Hocko]

On Mon 23-05-16 15:29:21, Martin Schwidefsky wrote:
> On Mon, 23 May 2016 15:05:38 +0200
> Michal Hocko <mhocko@kernel.org> wrote:
> 
> > On Mon 23-05-16 14:43:19, Martin Schwidefsky wrote:
> > > On Mon, 23 May 2016 13:16:30 +0200
> > [...]
> > > > diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> > > > index 791a4146052c..41913fac14e4 100644
> > > > --- a/arch/s390/mm/fault.c
> > > > +++ b/arch/s390/mm/fault.c
> > > > @@ -248,6 +248,7 @@ static noinline void do_sigsegv(struct pt_regs *regs, int si_code)
> > > >  	si.si_signo = SIGSEGV;
> > > >  	si.si_code = si_code;
> > > >  	si.si_addr = (void __user *)(regs->int_parm_long & __FAIL_ADDR_MASK);
> > > > +	si.si_errno = 0;
> > > >  	force_sig_info(SIGSEGV, &si, current);
> > > >  }
> > > > 
> > > 
> > > The other for place where s390 calls force_sig_info are correct.
> > > Only do_sigsegv misses the clear of si_errno.
> > 
> > I can send a full patch with the proper changelog but I am really
> > wondering whether we can plug this in a more systematic way. If you
> > prefer a small s390 specific I will do it right away though. Same
> > applies to x86 one.
> 
> Why not fix the bug with a small patch and then provide the "big"
> solution? A potential information leak is not good ..

Fair enough. Will send two patches for the places which do not do the
proper initialization.

-- 
Michal Hocko
SUSE Labs

[PATCH] s390: fix info leak in do_sigsegv

[Michal Hocko]

OK, Martin would prefer a simple patch so here we go.
---
>From de1ad037f3181e795ef0e66a61b8fbe1157f66cc Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Mon, 23 May 2016 15:35:51 +0200
Subject: [PATCH] s390: fix info leak in do_sigsegv

Aleksa has reported incorrect si_errno value when stracing task which
received SIGSEGV:
[pid 20799] --- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_MAPERR, si_errno=2510266, si_addr=0x100000000000000}

The reason seems to be that do_sigsegv is not initializing siginfo
structure defined on the stack completely so it will leak 4B of
the previous stack content. Fix it simply by initializing si_errno
to 0 (same as do_sigbus does already).

Cc: stable # introduced pre-git times
Reported-by: Aleksa Sarai <asarai@suse.de>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 arch/s390/mm/fault.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 7a3144017301..19288c1b36d3 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -250,6 +250,7 @@ static noinline void do_sigsegv(struct pt_regs *regs, int si_code)
 
 	report_user_fault(regs, SIGSEGV, 1);
 	si.si_signo = SIGSEGV;
+	si.si_errno = 0;
 	si.si_code = si_code;
 	si.si_addr = (void __user *)(regs->int_parm_long & __FAIL_ADDR_MASK);
 	force_sig_info(SIGSEGV, &si, current);
-- 
2.8.1

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] s390: fix info leak in do_sigsegv

[Martin Schwidefsky]

On Mon, 23 May 2016 15:43:20 +0200
Michal Hocko <mhocko@kernel.org> wrote:

> OK, Martin would prefer a simple patch so here we go.
> ---
> From de1ad037f3181e795ef0e66a61b8fbe1157f66cc Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Mon, 23 May 2016 15:35:51 +0200
> Subject: [PATCH] s390: fix info leak in do_sigsegv
> 
> Aleksa has reported incorrect si_errno value when stracing task which
> received SIGSEGV:
> [pid 20799] --- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_MAPERR, si_errno=2510266, si_addr=0x100000000000000}
> 
> The reason seems to be that do_sigsegv is not initializing siginfo
> structure defined on the stack completely so it will leak 4B of
> the previous stack content. Fix it simply by initializing si_errno
> to 0 (same as do_sigbus does already).
> 
> Cc: stable # introduced pre-git times
> Reported-by: Aleksa Sarai <asarai@suse.de>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  arch/s390/mm/fault.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> index 7a3144017301..19288c1b36d3 100644
> --- a/arch/s390/mm/fault.c
> +++ b/arch/s390/mm/fault.c
> @@ -250,6 +250,7 @@ static noinline void do_sigsegv(struct pt_regs *regs, int si_code)
> 
>  	report_user_fault(regs, SIGSEGV, 1);
>  	si.si_signo = SIGSEGV;
> +	si.si_errno = 0;
>  	si.si_code = si_code;
>  	si.si_addr = (void __user *)(regs->int_parm_long & __FAIL_ADDR_MASK);
>  	force_sig_info(SIGSEGV, &si, current);

Applied to linux-s390:fixes. Thanks.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

[PATCH] x86: fix potential memleak in do_error_trap

[Michal Hocko]

And here is the x86 one. I haven't found others so far but I haven't
checked other architectures and I might have missed some callpaths for
x86 as well. Also please note this hasn't been tested properly and
it is based on the code reading.
---
>From fa8e84058c243f81a49c847624daaf935efdeb5a Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Mon, 23 May 2016 15:47:28 +0200
Subject: [PATCH] x86: fix potential memleak in do_error_trap

do_error_trap defines on stack siginfo structure which is then sent down
to do_trap -> force_sig_info without initializing it. __send_signal ->
copy_siginfo will copy the content for later use when the signal is
dequeued. This information might later leak into userspace. Fix it by
clearing the whole siginfo in do_error_trap before sending it to
do_trap.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 arch/x86/kernel/traps.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index d1590486204a..945b4dfc02e6 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -271,6 +271,7 @@ static void do_error_trap(struct pt_regs *regs, long error_code, char *str,
 
 	if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) !=
 			NOTIFY_STOP) {
+		memset(&info, 0, sizeof(info));
 		cond_local_irq_enable(regs);
 		do_trap(trapnr, signr, str, regs, error_code,
 			fill_trap_info(regs, signr, trapnr, &info));
-- 
2.8.1

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] x86: fix potential memleak in do_error_trap

[Oleg Nesterov]

On 05/23, Michal Hocko wrote:
>
> @@ -271,6 +271,7 @@ static void do_error_trap(struct pt_regs *regs, long error_code, char *str,
>
>  	if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) !=
>  			NOTIFY_STOP) {
> +		memset(&info, 0, sizeof(info));
>  		cond_local_irq_enable(regs);
>  		do_trap(trapnr, signr, str, regs, error_code,
>  			fill_trap_info(regs, signr, trapnr, &info));

at first glance fill_trap_info() initializes everything we will copy
to user-space in copy_siginfo_to_user(__SI_FAULT).

But even if not, shuldn't we change fill_trap_info() instead ?

Oleg.

Re: [PATCH] x86: fix potential memleak in do_error_trap

[Michal Hocko]

On Mon 23-05-16 17:33:55, Oleg Nesterov wrote:
> On 05/23, Michal Hocko wrote:
> >
> > @@ -271,6 +271,7 @@ static void do_error_trap(struct pt_regs *regs, long error_code, char *str,
> >
> >  	if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) !=
> >  			NOTIFY_STOP) {
> > +		memset(&info, 0, sizeof(info));
> >  		cond_local_irq_enable(regs);
> >  		do_trap(trapnr, signr, str, regs, error_code,
> >  			fill_trap_info(regs, signr, trapnr, &info));
> 
> at first glance fill_trap_info() initializes everything we will copy
> to user-space in copy_siginfo_to_user(__SI_FAULT).

Ohh, you are right. Dunno, how I managed to miss it. Sorry about the
noise.
 
> But even if not, shuldn't we change fill_trap_info() instead ?

Yes that would be the proper place.
-- 
Michal Hocko
SUSE Labs

Re: siginfo memory leak?

[Oleg Nesterov]

On 05/23, Michal Hocko wrote:
>
> x86 do_error_trap doesn't do any initialization at all!

it actually does, please see fill_trap_info() although I'll try to
re-check if it initializes everything we need.

Oleg.