Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 3 Jul 2019 15:46:00 -0700
From: Kees Cook <>
To: Romain Perier <>
Cc: Kernel Hardening <>,
	Shyam Saini <>
Subject: Re: refactor tasklets to avoid unsigned long argument

On Wed, Jul 03, 2019 at 05:48:42PM +0200, Romain Perier wrote:
> Mhhh, so If I understand it right, the purpose of this task is to
> remove the "unsigned long data"  argument passed to tasklet_init() ,
> that
> is mostly used to pass the pointer of the parent structure that
> contains the tasklet_struct to the handler.

Right. The idea being that when a tasklet is stored in memory, it no
longer contains both the callback function pointer AND the argument to
pass it. This is the same problem that existed for struct timer_list.
You can see more details about this in at the start of the timer_list

> We don't change the API of tasklet, we simply remove the code that use
> this "unsigned long data" wrongly to pass the pointer of the parent
> structure
> (by using container_of() or something equivalent).

Kind of. In the timer_list case, there were some places were actual data
(and not a pointer) was being passed -- those needed some thought to
convert sanely. I'm hoping that the tasklets are a much smaller part of
the kernel and won't pose as much of a problem, but I haven't studied

> For example this is the case in:   drivers/firewire/ohci.c   or
> drivers/s390/block/dasd.c  .


struct ar_context {
        struct tasklet_struct tasklet;

static void ar_context_tasklet(unsigned long data)
        struct ar_context *ctx = (struct ar_context *)data;

static int ar_context_init(...)
        tasklet_init(&ctx->tasklet, ar_context_tasklet, (unsigned long)ctx);

this could instead be:

static void ar_context_tasklet(struct tasklet_struct *tasklet)
	struct ar_context *ctx = container_of(tasklet, typeof(*ctx), tasklet);

static int ar_context_init(...)
        tasklet_setup(&ctx->tasklet, ar_context_tasklet);

> Several question come:
> 1. I am not sure but, do we need to modify the prototype of
> tasklet_init() ?  well, this "unsigned long data" might be use for
> something else that pass the pointer of the parent struct. So I would
> say "no"

Yes, the final step in the refactoring would be to modify the tasklet_init()
prototype. I've included some example commits from the timer_list
refactoring, but look at the history of include/linux/timer.h and
kernel/time/timer.c for more details.

I would expect the refactoring to follow similar changes to timer_list:

- add a new init API (perhaps tasklet_setup() to follow timer_setup()?)
  that passes the tasklet pointer to tasklet_init(), and casts the
- convert all users to the new prototype
- remove the "data" member and convert the callback infrastructure to
  pass the tasklet pointer
- and then clean up anything (cast macros, etc)

Hopefully tasklet doesn't have a lot of open-coded initialization. This
is what made timer_list such a challenge. Stuff like this:

> 2. In term of security, this is a problem ? Or this is just an
> improvement to force developpers to do things correctly ?

It's a reduction in attack surface (attacker has less control
over the argument if the function pointer is overwritten) and it
provides a distinct prototype for CFI, to make is separate from other
functions that take a single unsigned long argument (e.g. before the
timer_list refactoring, all timer callbacks had the same prototype as
native_write_cr4(), making them a powerful target to control on x86).

For examples of the timer_list attacks (which would likely match a
tasklet attack if one got targeted), see "retire_blk_timer" in:

There's also some more detail on the timer_list work in my blog post
for v4.15:

> I will update the WIKI


Thanks for looking at this! I hope it's not at bad as timer_list. :)

Kees Cook

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.