Re: [PATCH] hwmon: (ibmpex) fix use-after-free in high/low store

[Jean Delvare <jdelvare@suse.de>]

Hi Junrui, Guenter,

On Wed, 10 Dec 2025 09:48:08 +0000, Junrui Luo wrote:
> The ibmpex_high_low_store() function retrieves driver data using
> dev_get_drvdata() and uses it without validation. This creates a race
> condition where the sysfs callback can be invoked after the data
> structure is freed, leading to use-after-free.
> 
> Fix by adding a NULL check after dev_get_drvdata(), and reordering
> operations in the deletion path to prevent TOCTOU.
> 
> Reported-by: Yuhao Jiang <danisjiang@gmail.com>
> Reported-by: Junrui Luo <moonafterrain@outlook.com>
> Fixes: 57c7c3a0fdea ("hwmon: IBM power meter driver")
> Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
> ---
>  drivers/hwmon/ibmpex.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/ibmpex.c b/drivers/hwmon/ibmpex.c
> index 228c5f6c6f38..129f3a9e8fe9 100644
> --- a/drivers/hwmon/ibmpex.c
> +++ b/drivers/hwmon/ibmpex.c
> @@ -277,6 +277,9 @@ static ssize_t ibmpex_high_low_store(struct device *dev,
>  {
>  	struct ibmpex_bmc_data *data = dev_get_drvdata(dev);
>  
> +	if (!data)
> +		return -ENODEV;
> +

If this is needed here, then why don't we add a similar check to
ibmpex_show_sensor()? It's also called by accessing the device's sysfs
attributes, and also calls dev_get_drvdata(dev).

>  	ibmpex_reset_high_low_data(data);
>  
>  	return count;

To be honest, I don't really understand the purpose of this fix. As I
read the code, either device_remove_file() waits for the file writer to
finish before returning and the code was already safe before (because
data is freed after removing the attribute files), or
device_remove_file() doesn't wait and the code is still racy even after
your fix, because then nothing prevents ibmpex_bmc_delete() from
finishing (hence freeing data) while the writer is still in
ibmpex_high_low_store() (after the !data check, but before calling
ibmpex_reset_high_low_data, for example).

Or am I missing something?

Does anyone know whether sysfs guarantees that device_remove_file()
blocks until all user-space users have called close() on the file?

If it does, then I believe this fix wasn't needed. It not, then I
believe this fix is not sufficient (it would shrink the race window
but does not close it completely).

-- 
Jean Delvare
SUSE L3 Support