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

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

[Jean Delvare]

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

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

[Guenter Roeck]

On Wed, Jan 21, 2026 at 09:53:42AM +0100, Jean Delvare wrote:
> 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).
> 
Gemini (using the 3 pro model and Chris Mason's prompts as guidance) agrees that
the fix is not complete. Review attached below.

I then asked it if the fix is needed and, if so, if it is complete.
Response below as well. In its verbal response it also said

"The original code likely did not have the UAF issue described in the commit
 message because device_remove_file() waits for active sysfs callbacks to
 complete. If ibmpex_high_low_store was running, device_remove_file would
 block until it finished, preventing kfree(data) from happening concurrently.
"

Based on that, I am going to revert the patch. Feel free to resubmit with a
complete fix but show that the fix is both needed and complete.

Thanks,
Guenter

---
# Commit 6946c726c3f4 ("hwmon: (ibmpex) fix use-after-free in high/low store")
commit 6946c726c3f4c36f0f049e6f97e88c510b15f65d
Author: Junrui Luo <moonafterrain@outlook.com>
Subject: hwmon: (ibmpex) fix use-after-free in high/low store

This commit fixes a use-after-free in ibmpex_high_low_store by checking for
NULL driver data, and reorders cleanup operations in ibmpex_bmc_delete to
clear driver data early.

Link: https://lore.kernel.org/r/MEYPR01MB7886BE2F51BFE41875B74B60AFA0A@MEYPR01MB7886.ausprd01.prod.outlook.com

> 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
> @@ -508,6 +511,9 @@ static void ibmpex_bmc_delete(struct ibmpex_bmc_data *data)
>  {
>  	int i, j;
>  
> +	hwmon_device_unregister(data->hwmon_dev);
> +	dev_set_drvdata(data->bmc_device, NULL);
> +

This change sets drvdata to NULL before removing the sensor attributes.

If a user reads a sensor attribute (e.g., temp1_input) during this window,
ibmpex_show_sensor() will run.

ibmpex_show_sensor() calls dev_get_drvdata(), which now returns NULL.
It then dereferences data->sensors without checking for NULL.

Does this introduce a NULL pointer dereference in ibmpex_show_sensor()?

>  	device_remove_file(data->bmc_device,
>  			   &sensor_dev_attr_reset_high_low.dev_attr);
>  	device_remove_file(data->bmc_device, &dev_attr_name.attr);

---
commit 6946c726c3f4c36f0f049e6f97e88c510b15f65d
Author: Junrui Luo <moonafterrain@outlook.com>
Subject: hwmon: (ibmpex) fix use-after-free in high/low store

Fix a use-after-free race in ibmpex_high_low_store() by adding a NULL check
for driver data and reordering cleanup operations in ibmpex_bmc_delete().
The cleanup reordering ensures driver data is cleared before file removal.
Link: https://lore.kernel.org/r/MEYPR01MB7886BE2F51BFE41875B74B60AFA0A@MEYPR01MB7886.ausprd01.prod.outlook.com

> 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
[ ... ]
> @@ -508,6 +511,9 @@ static void ibmpex_bmc_delete(struct ibmpex_bmc_data *data)
>  {
>  	int i, j;
>  
> +	hwmon_device_unregister(data->hwmon_dev);
> +	dev_set_drvdata(data->bmc_device, NULL);
> +
>  	device_remove_file(data->bmc_device,
>  			   &sensor_dev_attr_reset_high_low.dev_attr);
>  	device_remove_file(data->bmc_device, &dev_attr_name.attr);

This reordering sets the driver data to NULL before removing the sensor
attributes in the loop below.

ibmpex_show_sensor() retrieves this driver data via dev_get_drvdata() but
does not check if it is NULL before dereferencing it to access
data->sensors[].

If a userspace process reads a sensor file (like temp1_input) while this
delete function is running, could it race with the dev_set_drvdata(..., NULL)
call here and crash in ibmpex_show_sensor()?

Would it be safer to keep the original order where device_remove_file() is
called before clearing the driver data? device_remove_file() should wait for
any active sysfs callbacks to complete, which might already prevent the
use-after-free this patch intends to fix.

> @@ -521,8 +527,7 @@ static void ibmpex_bmc_delete(struct ibmpex_bmc_data *data)
>  		}
>  
>  	list_del(&data->list);
> -	dev_set_drvdata(data->bmc_device, NULL);
> -	hwmon_device_unregister(data->hwmon_dev);
> +
>  	ipmi_destroy_user(data->user);
>  	kfree(data->sensors);
>  	kfree(data);

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

[Junrui Luo]

On Sat, 7 Feb 2026 at 08:11:57 -0800, Guenter Roeck wrote:
> Based on that, I am going to revert the patch. Feel free to resubmit with a
> complete fix but show that the fix is both needed and complete.

Thank you Jean and Guenter for the thorough review.

After digging into the sysfs/kernfs internals, I now understand that
the original code was already safe. device_remove_file() calls into
kernfs_drain(), which does a blocking wait_event() until all active
sysfs callbacks (show/store) have completed.

My patch was unnecessary and actively harmful by moving dev_set_drvdata()
before device_remove_file(), it introduced a window where ibmpex_show_sensor()
could get a NULL pointer from dev_get_drvdata().

I agree with the revert.

Sorry for the noise, and thank you both for catching this.

Thanks,
Junrui Luo

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

[Junrui Luo]

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;
+
 	ibmpex_reset_high_low_data(data);
 
 	return count;
@@ -508,6 +511,9 @@ static void ibmpex_bmc_delete(struct ibmpex_bmc_data *data)
 {
 	int i, j;
 
+	hwmon_device_unregister(data->hwmon_dev);
+	dev_set_drvdata(data->bmc_device, NULL);
+
 	device_remove_file(data->bmc_device,
 			   &sensor_dev_attr_reset_high_low.dev_attr);
 	device_remove_file(data->bmc_device, &dev_attr_name.attr);
@@ -521,8 +527,7 @@ static void ibmpex_bmc_delete(struct ibmpex_bmc_data *data)
 		}
 
 	list_del(&data->list);
-	dev_set_drvdata(data->bmc_device, NULL);
-	hwmon_device_unregister(data->hwmon_dev);
+
 	ipmi_destroy_user(data->user);
 	kfree(data->sensors);
 	kfree(data);

---
base-commit: cfd4039213e7b5a828c5b78e1b5235cac91af53d
change-id: 20251210-fixes-d07b37290350

Best regards,
-- 
Junrui Luo <moonafterrain@outlook.com>

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

[Guenter Roeck]

On Wed, Dec 10, 2025 at 05:48:08PM +0800, 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>

Applied.

Thanks,
Guenter