Re: [PATCH 1/7] hwmon: (dell-smm) Allow for specifying fan control method as module parameter

From: "Pali Rohár" <pali@kernel.org>
To: Armin Wolf <W_Armin@gmx.de>
Cc: jdelvare@suse.com, linux@roeck-us.net,
	linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/7] hwmon: (dell-smm) Allow for specifying fan control method as module parameter
Date: Tue, 15 Feb 2022 21:31:51 +0100	[thread overview]
Message-ID: <20220215203151.kajdh6uezx5mzyal@pali> (raw)
In-Reply-To: <6c9a1f7a-004e-1991-abef-181cc117907e@gmx.de>

On Tuesday 15 February 2022 21:19:17 Armin Wolf wrote:
> Am 15.02.22 um 20:49 schrieb Pali Rohár:
> > On Tuesday 15 February 2022 20:44:20 Armin Wolf wrote:
> > > Am 15.02.22 um 20:19 schrieb Pali Rohár:
> > > > On Tuesday 15 February 2022 20:11:07 Armin Wolf wrote:
> > > > > Right now, the only way to test if setting manual/auto fan control works
> > > > > is to edit and recompile the module, which may be too cumbersome for
> > > > > the average user.
> > > > There is also another way suitable for testing purposes which do not
> > > > requires any kernel patch. Call iopl(3) syscall which changes I/O
> > > > privilege level to 3 and which allows to poke I/O registers.
> > > > 
> > > This is not possible under kernel lockdown/Secure Boot.
> > Under Secure Boot it should be still possible.
> 
> Afaik, access to iopl() and ioperm() is blocked in this mode too.
> This for example causes dell-bios-fan-control to fail when
> the machine is booted in Secure Boot mode.

Well, then it is the cost of the usage of Secure Boot. If you need
additional functionality then disable it.

> > With kernel lockdown, no kernel testing/debugging at HW level is
> > acceptable due to security reasons, as it is against what kernel
> > lockdown should achieve.
> > 
> That is the reason i limited the number of selectable code pairs to
> two which are known to work on a wide range of machines.
> So users are unable to specify SMM commands, which are controlled by
> the driver. If this still is to much, then we may drop this patch.

This kernel driver dell-smm-hwmon should use same security model as
other parts in kernel. So if kernel disallow user to specify custom
parameters for io ports (those which are not provided by platform data)
then it should be disabled also in this driver.

It is really _bad_ practice to trying invent a security hole to the
existing security model.

