[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sun, 21 Jul 2019 19:55:33 +0200
From: Romain Perier <romain.perier@...il.com>
To: Kees Cook <keescook@...omium.org>
Cc: Kernel Hardening <kernel-hardening@...ts.openwall.com>,
Shyam Saini <mayhs11saini@...il.com>
Subject: Re: refactor tasklets to avoid unsigned long argument
Ok, thanks for these explanations.
The task is in progress, you can follow the status here :
https://salsa.debian.org/rperier-guest/linux-tree/tree/tasklet_init
(the commit messages are tagged WIP, I will add a long message and
signed-off-by , when it's done)
Regards,
Romain
Le jeu. 4 juil. 2019 à 00:46, Kees Cook <keescook@...omium.org> a écrit :
>
> 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
> refactoring:
> https://git.kernel.org/linus/686fef928bba6be13cabe639f154af7d72b63120
>
> > 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
> it.
>
> > For example this is the case in: drivers/firewire/ohci.c or
> > drivers/s390/block/dasd.c .
>
> Right:
>
> 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
> callback.
> https://git.kernel.org/linus/686fef928bba6be13cabe639f154af7d72b63120
> - convert all users to the new prototype
> https://git.kernel.org/linus/e99e88a9d2b067465adaa9c111ada99a041bef9a
> - remove the "data" member and convert the callback infrastructure to
> pass the tasklet pointer
> https://git.kernel.org/linus/c1eba5bcb6430868427e0b9d1cd1205a07302f06
> - and then clean up anything (cast macros, etc)
> https://git.kernel.org/linus/354b46b1a0adda1dd5b7f0bc2a5604cca091be5f
>
> Hopefully tasklet doesn't have a lot of open-coded initialization. This
> is what made timer_list such a challenge. Stuff like this:
> https://git.kernel.org/linus/b9eaf18722221ef8b2bd6a67240ebe668622152a
>
> > 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:
> https://googleprojectzero.blogspot.com/2017/05/exploiting-linux-kernel-via-packet.html
>
> There's also some more detail on the timer_list work in my blog post
> for v4.15:
> https://outflux.net/blog/archives/2018/02/05/security-things-in-linux-v4-15/
>
> > I will update the WIKI
>
> Awesome!
>
> 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.
