[PATCH] [RFC] Taint the kernel for unsafe module options

[PATCH] [RFC] Taint the kernel for unsafe module options

[Daniel Vetter]

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. In drm/i915 all
these options follow the same pattern of using -1 as the per-machine
default, and any other value being used for force the parameter.

Adding a pile of cc's to solicit input and figure out whether this
would be generally useful - this quick rfc is just for drm/i915.

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: linux-kernel@vger.kernel.org
References: https://bugzilla.kernel.org/show_bug.cgi?id=71391
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_params.c | 49 ++++++++++++++++++++++++++++++++++----
 1 file changed, 44 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 3b482585c5ae..9cb89adeabcd 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -50,7 +50,46 @@ struct i915_params i915 __read_mostly = {
 	.disable_display = 0,
 };
 
-module_param_named(modeset, i915.modeset, int, 0400);
+int param_set_unsafe_int(const char *val, const struct kernel_param *kp)
+{
+	long l;
+	int ret;
+
+	ret = kstrtol(val, 0, &l);
+	if (ret < 0 || ((int)l != l))
+		return ret < 0 ? ret : -EINVAL;
+
+	/* Taint if userspace overrides the kernel defaults. */
+	if (l != -1) {
+		printk(KERN_WARNING "Setting dangerous option %s to non-default value!\n",
+		       kp->name);
+		add_taint(TAINT_USER, LOCKDEP_STILL_OK);
+	}
+
+	*((int *)kp->arg) = l;
+	return 0;
+}
+
+struct kernel_param_ops param_ops_unsafe_int = {
+	.set = param_set_unsafe_int,
+	.get = param_get_int,
+};
+/**
+ * module_param_unsafe_int_named - integer module param which taints the kernel
+ * @name: a valid C identifier which is the parameter name.
+ * @value: the actual lvalue to alter.
+ * @perm: visibility in sysfs.
+ *
+ * This is a special integer module parameter. The default is assumed to be -1,
+ * if any other values are set this will taint the kernel. Useful for debug and
+ * other dangerous options which users really shouldn't set normally.
+ */
+#define module_param_unsafe_int_named(name, value, perm)		\
+	param_check_int(name, &(value));				\
+	module_param_cb(name, &param_ops_unsafe_int, &value, perm);	\
+	__MODULE_PARM_TYPE(name, "int")
+
+module_param_unsafe_int_named(modeset, i915.modeset, 0400);
 MODULE_PARM_DESC(modeset,
 	"Use kernel modesetting [KMS] (0=DRM_I915_KMS from .config, "
 	"1=on, -1=force vga console preference [default])");
