[PATCH] posix-timers: fix stack info leak in timer_create()

[PATCH] posix-timers: fix stack info leak in timer_create()

[Mathias Krause]

If userland creates a timer without specifying a sigevent info, we'll
create one ourself, using a stack local variable. Particularly will we
use the timer ID as sival_int. But as sigev_value is a union containing
a pointer and an int, that assignment will only partially initialize
sigev_value on systems where the size of a pointer is bigger than the
size of an int. On such systems we'll copy the uninitialized stack bytes
from the timer_create() call to userland when the timer actually fires
and we're going to deliver the signal.

Initialize sigev_value with 0 to plug the stack info leak.

Found in the PaX patch, written by the PaX Team.

Fixes: 5a9fa7307285 ("posix-timers: kill ->it_sigev_signo and...")
Signed-off-by: Mathias Krause <minipli@googlemail.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Brad Spengler <spender@grsecurity.net>
Cc: PaX Team <pageexec@freemail.hu>
Cc: <stable@vger.kernel.org>	# v2.6.28+
---
 kernel/time/posix-timers.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 42b463ad90f2..31ea01f42e1f 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -636,6 +636,7 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
 			goto out;
 		}
 	} else {
+		memset(&event.sigev_value, 0, sizeof(event.sigev_value));
 		event.sigev_notify = SIGEV_SIGNAL;
 		event.sigev_signo = SIGALRM;
 		event.sigev_value.sival_int = new_timer->it_id;
-- 
1.7.10.4

Re: [PATCH] posix-timers: fix stack info leak in timer_create()

[Oleg Nesterov]

On 10/04, Mathias Krause wrote:
>
> If userland creates a timer without specifying a sigevent info, we'll
> create one ourself, using a stack local variable. Particularly will we
> use the timer ID as sival_int. But as sigev_value is a union containing
> a pointer and an int, that assignment will only partially initialize
> sigev_value on systems where the size of a pointer is bigger than the
> size of an int. On such systems we'll copy the uninitialized stack bytes
> from the timer_create() call to userland when the timer actually fires
> and we're going to deliver the signal.

So we have a minor information leak,

> Cc: <stable@vger.kernel.org>	# v2.6.28+

not sure this is -stable material but I won't really argue.

> --- a/kernel/time/posix-timers.c
> +++ b/kernel/time/posix-timers.c
> @@ -636,6 +636,7 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
>  			goto out;
>  		}
>  	} else {
> +		memset(&event.sigev_value, 0, sizeof(event.sigev_value));
>  		event.sigev_notify = SIGEV_SIGNAL;
>  		event.sigev_signo = SIGALRM;
>  		event.sigev_value.sival_int = new_timer->it_id;

How about

	-	event.sigev_value.sival_int = new_timer->it_id;
	+	event.sigev_value = (sigval_t) { .sival_int = new_timer_id };

?

(btw, new_timer->sigq->info.si_tid initialization can use new_timer_id too)

this makes the initialization more explicit and can help gcc to optimize
this assignment although this is minor.

In any case this all looks confusing to me. sys_timer_create() does

	new_timer->sigq->info.si_value = event.sigev_value;
	new_timer->sigq->info.si_tid   = new_timer->it_id;

later, this writes to the differents members (_rt and _timer) in the
same union. But the comment in struct siginfo says that we should use
_timer. And copy_siginfo_to_user() reports si_tid and si_ptr, this
again reads _timer and _rt. This should actually work, _sigval should
have the same offset in both struct's, still it looks confusing imho.
Perhaps we should change

	#define si_value	_sifields._rt._sigval
	#define si_int		_sifields._rt._sigval.sival_int
	#define si_ptr		_sifields._rt._sigval.sival_ptr

to use _timer instead. Nevermind, this is off-topic.

Oleg.

Re: [PATCH] posix-timers: fix stack info leak in timer_create()

[Thomas Gleixner]

On Sun, 5 Oct 2014, Oleg Nesterov wrote:
> In any case this all looks confusing to me. sys_timer_create() does
> 
> 	new_timer->sigq->info.si_value = event.sigev_value;
> 	new_timer->sigq->info.si_tid   = new_timer->it_id;
> 
> later, this writes to the differents members (_rt and _timer) in the
> same union. But the comment in struct siginfo says that we should use
> _timer. And copy_siginfo_to_user() reports si_tid and si_ptr, this
> again reads _timer and _rt. This should actually work, _sigval should
> have the same offset in both struct's, still it looks confusing imho.

It does.

> Perhaps we should change
> 
> 	#define si_value	_sifields._rt._sigval
> 	#define si_int		_sifields._rt._sigval.sival_int
> 	#define si_ptr		_sifields._rt._sigval.sival_ptr
> 
> to use _timer instead. Nevermind, this is off-topic.

Well that would cause mqueue, perf and procfs to read/set the timer
fields. Odd as well.

Thanks,

	tglx

Re: [PATCH] posix-timers: fix stack info leak in timer_create()

[Mathias Krause]

On 5 October 2014 23:06, Oleg Nesterov <oleg@redhat.com> wrote:
> On 10/04, Mathias Krause wrote:
>>
>> If userland creates a timer without specifying a sigevent info, we'll
>> create one ourself, using a stack local variable. Particularly will we
>> use the timer ID as sival_int. But as sigev_value is a union containing
>> a pointer and an int, that assignment will only partially initialize
>> sigev_value on systems where the size of a pointer is bigger than the
>> size of an int. On such systems we'll copy the uninitialized stack bytes
>> from the timer_create() call to userland when the timer actually fires
>> and we're going to deliver the signal.
>
> So we have a minor information leak,
>

Yup.

>> Cc: <stable@vger.kernel.org>  # v2.6.28+
>
> not sure this is -stable material but I won't really argue.
>

This should fall into the class "security fixes". Info leaks like that
-- if flagged as such -- got backported regularly in the past. They
tend to get CVE IDs, even.

>> --- a/kernel/time/posix-timers.c
>> +++ b/kernel/time/posix-timers.c
>> @@ -636,6 +636,7 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
>>                       goto out;
>>               }
>>       } else {
>> +             memset(&event.sigev_value, 0, sizeof(event.sigev_value));
>>               event.sigev_notify = SIGEV_SIGNAL;
>>               event.sigev_signo = SIGALRM;
>>               event.sigev_value.sival_int = new_timer->it_id;
>
> How about
>
>         -       event.sigev_value.sival_int = new_timer->it_id;
>         +       event.sigev_value = (sigval_t) { .sival_int = new_timer_id };
>
> ?

Oh, well. It's a matter of taste, I guess. It won't fit mine ;) I
think it makes it slightly less readable.

