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 <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Brad Spengler <[email protected]>
Cc: PaX Team <[email protected]>
Cc: <[email protected]> # 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
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: <[email protected]> # 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.
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
On 5 October 2014 23:06, Oleg Nesterov <[email protected]> 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: <[email protected]> # 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.
>
On 10/05, Mathias Krause wrote:
>
> On 5 October 2014 23:06, Oleg Nesterov <[email protected]> wrote:
> > On 10/04, Mathias Krause wrote:
> >>
> >> Cc: <[email protected]> # 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.
Commit-ID: 6891c4509c792209c44ced55a60f13954cb50ef4
Gitweb: http://git.kernel.org/tip/6891c4509c792209c44ced55a60f13954cb50ef4
Author: Mathias Krause <[email protected]>
AuthorDate: Sat, 4 Oct 2014 23:06:39 +0200
Committer: Thomas Gleixner <[email protected]>
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 <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Brad Spengler <[email protected]>
Cc: PaX Team <[email protected]>
Cc: <[email protected]> # v2.6.28+
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Thomas Gleixner <[email protected]>
---
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;