@@ -64,12 +103,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_unsafe_int_named(semaphores, i915.semaphores, 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_unsafe_int_named(enable_rc6, i915.enable_rc6, 0400);
 MODULE_PARM_DESC(enable_rc6,
 	"Enable power-saving render C-state 6. "
 	"Different stages can be selected via bitmask values "
@@ -77,7 +116,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_unsafe_int_named(enable_fbc, i915.enable_fbc, 0600);
 MODULE_PARM_DESC(enable_fbc,
 	"Enable frame buffer compression for power savings "
 	"(default: -1 (use per-chip default))");
@@ -111,7 +150,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_unsafe_int_named(enable_ppgtt, i915.enable_ppgtt, 0400);
 MODULE_PARM_DESC(enable_ppgtt,
 	"Override PPGTT usage. "
 	"(-1=auto [default], 0=disabled, 1=aliasing, 2=full)");
-- 
1.8.5.2

Re: [PATCH] [RFC] Taint the kernel for unsafe module options

[Andrew Morton]

On Wed,  5 Mar 2014 10:33:14 +0100 Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> 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. In drm/i915 all
> these options follow the same pattern of using -1 as the per-machine
> default, and any other value being used for force the parameter.
> 
> Adding a pile of cc's to solicit input and figure out whether this
> would be generally useful - this quick rfc is just for drm/i915.

Seems harmless and potentially useful to others so yes, I'd vote for
putting it in core kernel.

However this only handles integers.  Will we end up needed great gobs
of new code to detect unsafe setting of u8's, strings, etc?


> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c

The patch adds lots of trailing whitespace.  checkpatch is ->thattaway.

> @@ -50,7 +50,46 @@ struct i915_params i915 __read_mostly = {
>  	.disable_display = 0,
>  };
>  
> -module_param_named(modeset, i915.modeset, int, 0400);
> +int param_set_unsafe_int(const char *val, const struct kernel_param *kp)
> +{
> +	long l;
> +	int ret;
> +
> +	ret = kstrtol(val, 0, &l);
> +	if (ret < 0 || ((int)l != l))
> +		return ret < 0 ? ret : -EINVAL;

That's a bit screwy.  Simpler:

	if (ret < 0)
		return ret;
	if ((int)l != l)
		return -EINVAL;


> +	/* Taint if userspace overrides the kernel defaults. */
> +	if (l != -1) {
> +		printk(KERN_WARNING "Setting dangerous option %s to non-default value!\n",
> +		       kp->name);

pr_warn() is nicer.

> +		add_taint(TAINT_USER, LOCKDEP_STILL_OK);
> +	}
> +
> +	*((int *)kp->arg) = l;
> +	return 0;
> +}
>
> ...
>

Re: [PATCH] [RFC] Taint the kernel for unsafe module options

[Daniel Vetter]

On Wed, Mar 5, 2014 at 9:32 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed,  5 Mar 2014 10:33:14 +0100 Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
>> 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. In drm/i915 all
>> these options follow the same pattern of using -1 as the per-machine
>> default, and any other value being used for force the parameter.
>>
>> Adding a pile of cc's to solicit input and figure out whether this
>> would be generally useful - this quick rfc is just for drm/i915.
>
> Seems harmless and potentially useful to others so yes, I'd vote for
> putting it in core kernel.
>
> However this only handles integers.  Will we end up needed great gobs
> of new code to detect unsafe setting of u8's, strings, etc?

Well I've just done integers because hardcoding the -1 default was so
easy ... But thinking about it some more (and looking at some more mod
params in i915) passing the default to the macro and storing it in
some struct, together with the pointer for the variable sounds useful.
With that this could be easily extended to all kinds of types.

Now would such a temporary structure to store the default be
acceptable or is there some neater trick to pull this off?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] [RFC] Taint the kernel for unsafe module options

[Daniel Vetter]

On Wed, Mar 5, 2014 at 10:00 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Wed, Mar 5, 2014 at 9:32 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
>> On Wed,  5 Mar 2014 10:33:14 +0100 Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>
>>> 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. In drm/i915 all
>>> these options follow the same pattern of using -1 as the per-machine
>>> default, and any other value being used for force the parameter.
>>>
>>> Adding a pile of cc's to solicit input and figure out whether this
>>> would be generally useful - this quick rfc is just for drm/i915.
>>
>> Seems harmless and potentially useful to others so yes, I'd vote for
>> putting it in core kernel.
>>
>> However this only handles integers.  Will we end up needed great gobs
>> of new code to detect unsafe setting of u8's, strings, etc?
>
> Well I've just done integers because hardcoding the -1 default was so
> easy ... But thinking about it some more (and looking at some more mod
> params in i915) passing the default to the macro and storing it in
> some struct, together with the pointer for the variable sounds useful.
> With that this could be easily extended to all kinds of types.
>
> Now would such a temporary structure to store the default be
> acceptable or is there some neater trick to pull this off?

s/temporary/created by the macro/ ... past beer time here already ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] [RFC] Taint the kernel for unsafe module options

[Rusty Russell]

Daniel Vetter <daniel.vetter@ffwll.ch> writes:
> 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. In drm/i915 all
> these options follow the same pattern of using -1 as the per-machine
> default, and any other value being used for force the parameter.
>
> Adding a pile of cc's to solicit input and figure out whether this
> would be generally useful - this quick rfc is just for drm/i915.

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

> -module_param_named(modeset, i915.modeset, int, 0400);

Wait, WTF?  Why do you prefix i915 here manually?  That means that
the commandline parameter would be "i915.i915.modeset=" and the
module parameter would be "i915.modeset="...

