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

[Guenter Roeck]

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