> > > > > Allow for specifying the desired fan mode control method when loading
> > > > > the module, but taint the kernel if so since there is the possibility
> > > > > for strange side effects on non-whitelisted models.
> > > > > Also update docs and kernel-parameters.txt accordingly.
> > > > > 
> > > > > Tested on a Dell Inspiron 3505.
> > > > > 
> > > > > Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> > > > > ---
> > > > >    .../admin-guide/kernel-parameters.txt         |  3 +++
> > > > >    Documentation/hwmon/dell-smm-hwmon.rst        | 21 ++++++++++------
> > > > >    drivers/hwmon/dell-smm-hwmon.c                | 25 +++++++++++++------
> > > > >    3 files changed, 35 insertions(+), 14 deletions(-)
> > > > > 
> > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > > > index d68053db21cc..4f1b6c2b7ed1 100644
> > > > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > > > @@ -968,6 +968,9 @@
> > > > >    	dell_smm_hwmon.fan_max=
> > > > >    			[HW] Maximum configurable fan speed.
> > > > > 
> > > > > +	dell_smm_hwmon.fan_mode_method=
> > > > > +			[HW] Method to use for changing fan mode.
> > > > > +
> > > > >    	dfltcc=		[HW,S390]
> > > > >    			Format: { on | off | def_only | inf_only | always }
> > > > >    			on:       s390 zlib hardware support for compression on
> > > > > diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
> > > > > index beec88491171..564d99cda869 100644
> > > > > --- a/Documentation/hwmon/dell-smm-hwmon.rst
> > > > > +++ b/Documentation/hwmon/dell-smm-hwmon.rst
> > > > > @@ -67,13 +67,16 @@ for your hardware. It is possible that codes that work for other
> > > > >    laptops actually work for yours as well, or that you have to discover
> > > > >    new codes.
> > > > > 
> > > > > -Check the list ``i8k_whitelist_fan_control`` in file
> > > > > -``drivers/hwmon/dell-smm-hwmon.c`` in the kernel tree: as a first
> > > > > -attempt you can try to add your machine and use an already-known code
> > > > > -pair. If, after recompiling the kernel, you see that ``pwm1_enable``
> > > > > -is present and works (i.e., you can manually control the fan speed),
> > > > > -then please submit your finding as a kernel patch, so that other users
> > > > > -can benefit from it. Please see
> > > > > +As a first step, you can load the module with the module parameter
> > > > > +``fan_mode_method`` set to 1 to test if your hardware works with
> > > > > +an already know method for disabling automatic BIOS fan control.
> > > > > +If ``pwm1_enable`` is now present and works (i.e., you can
> > > > > +manually control the fan speed), then please submit your finding
> > > > > +as a kernel patch, so that other users can benefit from it.
> > > > > +Just add your model to the list ``i8k_whitelist_fan_control`` in
> > > > > +file ``drivers/hwmon/dell-smm-hwmon.c`` in the kernel tree and use
> > > > > +the already known code pair.
> > > > > +Please read
> > > > >    :ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
> > > > >    for information on submitting patches.
> > > > > 
> > > > > @@ -120,6 +123,10 @@ Module parameters
> > > > >                       Maximum configurable fan speed. (default:
> > > > >                       autodetect)
> > > > > 
> > > > > +* fan_mode_method:uint
> > > > > +                   Method to use for changing fan mode (default:
> > > > > +                   from whitelist)
> > > > > +
> > > > >    Legacy ``/proc`` interface
> > > > >    --------------------------
> > > > > 
> > > > > diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> > > > > index 9949eeb79378..1c4cc516c8b2 100644
> > > > > --- a/drivers/hwmon/dell-smm-hwmon.c
> > > > > +++ b/drivers/hwmon/dell-smm-hwmon.c
> > > > > @@ -111,6 +111,10 @@ static uint fan_max;
> > > > >    module_param(fan_max, uint, 0);
> > > > >    MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed (default: autodetect)");
> > > > > 
> > > > > +static uint fan_mode_method;
> > > > > +module_param_unsafe(fan_mode_method, uint, 0);
> > > > > +MODULE_PARM_DESC(fan_mode_method, "Method to use for changing fan mode (default: from whitelist)");
> > > > No, please really do not introduce another kernel parameter for this
> > > > driver. There are already many and we do not need to extend this list.
> 
> Since there are two code pairs, i could not reuse the force param for that.
> Since the newly added pair is unused, should i remove it and reuse the
> force param?

For such things like this there is debugfs, if you really need to
specify / provide custom values.

> > > > > +
> > > > >    struct smm_regs {
> > > > >    	unsigned int eax;
> > > > >    	unsigned int ebx;
> > > > > @@ -677,7 +681,7 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
> > > > > 
> > > > >    			break;
> > > > >    		case hwmon_pwm_enable:
> > > > > -			if (data->auto_fan)
> > > > > +			if (data->auto_fan && data->manual_fan)
> > > > >    				/*
> > > > >    				 * There is no command for retrieve the current status
> > > > >    				 * from BIOS, and userspace/firmware itself can change
> > > > > @@ -1282,14 +1286,21 @@ static int __init dell_smm_probe(struct platform_device *pdev)
> > > > >    	data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
> > > > >    	data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max);
> > > > > 
> > > > > -	fan_control = dmi_first_match(i8k_whitelist_fan_control);
> > > > > -	if (fan_control && fan_control->driver_data) {
> > > > > -		const struct i8k_fan_control_data *control = fan_control->driver_data;
> > > > > +	/* value specified via module param overrides whitelist */
> > > > > +	if (fan_mode_method > 0 && fan_mode_method <= ARRAY_SIZE(i8k_fan_control_data)) {
> > > > > +		data->manual_fan = i8k_fan_control_data[fan_mode_method - 1].manual_fan;
> > > > > +		data->auto_fan = i8k_fan_control_data[fan_mode_method - 1].auto_fan;
> > > > > +	} else {
> > > > > +		fan_control = dmi_first_match(i8k_whitelist_fan_control);
> > > > > +		if (fan_control && fan_control->driver_data) {
> > > > > +			const struct i8k_fan_control_data *control = fan_control->driver_data;
> > > > > 
> > > > > -		data->manual_fan = control->manual_fan;
> > > > > -		data->auto_fan = control->auto_fan;
> > > > > -		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
> > > > > +			data->manual_fan = control->manual_fan;
> > > > > +			data->auto_fan = control->auto_fan;
> > > > > +		}
> > > > >    	}
> > > > > +	if (data->manual_fan && data->auto_fan)
> > > > > +		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
> > > > > 
> > > > >    	if (!fan_mult) {
> > > > >    		/*
> > > > > --
> > > > > 2.30.2
> > > > > 