| Message ID | 20200716030847.1564131-4-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Modernize tasklet callback API | expand |
On Wed, Jul 15, 2020 at 08:08:47PM -0700, Kees Cook wrote: > From: Romain Perier <romain.perier@gmail.com> > > Nowadays, modern kernel subsystems that use callbacks pass the data > structure associated with a given callback as argument to the callback. > The tasklet subsystem remains one which passes an arbitrary unsigned > long to the callback function. This has several problems: > > - This keeps an extra field for storing the argument in each tasklet > data structure, it bloats the tasklet_struct structure with a redundant > .data field > > - No type checking can be performed on this argument. Instead of > using container_of() like other callback subsystems, it forces callbacks > to do explicit type cast of the unsigned long argument into the required > object type. > > - Buffer overflows can overwrite the .func and the .data field, so > an attacker can easily overwrite the function and its first argument > to whatever it wants. > > Add a new tasklet initialization API, via DECLARE_TASKLET() and > tasklet_setup(), which will replace the existing ones. > > This work is greatly inspired by the timer_struct conversion series, > see commit e99e88a9d2b0 ("treewide: setup_timer() -> timer_setup()") > > To avoid problems with both -Wcast-function-type (which is enabled in > the kernel via -Wextra is several subsystems), and with mismatched > function prototypes when build with Control Flow Integrity enabled, > this adds the "use_callback" member to let the tasklet caller choose > which union member to call through. Once all old API uses are removed, > this and the .data member will be removed as well. (On 64-bit this does > not grow the struct size as the new member fills the hole after atomic_t, > which is also "int" sized.) > > Signed-off-by: Romain Perier <romain.perier@gmail.com> > Co-developed-by: Allen Pais <allen.lkml@gmail.com> > Signed-off-by: Allen Pais <allen.lkml@gmail.com> > Co-developed-by: Kees Cook <keescook@chromium.org> > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > include/linux/interrupt.h | 24 +++++++++++++++++++++++- > kernel/softirq.c | 18 +++++++++++++++++- > 2 files changed, 40 insertions(+), 2 deletions(-) Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Wed, Jul 15, 2020 at 08:08:47PM -0700, Kees Cook wrote: > +#define DECLARE_TASKLET(name, _callback) \ > +struct tasklet_struct name = { \ > + .count = ATOMIC_INIT(0), \ > + .callback = _callback, \ > + .use_callback = true, \ > +} > + > +#define DECLARE_TASKLET_DISABLED(name, _callback) \ > +struct tasklet_struct name = { \ > + .count = ATOMIC_INIT(1), \ > + .callback = _callback, \ > +} You forgot to set use_callback here. > @@ -547,7 +547,10 @@ static void tasklet_action_common(struct softirq_action *a, > if (!test_and_clear_bit(TASKLET_STATE_SCHED, > &t->state)) > BUG(); > - t->func(t->data); > + if (t->use_callback) > + t->callback(t); > + else > + t->func(t->data); I think this is the wrong way to do the conversion. Start out by setting t->data to (unsigned long)t in the new initialisers. Then convert the drivers (all 350 of them) to the new API. Then you can get rid of 'data' from the tasklet_struct.
On Thu, Jul 16, 2020 at 04:37:04PM +0100, Matthew Wilcox wrote: > On Wed, Jul 15, 2020 at 08:08:47PM -0700, Kees Cook wrote: > > +#define DECLARE_TASKLET(name, _callback) \ > > +struct tasklet_struct name = { \ > > + .count = ATOMIC_INIT(0), \ > > + .callback = _callback, \ > > + .use_callback = true, \ > > +} > > + > > +#define DECLARE_TASKLET_DISABLED(name, _callback) \ > > +struct tasklet_struct name = { \ > > + .count = ATOMIC_INIT(1), \ > > + .callback = _callback, \ > > +} > > You forgot to set use_callback here. Eek; thank you. > > @@ -547,7 +547,10 @@ static void tasklet_action_common(struct softirq_action *a, > > if (!test_and_clear_bit(TASKLET_STATE_SCHED, > > &t->state)) > > BUG(); > > - t->func(t->data); > > + if (t->use_callback) > > + t->callback(t); > > + else > > + t->func(t->data); > > I think this is the wrong way to do the conversion. Start out by setting > t->data to (unsigned long)t in the new initialisers. Then convert the > drivers (all 350 of them) to the new API. Then you can get rid of 'data' > from the tasklet_struct. That's what I did when I converted timer_struct, and it ended up creating a mess for Control Flow Integrity checking. (The problem isn't actually casting .data, but rather in how the callsite calls the callback -- casting the callback assignments doesn't fix the mismatch between the caller and the callback's expectation about the function prototype under CFI.) I got lucky with timer_struct (in v4.14) in that not much had been converted, and I was able to do the entire conversion in the next kernel release. So, this time, I'm trying to avoid the prototype mismatch mess by providing a selector to determine which prototype the callback should be called through, and I was happy to discover I could do it without growing the tasklet structure. Obviously the memory corruption safety improvement won't be realized until both .data, .use_callback, and .func are removed, but that was true even with the earlier style of conversion.
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h index b911196f03eb..15570b237e53 100644 --- a/include/linux/interrupt.h +++ b/include/linux/interrupt.h @@ -608,10 +608,30 @@ struct tasklet_struct struct tasklet_struct *next; unsigned long state; atomic_t count; - void (*func)(unsigned long); + bool use_callback; + union { + void (*func)(unsigned long data); + void (*callback)(struct tasklet_struct *t); + }; unsigned long data; }; +#define DECLARE_TASKLET(name, _callback) \ +struct tasklet_struct name = { \ + .count = ATOMIC_INIT(0), \ + .callback = _callback, \ + .use_callback = true, \ +} + +#define DECLARE_TASKLET_DISABLED(name, _callback) \ +struct tasklet_struct name = { \ + .count = ATOMIC_INIT(1), \ + .callback = _callback, \ +} + +#define from_tasklet(var, callback_tasklet, tasklet_fieldname) \ + container_of(callback_tasklet, typeof(*var), tasklet_fieldname) + #define DECLARE_TASKLET_OLD(name, _func) \ struct tasklet_struct name = { \ .count = ATOMIC_INIT(0), \ @@ -691,6 +711,8 @@ extern void tasklet_kill(struct tasklet_struct *t); extern void tasklet_kill_immediate(struct tasklet_struct *t, unsigned int cpu); extern void tasklet_init(struct tasklet_struct *t, void (*func)(unsigned long), unsigned long data); +extern void tasklet_setup(struct tasklet_struct *t, + void (*callback)(struct tasklet_struct *)); /* * Autoprobing for irqs: diff --git a/kernel/softirq.c b/kernel/softirq.c index c4201b7f42b1..292e7c2d2333 100644 --- a/kernel/softirq.c +++ b/kernel/softirq.c @@ -547,7 +547,10 @@ static void tasklet_action_common(struct softirq_action *a, if (!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state)) BUG(); - t->func(t->data); + if (t->use_callback) + t->callback(t); + else + t->func(t->data); tasklet_unlock(t); continue; } @@ -573,6 +576,18 @@ static __latent_entropy void tasklet_hi_action(struct softirq_action *a) tasklet_action_common(a, this_cpu_ptr(&tasklet_hi_vec), HI_SOFTIRQ); } +void tasklet_setup(struct tasklet_struct *t, + void (*callback)(struct tasklet_struct *)) +{ + t->next = NULL; + t->state = 0; + atomic_set(&t->count, 0); + t->callback = callback; + t->use_callback = true; + t->data = 0; +} +EXPORT_SYMBOL(tasklet_setup); + void tasklet_init(struct tasklet_struct *t, void (*func)(unsigned long), unsigned long data) { @@ -580,6 +595,7 @@ void tasklet_init(struct tasklet_struct *t, t->state = 0; atomic_set(&t->count, 0); t->func = func; + t->use_callback = false; t->data = data; } EXPORT_SYMBOL(tasklet_init);