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.
--
Kees Cook
