module: add support for unsafe, tainting parameters

PATCH Applied Jani Nikula 3 participants 9 messages Aug 11, 2014 – Aug 21, 2014

Subsystems (6)

DRM drivers Intel i915 TTY layer and serial drivers Security (LSM) +2 more subsystems

lore

Share & tools

lore

Jani Nikula

author

2014-08-11 13:54 UTC #0 lore raw

cover module: add support for unsafe, tainting parameters

This is a generic version of Daniel's patch [1] letting us have unsafe module parameters (experimental, debugging, testing, etc.) that taint the kernel when set. Quoting Daniel,

"""
Users just love to set random piles of options since surely enabling all the experimental stuff helps. Later on we get bug reports because it all fell apart.

Even more fun when it's labelled a regression when some change only just made the feature possible (e.g. stolen memory fixes suddenly making fbc possible).

Make it clear that users are playing with fire here. """

Patches 1-3 add the core functionality, patch 4 is our use case as an example.

BR,
Jani.

[1] http://lkml.kernel.org/r/[email protected]

Jani Nikula (4):
module: rename KERNEL_PARAM_FL_NOARG to avoid confusion
module: make it possible to have unsafe, tainting module params
module: add module_param_unsafe and module_param_named_unsafe drm/i915: taint the kernel if unsafe module parameters are set

 drivers/gpu/drm/i915/i915_params.c  |  8 ++---
 drivers/tty/serial/8250/8250_core.c |  2 +-
 include/linux/moduleparam.h         | 64 +++++++++++++++++++++++++++++++------
 kernel/module.c                     |  2 +-
 kernel/params.c                     | 17 ++++++++--
 security/apparmor/lsm.c             |  4 +--
 6 files changed, 76 insertions(+), 21 deletions(-)

Signature · 1 line

1.9.1

Jani Nikula

author

2014-08-11 13:54 UTC #1 lore raw

PATCH 1/4 module: rename KERNEL_PARAM_FL_NOARG to avoid confusion

Make it clear this is about kernel_param_ops, not kernel_param (which will soon have a flags field of its own). No functional changes.

Cc Rusty Russell <[email protected]>

Cc Jean Delvare <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Li Zhong <[email protected]>

Cc Jon Mason <[email protected]>

Cc Daniel Vetter <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 include/linux/moduleparam.h | 2 +-
 kernel/module.c             | 2 +-
 kernel/params.c             | 6 +++---
 security/apparmor/lsm.c     | 4 ++--
 4 files changed, 7 insertions(+), 7 deletions(-)

include/linux/moduleparam.h +1 -1