Concerning optimizations, gcc is clever enough to optimize the
memset(0) without the structure cast and assignment trick. For me it
reduces the memset(0) to a single additional instruction: 'movq    $0,
-112(%rbp)'.

>
> (btw, new_timer->sigq->info.si_tid initialization can use new_timer_id too)
>
> this makes the initialization more explicit and can help gcc to optimize
> this assignment although this is minor.

True. Albeit not relevant for this patch.

>
> In any case this all looks confusing to me. sys_timer_create() does
>
>         new_timer->sigq->info.si_value = event.sigev_value;
>         new_timer->sigq->info.si_tid   = new_timer->it_id;
>
> later, this writes to the differents members (_rt and _timer) in the
> same union. But the comment in struct siginfo says that we should use
> _timer. And copy_siginfo_to_user() reports si_tid and si_ptr, this
> again reads _timer and _rt. This should actually work, _sigval should
> have the same offset in both struct's, still it looks confusing imho.
> Perhaps we should change
>
>         #define si_value        _sifields._rt._sigval
>         #define si_int          _sifields._rt._sigval.sival_int
>         #define si_ptr          _sifields._rt._sigval.sival_ptr
>
> to use _timer instead. Nevermind, this is off-topic.

Yeah, the _pad[] struct in _timer should ensure both _sigval members
should have the same offset. So I guess the _pad[] member is
intentionally there to solve the "aliasing" issue of si_value? But, in
fact, it's off-topic concerning the info leak fix.

