linux-kernel - Re: [PATCH v2 02/11] hwmon: (amc6821) Make reading and writing fan speed limits consistent

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8cb9dc38-499e-4f10-93a4-c07898a776b4@roeck-us.net>
Date: Wed, 3 Jul 2024 14:48:50 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Quentin Schulz <quentin.schulz@...rry.de>, linux-hwmon@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Farouk Bouabid <farouk.bouabid@...rry.de>
Subject: Re: [PATCH v2 02/11] hwmon: (amc6821) Make reading and writing fan
 speed limits consistent

On 7/3/24 07:35, Quentin Schulz wrote:
> Hi Guenter,
> 
> On 7/1/24 11:23 PM, Guenter Roeck wrote:
>> The default value of the maximum fan speed limit register is 0,
>> essentially translating to an unlimited fan speed. When reading
>> the limit, a value of 0 is reported in this case. However, writing
>> a value of 0 results in writing a value of 0xffff into the register,
>> which is inconsistent.
>>
>> To solve the problem, permit writing a limit of 0 for the maximim fan
>> speed, effectively translating to "no limit". Write 0 into the register
>> if a limit value of 0 is written. Otherwise limit the range to
>> <1..6000000> and write 1..0xffff into the register. This ensures that
>> reading and writing from and to a limit register return the same value
>> while at the same time not changing reported values when reading the
>> speed or limits.
>>
>> While at it, restrict fan limit writes to non-negative numbers; writing
>> a negative limit does not make sense and should be reported instead of
>> being corrected.
>>
>> Signed-off-by: Guenter Roeck <linux@...ck-us.net>
>> ---
>> v2: Do not accept negative fan speed values
>>      Display fan speed and speed limit as 0 if register value is 0
>>      (instead of 6000000), as in original code.
>>      Only permit writing 0 (unlimited) for the maximum fan speed.
>>
>>   drivers/hwmon/amc6821.c | 13 +++++++++----
>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
>> index eb2d5592a41a..9c19d4d278ec 100644
>> --- a/drivers/hwmon/amc6821.c
>> +++ b/drivers/hwmon/amc6821.c
>> @@ -617,15 +617,20 @@ static ssize_t fan_store(struct device *dev, struct device_attribute *attr,
>>   {
>>       struct amc6821_data *data = dev_get_drvdata(dev);
>>       struct i2c_client *client = data->client;
>> -    long val;
>> +    unsigned long val;
>>       int ix = to_sensor_dev_attr(attr)->index;
>> -    int ret = kstrtol(buf, 10, &val);
>> +    int ret = kstrtoul(buf, 10, &val);
>>       if (ret)
>>           return ret;
>> -    val = 1 > val ? 0xFFFF : 6000000/val;
>> +
>> +    /* The minimum fan speed must not be unlimited (0) */
>> +    if (ix == IDX_FAN1_MIN && !val)
>> +        return -EINVAL;
>> +
>> +    val = val > 0 ? 6000000 / clamp_val(val, 1, 6000000) : 0;
> 
> I'm wondering if we shouldn't check !val for min after this line instead? Otherwise we allow 6000001+RPM speeds... which is technically unlimited.
> 

If ix == IDX_FAN1_MIN, val must be positive because of the check above.
The expression "6000000 / clamp_val(val, 1, 6000000)" is therefore always
positive as well because val is clamped. Its minimum result would be
6000000/6000000 = 1. The alternate case of the ternary expression would
never hit because it is guaranteed that val > 0. Am I missing something ?

Thanks,
Guenter