index 494f99e852da..16fdddab856a 100644
@@ -42,7 +42,7 @@ struct kernel_param;
  * NOARG - the parameter allows for no argument (foo instead of foo=1)
  */
 enum {
-	KERNEL_PARAM_FL_NOARG = (1 << 0)
+	KERNEL_PARAM_OPS_FL_NOARG = (1 << 0)
 };
 
 struct kernel_param_ops {

kernel/module.c +1 -1

index ae79ce615cb9..c8f04f9bdba0 100644
@@ -135,7 +135,7 @@ static int param_set_bool_enable_only(const char *val,
 }
 
 static const struct kernel_param_ops param_ops_bool_enable_only = {
-	.flags = KERNEL_PARAM_FL_NOARG,
+	.flags = KERNEL_PARAM_OPS_FL_NOARG,
 	.set = param_set_bool_enable_only,
 	.get = param_get_bool,
 };

kernel/params.c +3 -3

index 34f527023794..8a484fc8bde8 100644
@@ -104,7 +104,7 @@ static int parse_one(char *param,
 				return 0;
 			/* No one handled NULL, so do it here. */
 			if (!val &&
-			    !(params[i].ops->flags & KERNEL_PARAM_FL_NOARG))
+			    !(params[i].ops->flags & KERNEL_PARAM_OPS_FL_NOARG))
 				return -EINVAL;
 			pr_debug("handling %s with %p\n", param,
 				params[i].ops->set);

@@ -318,7 +318,7 @@ int param_get_bool(char *buffer, const struct kernel_param *kp)
 EXPORT_SYMBOL(param_get_bool);
 
 struct kernel_param_ops param_ops_bool = {
-	.flags = KERNEL_PARAM_FL_NOARG,
+	.flags = KERNEL_PARAM_OPS_FL_NOARG,
 	.set = param_set_bool,
 	.get = param_get_bool,
 };

@@ -369,7 +369,7 @@ int param_set_bint(const char *val, const struct kernel_param *kp)
 EXPORT_SYMBOL(param_set_bint);
 
 struct kernel_param_ops param_ops_bint = {
-	.flags = KERNEL_PARAM_FL_NOARG,
+	.flags = KERNEL_PARAM_OPS_FL_NOARG,
 	.set = param_set_bint,
 	.get = param_get_int,
 };

security/apparmor/lsm.c +2 -2

index 998100093332..65ca451a764d 100644
@@ -668,7 +668,7 @@ static int param_set_aabool(const char *val, const struct kernel_param *kp);
 static int param_get_aabool(char *buffer, const struct kernel_param *kp);
 #define param_check_aabool param_check_bool
 static struct kernel_param_ops param_ops_aabool = {
-	.flags = KERNEL_PARAM_FL_NOARG,
+	.flags = KERNEL_PARAM_OPS_FL_NOARG,
 	.set = param_set_aabool,
 	.get = param_get_aabool
 };

@@ -685,7 +685,7 @@ static int param_set_aalockpolicy(const char *val, const struct kernel_param *kp
 static int param_get_aalockpolicy(char *buffer, const struct kernel_param *kp);
 #define param_check_aalockpolicy param_check_bool
 static struct kernel_param_ops param_ops_aalockpolicy = {
-	.flags = KERNEL_PARAM_FL_NOARG,
+	.flags = KERNEL_PARAM_OPS_FL_NOARG,
 	.set = param_set_aalockpolicy,
 	.get = param_get_aalockpolicy
 };

Signature · 1 line

1.9.1

Jani Nikula

author

2014-08-11 13:54 UTC #2 lore raw

PATCH 2/4 module: make it possible to have unsafe, tainting module params

In reply to Jani Nikula

Add flags field to struct kernel_params, and add the first flag: unsafe parameter. Modifying a kernel parameter with the unsafe flag set, either via the kernel command line or sysfs, will issue a warning and taint the kernel.

Cc Rusty Russell <[email protected]>

Cc Jean Delvare <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Li Zhong <[email protected]>

Cc Jon Mason <[email protected]>

Cc Daniel Vetter <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/tty/serial/8250/8250_core.c |  2 +-
 include/linux/moduleparam.h         | 44 +++++++++++++++++++++++++++++--------
 kernel/params.c                     | 11 ++++++++++
 3 files changed, 47 insertions(+), 10 deletions(-)

drivers/tty/serial/8250/8250_core.c +1 -1

index 1d42dba6121d..bd672948f2f1 100644
@@ -3587,7 +3587,7 @@ static void __used s8250_options(void)
 #ifdef CONFIG_SERIAL_8250_RSA
 	__module_param_call(MODULE_PARAM_PREFIX, probe_rsa,
 		&param_array_ops, .arr = &__param_arr_probe_rsa,
-		0444, -1);
+		0444, -1, 0);
 #endif
 }
 #else

include/linux/moduleparam.h +35 -9

108 lines not on this page · raw message

kernel/params.c +11 -0

index 8a484fc8bde8..ad8d04563c3a 100644
@@ -233,6 +233,7 @@ char *parse_args(const char *doing,
 #define STANDARD_PARAM_DEF(name, type, format, strtolfn)      		\
 	int param_set_##name(const char *val, const struct kernel_param *kp) \
 	{								\
+		param_check_unsafe(kp);					\
 		return strtolfn(val, 0, (type *)kp->arg);		\
 	}								\
 	int param_get_##name(char *buffer, const struct kernel_param *kp) \

@@ -265,6 +266,8 @@ int param_set_charp(const char *val, const struct kernel_param *kp)
 		return -ENOSPC;
 	}
 