Thanks,
Mathias

>
> Oleg.
>

Re: [PATCH] posix-timers: fix stack info leak in timer_create()

[Oleg Nesterov]

On 10/05, Mathias Krause wrote:
>
> On 5 October 2014 23:06, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 10/04, Mathias Krause wrote:
> >>
> >> Cc: <stable@vger.kernel.org>  # v2.6.28+
> >
> > not sure this is -stable material but I won't really argue.
> >
>
> This should fall into the class "security fixes". Info leaks like that
> -- if flagged as such -- got backported regularly in the past. They
> tend to get CVE IDs, even.

And imo, very often we do this for absolutely no reason. Like this fix
tries to do ;) But again, I won't really argue.

> >> --- a/kernel/time/posix-timers.c
> >> +++ b/kernel/time/posix-timers.c
> >> @@ -636,6 +636,7 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
> >>                       goto out;
> >>               }
> >>       } else {
> >> +             memset(&event.sigev_value, 0, sizeof(event.sigev_value));
> >>               event.sigev_notify = SIGEV_SIGNAL;
> >>               event.sigev_signo = SIGALRM;
> >>               event.sigev_value.sival_int = new_timer->it_id;
> >
> > How about
> >
> >         -       event.sigev_value.sival_int = new_timer->it_id;
> >         +       event.sigev_value = (sigval_t) { .sival_int = new_timer_id };
> >
> > ?
>
> Oh, well. It's a matter of taste, I guess.

Sure.

> It won't fit mine ;) I
> think it makes it slightly less readable.

To me it looks more readable, that is why I suggested this change.
But of course, of course, this is subjective, so I won't insist.

So I think this patch is fine.

Oleg.

[tip:timers/urgent] posix-timers: Fix stack info leak in timer_create()

[tip-bot for Mathias Krause]

Commit-ID:  6891c4509c792209c44ced55a60f13954cb50ef4
Gitweb:     http://git.kernel.org/tip/6891c4509c792209c44ced55a60f13954cb50ef4
Author:     Mathias Krause <minipli@googlemail.com>
AuthorDate: Sat, 4 Oct 2014 23:06:39 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sat, 25 Oct 2014 10:43:15 +0200

posix-timers: Fix stack info leak in timer_create()

If userland creates a timer without specifying a sigevent info, we'll
create one ourself, using a stack local variable. Particularly will we
use the timer ID as sival_int. But as sigev_value is a union containing
a pointer and an int, that assignment will only partially initialize
sigev_value on systems where the size of a pointer is bigger than the
size of an int. On such systems we'll copy the uninitialized stack bytes
from the timer_create() call to userland when the timer actually fires
and we're going to deliver the signal.

Initialize sigev_value with 0 to plug the stack info leak.

Found in the PaX patch, written by the PaX Team.

Fixes: 5a9fa7307285 ("posix-timers: kill ->it_sigev_signo and...")
Signed-off-by: Mathias Krause <minipli@googlemail.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Brad Spengler <spender@grsecurity.net>
Cc: PaX Team <pageexec@freemail.hu>
Cc: <stable@vger.kernel.org>	# v2.6.28+
Link: http://lkml.kernel.org/r/1412456799-32339-1-git-send-email-minipli@googlemail.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/time/posix-timers.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 42b463a..31ea01f 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -636,6 +636,7 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
 			goto out;
 		}
 	} else {
+		memset(&event.sigev_value, 0, sizeof(event.sigev_value));
 		event.sigev_notify = SIGEV_SIGNAL;
 		event.sigev_signo = SIGALRM;
 		event.sigev_value.sival_int = new_timer->it_id;