Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1713410 > unrolled thread

refactoring timers to avoid init_timer*()

Back to article view | Back to linux.kernel

Contents

  refactoring timers to avoid init_timer*() Kees Cook <keescook@google.com> - 2017-08-17 01:30 +0200
    Re: refactoring timers to avoid init_timer*() Kees Cook <keescook@google.com> - 2017-08-17 16:40 +0200
    Re: refactoring timers to avoid init_timer*() Thomas Gleixner <tglx@linutronix.de> - 2017-08-17 16:40 +0200
    Re: refactoring timers to avoid init_timer*() Christoph Hellwig <hch@infradead.org> - 2017-08-17 16:50 +0200
      Re: refactoring timers to avoid init_timer*() Kees Cook <keescook@google.com> - 2017-08-17 17:10 +0200

#1713410 — refactoring timers to avoid init_timer*()

Hi,

So, my earlier patch[1] to add canaries to the timer struct did not
handle many many cases, as I've uncovered. To have a sensible canary,
it needs to be written at function-assignment time, but right now
things are extremely ad-hoc in the timer API, making it non-trivial to
figure out if the code changed the function or if some attacker is
exploiting a flaw to overwrite the timer struct function pointer.

What I'd like to do is to eliminate all the uses of init_timer*() in
favor of setup_timer*() which already includes the function as part of
the initialization (which is where a canary could be generated). There
are a couple cases where a timer user switches the function out
intentionally after setup, but that is the very rare case. Those could
use a new helper that would set the function (and canary) after an
earlier setup_timer() call (or maybe just re-call setup_timer()?).

In the process I noticed that we already have
scripts/coccinelle/api/setup_timer.cocci to detect existing cases of:

init_timer(t);
t->function = func;
t->data = data;

And replace it with: setup_timer(t, func, data);

Another pattern was:

t->expires = when;
add_timer(t);

Which can be replaced with mod_timer(t, when);

So, I've created scripts/coccinelle/api/mod_timer.cocci for the
latter, and done a few passes with manual review. The current result
doesn't fully eliminate init_timer() yet, but it gets much closer. I
just wanted to be sure that this whole clean-up would actually be
welcome before I try to nail down the last many cases.

You can see it here:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/timer/refactor-exploded

135 commits (split by maintainer):
 243 files changed, 583 insertions(+), 1055 deletions(-)

-Kees

[1] http://www.openwall.com/lists/kernel-hardening/2017/08/08/2

-- 
Kees Cook
Pixel Security

[toc] | [next] | [standalone]

#1714092

On Thu, Aug 17, 2017 at 7:30 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Wed, 16 Aug 2017, Kees Cook wrote:
>> In the process I noticed that we already have
>> scripts/coccinelle/api/setup_timer.cocci to detect existing cases of:
>>
>> init_timer(t);
>> t->function = func;
>> t->data = data;
>>
>> And replace it with: setup_timer(t, func, data);
>>
>> Another pattern was:
>>
>> t->expires = when;
>> add_timer(t);
>>
>> Which can be replaced with mod_timer(t, when);
>>
>> So, I've created scripts/coccinelle/api/mod_timer.cocci for the
>> latter, and done a few passes with manual review. The current result
>> doesn't fully eliminate init_timer() yet, but it gets much closer. I
>> just wanted to be sure that this whole clean-up would actually be
>> welcome before I try to nail down the last many cases.
>
> I think it's worth the trouble, but rather than having a gazillion of
> commits with the same changelog, we should do that based on a cocci script
> right before the next rc1 in one go and be done with it.
>
> That will cover most of the init_timer() cases and we can fixup the
> remaining few oddballs manually after that.

Okay, that sounds good. I'll work on improving the cocci scripts. For
example, I noticed that it doesn't notice having assignments _before_
the init_timer() call:

timer->function = func;
timer->data = data;
init_timer(timer);

Whee. :)

BTW, I may back off on doing ->expires = when; add_timer(timer) -->
mod_timer(timer, when) changes since that actually removes a sanity
check in add_timer().

My plans currently are collapsing all the open-coded init_timer*()s
into setup_*timer() and then adding something like aim_timer(timer,
func, data) for post-setup function/data changes.

-Kees

>
> I just noticed that we have the same pattern with hrtimer_init(). I had a
> stab on adding hrtimer_setup() and friends which takes a function argument
> and converted the bulk with coccinelle.
>
>     82 files changed, 186 insertions(+), 210 deletions(-)
>
> We can do that in the same sweep as the init_timer() one and then you can
> do the canary magic on hrtimers as well.
>
> Thanks,
>
>         tglx
>
>



-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1714096

On Wed, 16 Aug 2017, Kees Cook wrote:
> In the process I noticed that we already have
> scripts/coccinelle/api/setup_timer.cocci to detect existing cases of:
> 
> init_timer(t);
> t->function = func;
> t->data = data;
> 
> And replace it with: setup_timer(t, func, data);
> 
> Another pattern was:
> 
> t->expires = when;
> add_timer(t);
> 
> Which can be replaced with mod_timer(t, when);
> 
> So, I've created scripts/coccinelle/api/mod_timer.cocci for the
> latter, and done a few passes with manual review. The current result
> doesn't fully eliminate init_timer() yet, but it gets much closer. I
> just wanted to be sure that this whole clean-up would actually be
> welcome before I try to nail down the last many cases.

I think it's worth the trouble, but rather than having a gazillion of
commits with the same changelog, we should do that based on a cocci script
right before the next rc1 in one go and be done with it.

That will cover most of the init_timer() cases and we can fixup the
remaining few oddballs manually after that.

I just noticed that we have the same pattern with hrtimer_init(). I had a
stab on adding hrtimer_setup() and friends which takes a function argument
and converted the bulk with coccinelle.

    82 files changed, 186 insertions(+), 210 deletions(-)

We can do that in the same sweep as the init_timer() one and then you can
do the canary magic on hrtimers as well.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1714103

Instea of bloating the timer even more we should kill off
the data field eventually, which should give you the same
protection.

See my proposal and the related discussion here:

http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1397209.html

[toc] | [prev] | [next] | [standalone]

#1714108

On Thu, Aug 17, 2017 at 7:40 AM, Christoph Hellwig <hch@infradead.org> wrote:
> Instea of bloating the timer even more we should kill off
> the data field eventually, which should give you the same
> protection.
>
> See my proposal and the related discussion here:
>
> http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1397209.html

Ah! Yes, very cool. Most callbacks could be converted to the new timer
callback pretty easily. Some, though, pass non-pointer data in the
"data" argument. Those can likely be moved to somewhere else, though.
I think it should be possible to just perform all the conversions.

Regardless, getting rid of init_timer*() and eliminating the
open-coded callback assignments should help. I'll keep grinding on
that.

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web