[3/3] tasklet: Introduce new initialization API
diff mbox series

Message ID 20200716030847.1564131-4-keescook@chromium.org
State New
Headers show
Series
  • Modernize tasklet callback API
Related show

Commit Message

Kees Cook July 16, 2020, 3:08 a.m. UTC
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(-)

Comments

Greg KH July 16, 2020, 7:30 a.m. UTC | #1
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>
Matthew Wilcox (Oracle) July 16, 2020, 3:37 p.m. UTC | #2
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.
Kees Cook July 16, 2020, 7:22 p.m. UTC | #3
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.

Patch
diff mbox series

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);