Confused,
Rusty.

Re: [PATCH] [RFC] Taint the kernel for unsafe module options

[Daniel Vetter]

On Thu, Mar 06, 2014 at 11:19:54AM +1030, Rusty Russell wrote:
> Daniel Vetter <daniel.vetter@ffwll.ch> writes:
> > 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. In drm/i915 all
> > these options follow the same pattern of using -1 as the per-machine
> > default, and any other value being used for force the parameter.
> >
> > Adding a pile of cc's to solicit input and figure out whether this
> > would be generally useful - this quick rfc is just for drm/i915.
> 
> If this is a good idea, you can write a macro module_param_unsafe_named
> which is a general wrapper.

For this to work I need to somehow store the safe default value somewhere.
since with bools or strings there really isn't such a thing, even less
than with integers where my fairly abitrary -1 choice is already
restricting. But I don't have a good idea how to do that, since creating a
local static struct in the macro to store the default + the pointer to the
storage location feels a bit ugly.

> > -module_param_named(modeset, i915.modeset, int, 0400);
> 
> Wait, WTF?  Why do you prefix i915 here manually?  That means that
> the commandline parameter would be "i915.i915.modeset=" and the
> module parameter would be "i915.modeset="...

Nope, this is the named macro. The name of the param is the first
parameter to the macro "modeset", "i915.modeset" is just the variable
it'll get stored in. We've specifically switched to the _named version to
avoid ugly i915.i915* paramters ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] [RFC] Taint the kernel for unsafe module options

[Rusty Russell]

Daniel Vetter <daniel@ffwll.ch> writes:
> On Thu, Mar 06, 2014 at 11:19:54AM +1030, Rusty Russell wrote:
>> Daniel Vetter <daniel.vetter@ffwll.ch> writes:
>> > 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. In drm/i915 all
>> > these options follow the same pattern of using -1 as the per-machine
>> > default, and any other value being used for force the parameter.
>> >
>> > Adding a pile of cc's to solicit input and figure out whether this
>> > would be generally useful - this quick rfc is just for drm/i915.
>> 
>> If this is a good idea, you can write a macro module_param_unsafe_named
>> which is a general wrapper.
>
> For this to work I need to somehow store the safe default value somewhere.
> since with bools or strings there really isn't such a thing, even less
> than with integers where my fairly abitrary -1 choice is already
> restricting. But I don't have a good idea how to do that, since creating a
> local static struct in the macro to store the default + the pointer to the
> storage location feels a bit ugly.

I was thinking that if use the parameter, they get marked unsafe.  If
they use it to set it to the default, Don't Do That.

>> > -module_param_named(modeset, i915.modeset, int, 0400);
>> 
>> Wait, WTF?  Why do you prefix i915 here manually?  That means that
>> the commandline parameter would be "i915.i915.modeset=" and the
>> module parameter would be "i915.modeset="...
>
> Nope, this is the named macro. The name of the param is the first
> parameter to the macro "modeset", "i915.modeset" is just the variable
> it'll get stored in. We've specifically switched to the _named version to
> avoid ugly i915.i915* paramters ;-)

Oh, oops, my bad reading!  I'll shut up now :)

Thanks,
Rusty.

Re: [PATCH] [RFC] Taint the kernel for unsafe module options

[Daniel Vetter]

On Fri, Mar 7, 2014 at 4:28 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>> If this is a good idea, you can write a macro module_param_unsafe_named
>>> which is a general wrapper.
>>
>> For this to work I need to somehow store the safe default value somewhere.
>> since with bools or strings there really isn't such a thing, even less
>> than with integers where my fairly abitrary -1 choice is already
>> restricting. But I don't have a good idea how to do that, since creating a
>> local static struct in the macro to store the default + the pointer to the
>> storage location feels a bit ugly.
>
> I was thinking that if use the parameter, they get marked unsafe.  If
> they use it to set it to the default, Don't Do That.

Makes sense. I'll try to come up with something more polished which
addresses your's and Andrew's comments somewhen next week. Presuming
the (here one week later than everywhere else) carnival doesn't
interfere too badly ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch