Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] hwmon: (dell-smm-hwmon) Disable BIOS fan control on
 SET_FAN
To:     =?UTF-8?Q?Pali_Roh=c3=a1r?= <pali.rohar@gmail.com>,
        Giovanni Mascellani <gio@debian.org>
Cc:     Jean Delvare <jdelvare@suse.com>, linux-hwmon@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20191114211408.22123-1-gio@debian.org>
 <20191114213901.GA28532@roeck-us.net> <20191114215154.hze2avicv7pwiksp@pali>
 <7ea74678-59e4-2c23-6744-6d0b2eff0a67@debian.org>
 <20191115112902.yfaoqttsafmilijh@pali>
 <97d17b37-0267-ae32-803c-ce8f7a871ab4@debian.org>
 <20191115143654.xxpxd4xrrapvv5xu@pali>
From:   Guenter Roeck <linux@roeck-us.net>
Message-ID: <afdf3d85-1950-2eec-a826-718a1c28f460@roeck-us.net>
Date:   Fri, 15 Nov 2019 06:46:20 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.9.0
MIME-Version: 1.0
In-Reply-To: <20191115143654.xxpxd4xrrapvv5xu@pali>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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