hwmon: (hp-wmi-sensors) Fix use-after-free in fungible_show()
Subsystems (3)
PATCH 1/1 hwmon: (hp-wmi-sensors) Fix use-after-free in fungible_show()
nsensor->current_state is dynamically replaced as the sensor's state changes. update_numeric_sensor_from_wobj() does this by freeing the old string and installing a new one:
if (strcmp(trimmed, nsensor->current_state)) {
new_string = hp_wmi_strdup(dev, trimmed);
if (new_string) {
devm_kfree(dev, nsensor->current_state);
nsensor->current_state = new_string;
}
}This function is only ever called from hp_wmi_update_info() while state->lock is held, so the free-and-replace itself is properly serialized against concurrent updates.
fungible_show(), however, reads the same pointer after the lock has already been dropped:
err = hp_wmi_update_info(state, info);
if (err)
return err;
switch (prop) {
...
case HP_WMI_PROPERTY_CURRENT_STATE:
seq_printf(seqf, "%s\n", nsensor->current_state);
break;hp_wmi_update_info() takes state->lock internally and releases it before returning, so by the time fungible_show() dereferences nsensor->current_state in seq_printf(), no lock is held. Two processes reading a sensor's current_state debugfs entry at overlapping times (or one reading it while another read of the same sensor triggers a refresh) can race: one thread's seq_printf() can be part-way through printing the string at the moment another thread's call into update_numeric_sensor_from_wobj() frees it with devm_kfree() and installs a new pointer, causing a use-after-free read.
Take state->lock around the read in fungible_show() as well, so it can never run concurrently with the free-and-replace in update_numeric_sensor_from_wobj().
--- drivers/hwmon/hp-wmi-sensors.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/hwmon/hp-wmi-sensors.c +2 -0
@@ -1247,7 +1247,9 @@ static int fungible_show(struct seq_file *seqf, enum hp_wmi_property prop)break ;case HP_WMI_PROPERTY_CURRENT_STATE : +mutex_lock (&state->lock);seq_printf (seqf,"%s\n" , nsensor->current_state); +mutex_unlock (&state->lock);break ;case HP_WMI_PROPERTY_UNIT_MODIFIER :
Signature · 1 line
2.43.0
[email protected]
AI reviewSashiko has reviewed this patch and found no issues. It looks great!
Quoted 18 lines
nsensor->current_stateis dynamically replaced as the sensor's state changes.update_numeric_sensor_from_wobj()does this by freeing the old string and installing a new one:if (strcmp(trimmed, nsensor->current_state)) { new_string = hp_wmi_strdup(dev, trimmed); if (new_string) { devm_kfree(dev, nsensor->current_state); nsensor->current_state = new_string; } }This function is only ever called from
hp_wmi_update_info()whilestate->lockis held, so the free-and-replace itself is properly serialized against concurrent updates.
[ ... ]update_numeric_sensor_from_wobj().
Quoted 3 lines
Fixes 23902f98f8d4 ("hwmon: add HP WMI Sensors driver")Signed-off-by Muhammad Bilal <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Quoted 10 lines · Muhammad Bilal
hp_wmi_update_info()takesstate->lockinternally and releases it before returning, so by the timefungible_show()dereferencesnsensor->current_stateinseq_printf(), no lock is held. Two processes reading a sensor'scurrent_statedebugfs entry at overlapping times (or one reading it while another read of the same sensor triggers a refresh) can race: one thread'sseq_printf()can be part-way through printing the string at the moment another thread's call intoupdate_numeric_sensor_from_wobj()frees it withdevm_kfree()and installs a new pointer, causing a use-after-free read.
Thanks for identifying this issue.
Quoted 28 lines
Take
state->lockaround the read infungible_show()as well, so it can never run concurrently with the free-and-replace inupdate_numeric_sensor_from_wobj().Fixes 23902f98f8d4 ("hwmon: add HP WMI Sensors driver")Signed-off-by Muhammad Bilal <[email protected]>--- drivers/hwmon/hp-wmi-sensors.c | 2 ++ 1 file changed, 2 insertions(+)drivers/hwmon/hp-wmi-sensors.c +2
-0@@ -1247,7 +1247,9 @@ static int fungible_show(struct seq_file *seqf, enum hp_wmi_property prop)break ;case HP_WMI_PROPERTY_CURRENT_STATE : +mutex_lock (&state->lock);seq_printf (seqf,"%s\n" , nsensor->current_state); +mutex_unlock (&state->lock);break ;case HP_WMI_PROPERTY_UNIT_MODIFIER :Signature · 1 line
2.43.0
Fine with me.
Quoted 40 lines · Muhammad Bilal
nsensor->current_stateis dynamically replaced as the sensor's state changes.update_numeric_sensor_from_wobj()does this by freeing the old string and installing a new one:if (strcmp(trimmed, nsensor->current_state)) { new_string = hp_wmi_strdup(dev, trimmed); if (new_string) { devm_kfree(dev, nsensor->current_state); nsensor->current_state = new_string; } }This function is only ever called from
hp_wmi_update_info()whilestate->lockis held, so the free-and-replace itself is properly serialized against concurrent updates.
fungible_show(), however, reads the same pointer after the lock has already been dropped:err = hp_wmi_update_info(state, info); if (err) return err; switch (prop) { ... case HP_WMI_PROPERTY_CURRENT_STATE: seq_printf(seqf, "%s\n", nsensor->current_state); break;
hp_wmi_update_info()takesstate->lockinternally and releases it before returning, so by the timefungible_show()dereferencesnsensor->current_stateinseq_printf(), no lock is held. Two processes reading a sensor'scurrent_statedebugfs entry at overlapping times (or one reading it while another read of the same sensor triggers a refresh) can race: one thread'sseq_printf()can be part-way through printing the string at the moment another thread's call intoupdate_numeric_sensor_from_wobj()frees it withdevm_kfree()and installs a new pointer, causing a use-after-free read.
Quoted 8 lines
Take
state->lockaround the read infungible_show()as well, so it can never run concurrently with the free-and-replace inupdate_numeric_sensor_from_wobj().Fixes 23902f98f8d4 ("hwmon: add HP WMI Sensors driver")Signed-off-by Muhammad Bilal <[email protected]>Acked-by James Seo <[email protected]>
Applied.
Thanks,
Guenter