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

[Mathias Krause]

On 5 October 2014 23:06, Oleg Nesterov <oleg@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx>  # 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.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/