Re: [PATCH] hwmon: (dell-smm-hwmon) Disable BIOS fan control on SET_FAN

[Guenter Roeck <linux@roeck-us.net>]

On 11/15/19 6:36 AM, Pali Rohár wrote:
> On Friday 15 November 2019 14:44:59 Giovanni Mascellani wrote:
>> Hi,
>>
>> Il 15/11/19 12:29, Pali Rohár ha scritto:
>>> No. I have not tested that my patch on other models. You can look at
>>> that my patch link, some people already reported that on some models it
>>> does not work...
>>
>> Yes, I saw that. But they are also using other laptops, which could be
>> excluded by the whitelist until we have a working command for them.
>>
>>> What is incompatible with Secure Boot? sys_iopl nor sys_ioperm has
>>> nothing to do with UEFI Secure Boot. They are just ordinary syscalls
>>> like any other and are executed on kernel side. And IIRC it is up to the
>>> kernel how it set privileges for I/O ports. Maybe bootloaders under
>>> Secure Boot can set some other default values, but kernel can overwrite
>>> them. I really do not see reason why it could be incompatible with UEFI
>>> Secure Boot nor why it should not work. It just looks like improper
>>> setup from userspace.
>>
>> Ah, my fault here: there is a patch to lock down the kernel when it is
>> started with Secure Boot[1], and I believed that was already in
>> mainline, but apparently it is not.
> 
> Ok, so, this is not a problem.
> 
> I hope that such patch is not going to be part of mainline kernel as it
> would break lot of things. UEFI Secure Boot and kernel lock down are two
> different things. It would be really suspicious that for "workarounding"
> broken functionality would be needed to turn of totally unrelated option
> in firmware SETUP.
> 
>>   [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit?id=160a99536afc317b337212dd40eaba341702343e
>>
>>> It makes sense to have implemented in kernel switching between automatic
>>> and manual mode as kernel has API for it. But it depends on the fact
>>> that we know which SMM API to call. And currently it is just some random
>>> call which we somehow observed that is working on 2 machines and on some
>>> more other does not. Until we have fully working implementation we
>>> cannot put it into kernel.
>>
>> Mmh, but then what is a plausible way forward to have this? Can't we
>> start populating a whitelist for the machines we already have a solution
>> for, and add more entries when they are discovered? This would already
>> give a benefit to the users of supported laptops, without impacting
>> users of unsupported laptops. My feeling is that if we pretend to have
>> information for all possible models supported by dell-smm-hwmon, we will
>> never benefit any user. Or can you suggest a plan?
> 
> The following question is up to the hwmon maintainers. Guenter, should
> we start creating whilelist of machines which support those SMM calls
> for enabling / disabling BIOS auto mode? And maintaining this whilelist
> in kernel dell-smm-hwmon driver?
> 
Ok with me.

>>> What does not make sense for me is to have that "protection" in kernel.
>>
>> I am not really sure which "protection" you mean. I didn't mean to
>> introduce any protection from userspace in my patch, I just wanted to
>> make SET_FAN working. I think that the kernel module cannot (and should
>> not) reliably protect itself from userspace sending random IO port
>> reads/writes.
> 
> I mean protection to disallow calling SET_FAN operation when auto mode
> is enabled.
> 
I don't have a problem with that, as long as it only applies in conjunction
with the whitelist. The whitelist would determine if the pwmX_enable attribute
is supported. If it is, pwmX can only be written if pwm1_enable is set to 1
(manual fan speed control). This is pretty common for other drivers.

Guenter