+	param_check_unsafe(kp);
+
 	maybe_kfree_parameter(*(char **)kp->arg);
 
 	/* This is a hack.  We can't kmalloc in early boot, and we

@@ -302,6 +305,8 @@ EXPORT_SYMBOL(param_ops_charp);
 /* Actually could be a bool or an int, for historical reasons. */
 int param_set_bool(const char *val, const struct kernel_param *kp)
 {
+	param_check_unsafe(kp);
+
 	/* No equals means "set"... */
 	if (!val) val = "1";

@@ -331,6 +336,8 @@ int param_set_invbool(const char *val, const struct kernel_param *kp)
 	bool boolval;
 	struct kernel_param dummy;
 
+	param_check_unsafe(kp);
+
 	dummy.arg = &boolval;
 	ret = param_set_bool(val, &dummy);
 	if (ret == 0)

@@ -357,6 +364,8 @@ int param_set_bint(const char *val, const struct kernel_param *kp)
 	bool v;
 	int ret;
 
+	param_check_unsafe(kp);
+
 	/* Match bool exactly, by re-using it. */
 	boolkp = *kp;
 	boolkp.arg = &v;

@@ -476,6 +485,8 @@ int param_set_copystring(const char *val, const struct kernel_param *kp)
 {
 	const struct kparam_string *kps = kp->str;
 
+	param_check_unsafe(kp);
+
 	if (strlen(val)+1 > kps->maxlen) {
 		pr_err("%s: string doesn't fit in %u chars.\n",
 		       kp->name, kps->maxlen-1);

Signature · 1 line

1.9.1

Jani Nikula

author

2014-08-11 13:57 UTC #3 lore raw

PATCH 3/4 module: add module_param_unsafe and module_param_named_unsafe

In reply to Jani Nikula

Add the helpers to be used by modules wishing to expose unsafe debugging or testing module parameters that taint the kernel when set.

Cc Rusty Russell <[email protected]>

Cc Jean Delvare <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Li Zhong <[email protected]>

Cc Jon Mason <[email protected]>

Cc Daniel Vetter <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 include/linux/moduleparam.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

include/linux/moduleparam.h +18 -0

index 1e3ffb839daa..9531f9f9729e 100644
@@ -123,6 +123,12 @@ struct kparam_array
 	module_param_named(name, name, type, perm)
 
 /**
+ * module_param_unsafe - same as module_param but taints kernel
+ */
+#define module_param_unsafe(name, type, perm)			\
+	module_param_named_unsafe(name, name, type, perm)
+
+/**
  * module_param_named - typesafe helper for a renamed module/cmdline parameter
  * @name: a valid C identifier which is the parameter name.
  * @value: the actual lvalue to alter.

@@ -139,6 +145,14 @@ struct kparam_array
 	__MODULE_PARM_TYPE(name, #type)
 
 /**
+ * module_param_named_unsafe - same as module_param_named but taints kernel
+ */
+#define module_param_named_unsafe(name, value, type, perm)		\
+	param_check_##type(name, &(value));				\
+	module_param_cb_unsafe(name, &param_ops_##type, &value, perm);	\
+	__MODULE_PARM_TYPE(name, #type)
+
+/**
  * module_param_cb - general callback for a module/cmdline parameter
  * @name: a valid C identifier which is the parameter name.
  * @ops: the set & get operations for this parameter.

@@ -149,6 +163,10 @@ struct kparam_array
 #define module_param_cb(name, ops, arg, perm)				      \
 	__module_param_call(MODULE_PARAM_PREFIX, name, ops, arg, perm, -1, 0)
 
+#define module_param_cb_unsafe(name, ops, arg, perm)			      \
+	__module_param_call(MODULE_PARAM_PREFIX, name, ops, arg, perm, -1,    \
+			    KERNEL_PARAM_FL_UNSAFE)
+
 /**
  * <level>_param_cb - general callback for a module/cmdline parameter
  *                    to be evaluated before certain initcall level

Signature · 1 line

1.9.1

Jani Nikula

author

2014-08-11 13:57 UTC #4 lore raw

PATCH 4/4 drm/i915: taint the kernel if unsafe module parameters are set

In reply to Jani Nikula

Taint the kernel if the semaphores, enable_rc6, enable_fbc, or ppgtt module parameters are modified. These module parameters are for debugging and testing only, and should never be changed from their platform specific default values by the users. We do not provide support for people enabling all the experimental features. Make this clear by tainting the kernel if the parameters are set.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_params.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/i915_params.c +4 -4

index 62ee8308d682..bba4225ba575 100644
@@ -66,12 +66,12 @@ module_param_named(powersave, i915.powersave, int, 0600);
 MODULE_PARM_DESC(powersave,
 	"Enable powersavings, fbc, downclocking, etc. (default: true)");
 
-module_param_named(semaphores, i915.semaphores, int, 0400);
+module_param_named_unsafe(semaphores, i915.semaphores, int, 0400);
 MODULE_PARM_DESC(semaphores,
 	"Use semaphores for inter-ring sync "
 	"(default: -1 (use per-chip defaults))");
 
-module_param_named(enable_rc6, i915.enable_rc6, int, 0400);
+module_param_named_unsafe(enable_rc6, i915.enable_rc6, int, 0400);
 MODULE_PARM_DESC(enable_rc6,
 	"Enable power-saving render C-state 6. "
 	"Different stages can be selected via bitmask values "

@@ -79,7 +79,7 @@ MODULE_PARM_DESC(enable_rc6,
 	"For example, 3 would enable rc6 and deep rc6, and 7 would enable everything. "
 	"default: -1 (use per-chip default)");
 
-module_param_named(enable_fbc, i915.enable_fbc, int, 0600);
+module_param_named_unsafe(enable_fbc, i915.enable_fbc, int, 0600);
 MODULE_PARM_DESC(enable_fbc,
 	"Enable frame buffer compression for power savings "
 	"(default: -1 (use per-chip default))");

@@ -113,7 +113,7 @@ MODULE_PARM_DESC(enable_hangcheck,
 	"WARNING: Disabling this can cause system wide hangs. "
 	"(default: true)");
 
-module_param_named(enable_ppgtt, i915.enable_ppgtt, int, 0400);
+module_param_named_unsafe(enable_ppgtt, i915.enable_ppgtt, int, 0400);
 MODULE_PARM_DESC(enable_ppgtt,
 	"Override PPGTT usage. "
 	"(-1=auto [default], 0=disabled, 1=aliasing, 2=full)");

Signature · 1 line

1.9.1

Rusty Russell

2014-08-13 20:25 UTC #5 lore raw

In reply to Jani Nikula

Quoted 3 lines · Jani Nikula

This is a generic version of Daniel's patch [1] letting us have unsafe module parameters (experimental, debugging, testing, etc.) that taint the kernel when set. Quoting Daniel,

OK, I think the idea is fine, but we'll probably only want this for a few types (eg. int and bool). So for the moment I prefer a more naive approach.

Does this work for you?

Subject: module: add taint_int type

An int parameter which taints the kernel if set; i915 at least wants this.

Based-on-patches-by Daniel Vetter <[email protected]>

Based-on-patches-by Jani Nikula <[email protected]>

Signed-off-by Rusty Russell <[email protected]>

include/linux/moduleparam.h +4 -0

index 494f99e852da..99ba68206ba4 100644
@@ -408,6 +408,10 @@ extern int param_set_bint(const char *val, const struct kernel_param *kp);
 #define param_get_bint param_get_int
 #define param_check_bint param_check_int
 
+/* An int, which taints the kernel if set. */
+extern struct kernel_param_ops param_ops_taint_int;
+#define param_check_taint_int param_check_int
+
 /**
  * module_param_array - a parameter which is an array of some type
  * @name: the name of the array variable

kernel/params.c +14 -0

index 34f527023794..3128218158cf 100644
@@ -375,6 +375,20 @@ struct kernel_param_ops param_ops_bint = {
 };
 EXPORT_SYMBOL(param_ops_bint);
 
+static int param_set_taint_int(const char *val, const struct kernel_param *kp)
+{
+	pr_warn("Setting dangerous option %s - tainting kernel\n", kp->name);
+	add_taint(TAINT_USER, LOCKDEP_STILL_OK);
+
+	return param_set_int(val, kp);
+}
+
+struct kernel_param_ops param_ops_taint_int = {
+	.set = param_set_taint_int,
+	.get = param_get_int,
+};
+EXPORT_SYMBOL(param_ops_taint_int);
+
 /* We break the rule and mangle the string. */
 static int param_array(const char *name,
 		       const char *val,

Daniel Vetter

2014-08-14 05:21 UTC #6 lore raw

Quoted 10 lines · Rusty Russell

Quoted 3 lines · Jani Nikula
This is a generic version of Daniel's patch [1] letting us have unsafe module parameters (experimental, debugging, testing, etc.) that taint the kernel when set. Quoting Daniel,
OK, I think the idea is fine, but we'll probably only want this for a few types (eg. int and bool). So for the moment I prefer a more naive approach.
Does this work for you?

Can you please discuss this with yourself from a few months back? We've done the general version since you suggested that just doing it for int is a bit lame ;-) And I actually agreed so asked Jani to look into that.

http://mid.mail-archive.com/[email protected]

"If this is a good idea, you can write a macro module_param_unsafe_named which is a general wrapper."

Cheers, Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Rusty Russell

Applied

2014-08-20 19:47 UTC #7 lore raw

Quoted 10 lines · Daniel Vetter

Quoted 9 lines · Rusty Russell
Quoted 3 lines · Jani Nikula
This is a generic version of Daniel's patch [1] letting us have unsafe module parameters (experimental, debugging, testing, etc.) that taint the kernel when set. Quoting Daniel,
OK, I think the idea is fine, but we'll probably only want this for a few types (eg. int and bool). So for the moment I prefer a more naive approach.

Quoted 6 lines

Quoted 1 line
Does this work for you?
Can you please discuss this with yourself from a few months back? We've done the general version since you suggested that just doing it for int is a bit lame ;-) And I actually agreed so asked Jani to look into that.

Don't listen to me, I'm an idiot!

Applied.

I've applied this cleanup on top, however.

Cheers,
Rusty.

Subject: param: check for tainting before calling set op.

This means every set op doesn't need to call it, and it can move into params.c.

Signed-off-by Rusty Russell <[email protected]>

include/linux/moduleparam.h +0 ~~-16~~

index 9531f9f9729e..593501996574 100644
@@ -374,22 +374,6 @@ static inline void destroy_params(const struct kernel_param *params,
 #define __param_check(name, p, type) \
 	static inline type __always_unused *__check_##name(void) { return(p); }
 
-/**
- * param_check_unsafe - Warn and taint the kernel if setting dangerous options.
- *
- * This gets called from all the standard param setters, but can be used from
- * custom setters as well.
- */
-static inline void
-param_check_unsafe(const struct kernel_param *kp)
-{
-	if (kp->flags & KERNEL_PARAM_FL_UNSAFE) {
-		pr_warn("Setting dangerous option %s - tainting kernel\n",
-			kp->name);
-		add_taint(TAINT_USER, LOCKDEP_STILL_OK);
-	}
-}
-
 extern struct kernel_param_ops param_ops_byte;
 extern int param_set_byte(const char *val, const struct kernel_param *kp);
 extern int param_get_byte(char *buffer, const struct kernel_param *kp);

kernel/params.c +11 ~~-11~~

index ad8d04563c3a..f3cc977d6a66 100644
@@ -83,6 +83,15 @@ bool parameq(const char *a, const char *b)
 	return parameqn(a, b, strlen(a)+1);
 }
 
+static void param_check_unsafe(const struct kernel_param *kp)
+{
+	if (kp->flags & KERNEL_PARAM_FL_UNSAFE) {
+		pr_warn("Setting dangerous option %s - tainting kernel\n",
+			kp->name);
+		add_taint(TAINT_USER, LOCKDEP_STILL_OK);
+	}
+}
+
 static int parse_one(char *param,
 		     char *val,
 		     const char *doing,

@@ -109,6 +119,7 @@ static int parse_one(char *param,
 			pr_debug("handling %s with %p\n", param,
 				params[i].ops->set);
 			mutex_lock(&param_lock);
+			param_check_unsafe(&params[i]);
 			err = params[i].ops->set(val, &params[i]);
 			mutex_unlock(&param_lock);
 			return err;

@@ -233,7 +244,6 @@ char *parse_args(const char *doing,
 #define STANDARD_PARAM_DEF(name, type, format, strtolfn)      		\
 	int param_set_##name(const char *val, const struct kernel_param *kp) \
 	{								\
-		param_check_unsafe(kp);					\
 		return strtolfn(val, 0, (type *)kp->arg);		\
 	}								\
 	int param_get_##name(char *buffer, const struct kernel_param *kp) \

@@ -266,8 +276,6 @@ int param_set_charp(const char *val, const struct kernel_param *kp)
 		return -ENOSPC;
 	}
 
-	param_check_unsafe(kp);
-
 	maybe_kfree_parameter(*(char **)kp->arg);
 
 	/* This is a hack.  We can't kmalloc in early boot, and we

@@ -305,8 +313,6 @@ EXPORT_SYMBOL(param_ops_charp);
 /* Actually could be a bool or an int, for historical reasons. */
 int param_set_bool(const char *val, const struct kernel_param *kp)
 {
-	param_check_unsafe(kp);
-
 	/* No equals means "set"... */
 	if (!val) val = "1";

@@ -336,8 +342,6 @@ int param_set_invbool(const char *val, const struct kernel_param *kp)
 	bool boolval;
 	struct kernel_param dummy;
 
-	param_check_unsafe(kp);
-
 	dummy.arg = &boolval;
 	ret = param_set_bool(val, &dummy);
 	if (ret == 0)

@@ -364,8 +368,6 @@ int param_set_bint(const char *val, const struct kernel_param *kp)
 	bool v;
 	int ret;
 
-	param_check_unsafe(kp);
-
 	/* Match bool exactly, by re-using it. */
 	boolkp = *kp;
 	boolkp.arg = &v;

@@ -485,8 +487,6 @@ int param_set_copystring(const char *val, const struct kernel_param *kp)
 {
 	const struct kparam_string *kps = kp->str;
 
-	param_check_unsafe(kp);
-
 	if (strlen(val)+1 > kps->maxlen) {
 		pr_err("%s: string doesn't fit in %u chars.\n",
 		       kp->name, kps->maxlen-1);

@@ -563,6 +563,7 @@ static ssize_t param_attr_store(struct module_attribute *mattr,
 		return -EPERM;
 
 	mutex_lock(&param_lock);
+	param_check_unsafe(attribute->param);
 	err = attribute->param->ops->set(buf, attribute->param);
 	mutex_unlock(&param_lock);
 	if (!err)

Jani Nikula

author

2014-08-21 07:01 UTC #8 lore raw

Quoted 9 lines · Rusty Russell

I've applied this cleanup on top, however.
Cheers,
Rusty.
Subject: param: check for tainting before calling set op.
This means every set op doesn't need to call it, and it can move into params.c.

Much better, thanks. I was looking for a way to do something like this, but obviously didn't look hard enough.

BR,
Jani.

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center