linux-kernel - Re: [PATCH] hwmon: pmbus: protect read-modify-write with lock

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] [day] [month] [year] [list]

Message-ID: <20190531091531.GA10821@localhost.localdomain>
Date:   Fri, 31 May 2019 10:12:35 +0000
From:   "Adamski, Krzysztof (Nokia - PL/Wroclaw)" 
        <krzysztof.adamski@...ia.com>
To:     Guenter Roeck <linux@...ck-us.net>
CC:     Jean Delvare <jdelvare@...e.com>,
        "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] hwmon: pmbus: protect read-modify-write with lock

On Thu, May 30, 2019 at 10:21:20AM -0700, Guenter Roeck wrote:
>Hi,
>
>On Thu, May 30, 2019 at 06:45:48AM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
>> The operation done in the pmbus_update_fan() function is a
>> read-modify-write operation but it lacks any kind of lock protection
>> which may cause problems if run more than once simultaneously. This
>> patch uses an existing update_lock mutex to fix this problem.
>>
>> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@...ia.com>
>> ---
>>
>> I'm resending this patch to proper recipients this time. Sorry if the
>> previous submission confused anybody.
>>
>>  drivers/hwmon/pmbus/pmbus_core.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
>> index ef7ee90ee785..94adbede7912 100644
>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>> @@ -268,6 +268,7 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
>>  	int rv;
>>  	u8 to;
>>
>> +	mutex_lock(&data->update_lock);
>>  	from = pmbus_read_byte_data(client, page,
>>  				    pmbus_fan_config_registers[id]);
>>  	if (from < 0)
>> @@ -278,11 +279,15 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
>>  		rv = pmbus_write_byte_data(client, page,
>>  					   pmbus_fan_config_registers[id], to);
>>  		if (rv < 0)
>> -			return rv;
>> +			goto out;
>>  	}
>>
>> -	return _pmbus_write_word_data(client, page,
>> -				      pmbus_fan_command_registers[id], command);
>> +	rv = _pmbus_write_word_data(client, page,
>> +				    pmbus_fan_command_registers[id], command);
>> +
>> +out:
>> +	mutex_lock(&data->update_lock);
>
>Should be mutex_unlock(), meaning you have not tested this ;-).
>
>Either case, I think this is unnecessary. The function is (or should be)
>always called with the lock already taken (ie with pmbus_set_sensor()
>in the call path). If not, we would need a locked and an unlocked version
>of this function to avoid lock recursion.

You've got me :) I did not test that as I do not have a workflow using
this. I just stumbled opon this when looking at the code related to my
other patches. So it was more like a - "hey, shouldn't there be a lock
here?". But I was wrong, thanks.

Krzysztof