[PATCH 0/4] module: add support for unsafe, tainting parameters

[PATCH 0/4] module: add support for unsafe, tainting parameters

[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,

"""
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/1394011994-30604-1-git-send-email-daniel.vetter@ffwll.ch


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

-- 
1.9.1

[PATCH 1/4] module: rename KERNEL_PARAM_FL_NOARG to avoid confusion

[Jani Nikula]

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 <rusty@rustcorp.com.au>
Cc: Jean Delvare <khali@linux-fr.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Li Zhong <zhong@linux.vnet.ibm.com>
Cc: Jon Mason <jon.mason@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 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(-)

diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 494f99e852da..16fdddab856a 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -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 {
diff --git a/kernel/module.c b/kernel/module.c
index ae79ce615cb9..c8f04f9bdba0 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -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,
 };
diff --git a/kernel/params.c b/kernel/params.c
index 34f527023794..8a484fc8bde8 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -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,
 };
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 998100093332..65ca451a764d 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -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
 };
-- 
1.9.1

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

[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 <rusty@rustcorp.com.au>
Cc: Jean Delvare <khali@linux-fr.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Li Zhong <zhong@linux.vnet.ibm.com>
Cc: Jon Mason <jon.mason@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/tty/serial/8250/8250_core.c |  2 +-
 include/linux/moduleparam.h         | 44 +++++++++++++++++++++++++++++--------
 kernel/params.c                     | 11 ++++++++++
 3 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 1d42dba6121d..bd672948f2f1 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -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
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 16fdddab856a..1e3ffb839daa 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -56,11 +56,21 @@ struct kernel_param_ops {
 	void (*free)(void *arg);
 };
 
+/*
+ * Flags available for kernel_param
+ *
+ * UNSAFE - the parameter is dangerous and setting it will taint the kernel
+ */
+enum {
+	KERNEL_PARAM_FL_UNSAFE = (1 << 0)
+};
+
 struct kernel_param {
 	const char *name;
 	const struct kernel_param_ops *ops;
 	u16 perm;
-	s16 level;
+	s8 level;
+	u8 flags;
 	union {
 		void *arg;
 		const struct kparam_string *str;
@@ -137,7 +147,7 @@ struct kparam_array
  * The ops can have NULL set or get functions.
  */
 #define module_param_cb(name, ops, arg, perm)				      \
-	__module_param_call(MODULE_PARAM_PREFIX, name, ops, arg, perm, -1)
+	__module_param_call(MODULE_PARAM_PREFIX, name, ops, arg, perm, -1, 0)
 
 /**
  * <level>_param_cb - general callback for a module/cmdline parameter
@@ -149,7 +159,7 @@ struct kparam_array
  * The ops can have NULL set or get functions.
  */
 #define __level_param_cb(name, ops, arg, perm, level)			\
-	__module_param_call(MODULE_PARAM_PREFIX, name, ops, arg, perm, level)
+	__module_param_call(MODULE_PARAM_PREFIX, name, ops, arg, perm, level, 0)
 
 #define core_param_cb(name, ops, arg, perm)		\
 	__level_param_cb(name, ops, arg, perm, 1)
@@ -184,14 +194,14 @@ struct kparam_array
 
 /* This is the fundamental function for registering boot/module
    parameters. */
-#define __module_param_call(prefix, name, ops, arg, perm, level)	\
+#define __module_param_call(prefix, name, ops, arg, perm, level, flags)	\
 	/* Default value instead of permissions? */			\
 	static const char __param_str_##name[] = prefix #name; \
 	static struct kernel_param __moduleparam_const __param_##name	\
 	__used								\
     __attribute__ ((unused,__section__ ("__param"),aligned(sizeof(void *)))) \
 	= { __param_str_##name, ops, VERIFY_OCTAL_PERMISSIONS(perm),	\
-	    level, { arg } }
+	    level, flags, { arg } }
 
 /* Obsolete - use module_param_cb() */
 #define module_param_call(name, set, get, arg, perm)			\
@@ -199,7 +209,7 @@ struct kparam_array
 		{ 0, (void *)set, (void *)get };			\
 	__module_param_call(MODULE_PARAM_PREFIX,			\
 			    name, &__param_ops_##name, arg,		\
-			    (perm) + sizeof(__check_old_set_param(set))*0, -1)
+			    (perm) + sizeof(__check_old_set_param(set))*0, -1, 0)
 
 /* We don't get oldget: it's often a new-style param_get_uint, etc. */
 static inline int
@@ -279,7 +289,7 @@ static inline void __kernel_param_unlock(void)
  */
 #define core_param(name, var, type, perm)				\
 	param_check_##type(name, &(var));				\
-	__module_param_call("", name, &param_ops_##type, &var, perm, -1)
+	__module_param_call("", name, &param_ops_##type, &var, perm, -1, 0)
 #endif /* !MODULE */
 
 /**
@@ -297,7 +307,7 @@ static inline void __kernel_param_unlock(void)
 		= { len, string };					\
 	__module_param_call(MODULE_PARAM_PREFIX, name,			\
 			    &param_ops_string,				\
-			    .str = &__param_string_##name, perm, -1);	\
+			    .str = &__param_string_##name, perm, -1, 0);\
 	__MODULE_PARM_TYPE(name, "string")
 
 /**
@@ -346,6 +356,22 @@ 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);
@@ -444,7 +470,7 @@ extern int param_set_bint(const char *val, const struct kernel_param *kp);
 	__module_param_call(MODULE_PARAM_PREFIX, name,			\
 			    &param_array_ops,				\
 			    .arr = &__param_arr_##name,			\
-			    perm, -1);					\
+			    perm, -1, 0);				\
 	__MODULE_PARM_TYPE(name, "array of " #type)
 
 extern struct kernel_param_ops param_array_ops;
diff --git a/kernel/params.c b/kernel/params.c
index 8a484fc8bde8..ad8d04563c3a 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -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);
-- 
1.9.1

[PATCH 3/4] module: add module_param_unsafe and module_param_named_unsafe

[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 <rusty@rustcorp.com.au>
Cc: Jean Delvare <khali@linux-fr.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Li Zhong <zhong@linux.vnet.ibm.com>
Cc: Jon Mason <jon.mason@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 include/linux/moduleparam.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 1e3ffb839daa..9531f9f9729e 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -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
-- 
1.9.1

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

[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 <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_params.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 62ee8308d682..bba4225ba575 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -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)");
-- 
1.9.1

Re: [PATCH 0/4] module: add support for unsafe, tainting parameters

[Rusty Russell]

Jani Nikula <jani.nikula@intel.com> writes:
> 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 <daniel.vetter@ffwll.ch>
Based-on-patches-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 494f99e852da..99ba68206ba4 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -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
diff --git a/kernel/params.c b/kernel/params.c
index 34f527023794..3128218158cf 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -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,

Re: [PATCH 0/4] module: add support for unsafe, tainting parameters

[Daniel Vetter]

On Wed, Aug 13, 2014 at 10:25 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Jani Nikula <jani.nikula@intel.com> writes:
>> 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/87r46gywul.fsf@rustcorp.com.au

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

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

Re: [PATCH 0/4] module: add support for unsafe, tainting parameters

[Rusty Russell]

Daniel Vetter <daniel.vetter@ffwll.ch> writes:
> On Wed, Aug 13, 2014 at 10:25 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Jani Nikula <jani.nikula@intel.com> writes:
>>> 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.

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 <rusty@rustcorp.com.au>

diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 9531f9f9729e..593501996574 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -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);
diff --git a/kernel/params.c b/kernel/params.c
index ad8d04563c3a..f3cc977d6a66 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -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)

Re: [PATCH 0/4] module: add support for unsafe, tainting parameters

[Jani Nikula]

On Wed, 20 Aug 2014, Rusty Russell <rusty@rustcorp.com.au> wrote:
> 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.


-- 
Jani Nikula, Intel Open Source Technology Center