hwmon: (ibmpex) fix use-after-free in high/low store
A message that is not in the archive (sent off-list or not yet received): MEYPR01MB7886BE2F51BFE41875B74B60AFA0A()MEYPR01MB7886!ausprd01!prod!outlook!com
Hi Junrui, Guenter,
Quoted 21 lines · Junrui Luo
The
ibmpex_high_low_store()function retrieves driver data usingdev_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 <[email protected]>Reported-by Junrui Luo <[email protected]>Fixes 57c7c3a0fdea ("hwmon: IBM power meter driver")Signed-off-by Junrui Luo <[email protected]>--- drivers/hwmon/ibmpex.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)drivers/hwmon/ibmpex.c +0
-0
drivers/hwmon/ibmpex.c static ssize_t ibmpex_high_low_store(struct device *dev, · line 282
Quoted 6 lines
drivers/hwmon/ibmpex.c +3
-0@@ -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).
drivers/hwmon/ibmpex.c static ssize_t ibmpex_high_low_store(struct device *dev, · line 285
Quoted 3 lines
drivers/hwmon/ibmpex.c +0
-0ibmpex_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).
Signature · 2 lines
Jean Delvare SUSE L3 Support
Guenter Roeck
maintainerQuoted 49 lines · Jean Delvare
Hi Junrui, Guenter,
Quoted 21 lines · Junrui Luo
The
ibmpex_high_low_store()function retrieves driver data usingdev_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 <[email protected]>Reported-by Junrui Luo <[email protected]>Fixes 57c7c3a0fdea ("hwmon: IBM power meter driver")Signed-off-by Junrui Luo <[email protected]>--- drivers/hwmon/ibmpex.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)drivers/hwmon/ibmpex.c +0
-0drivers/hwmon/ibmpex.c static ssize_t ibmpex_high_low_store(struct device *dev, · line 282
Quoted 6 lines
drivers/hwmon/ibmpex.c +3
-0@@ -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 callsdev_get_drvdata(dev).drivers/hwmon/ibmpex.c static ssize_t ibmpex_high_low_store(struct device *dev, · line 285
Quoted 3 lines
drivers/hwmon/ibmpex.c +0
-0ibmpex_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), ordevice_remove_file()doesn't wait and the code is still racy even after your fix, because then nothing preventsibmpex_bmc_delete()from finishing (hence freeing data) while the writer is still inibmpex_high_low_store()(after the !data check, but before callingibmpex_reset_high_low_data, for example).
Quoted 9 lines
Or am I missing something?
Does anyone know whether sysfs guarantees that
device_remove_file()blocks until all user-space users have calledclose()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 <[email protected]>
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.
drivers/hwmon/ibmpex.c static void ibmpex_bmc_delete(struct ibmpex_bmc_data *data) · line 516
Quoted 11 lines
drivers/hwmon/ibmpex.c +3
-0@@ -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()?
drivers/hwmon/ibmpex.c static void ibmpex_bmc_delete(struct ibmpex_bmc_data *data) · line 519
Quoted 3 lines
drivers/hwmon/ibmpex.c +0
-0device_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 <[email protected]>
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.
Quoted 4 lines
drivers/hwmon/ibmpex.c +0
-0
[ ... ]
drivers/hwmon/ibmpex.c static void ibmpex_bmc_delete(struct ibmpex_bmc_data *data) · line 519
Quoted 10 lines
drivers/hwmon/ibmpex.c +3
-0@@ -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.
Quoted 10 lines
drivers/hwmon/ibmpex.c +1
-2@@ -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);
Quoted 2 lines · Guenter Roeck
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