[PATCH 0/2] hwmon: (dell-smm) Validate pwm and fan_max values

[PATCH 0/2] hwmon: (dell-smm) Validate pwm and fan_max values

[Rosen Penev]

Two input validation fixes for dell-smm-hwmon: a huge value written to
pwmN overflowed the scaling and turned the fan off, and an out-of-range
fan_max module parameter could wrap an allocation size to zero.

These patches were developed with the help of an AI coding assistant
(Claude Opus 5.5): it reviewed the drivers, found the issues and drafted
the fixes and changelogs. I reviewed every change and I am responsible
for them.

All patches build cleanly with W=1.

Tested on a Dell Precision M4800 running v7.2.15, using the equivalent
backports built as out-of-tree modules:
fan_max=256 and fan_max=4294967295 are rejected with the new message,
fan_max=3 and the default still load. The pwm clamp could not be
observed: this machine's BIOS ignores manual pwm writes.


Rosen Penev (2):
  hwmon: (dell-smm) Clamp the pwm value before scaling it
  hwmon: (dell-smm) Reject a fan_max that the BIOS cannot take

 drivers/hwmon/dell-smm-hwmon.c | 7 +++++++
 1 file changed, 7 insertions(+)


base-commit: 2800868f63227c8237d70c41e70d441c919620d6
-- 
2.56.0

[PATCH 0/3] platform/x86: dell-laptop: Fix keyboard backlight and charge threshold handling

[Rosen Penev]

Three fixes for dell-laptop: keyboard backlight timeouts that overflow
during unit conversion were silently accepted, an unread keyboard state
was used at init when the SMBIOS call failed, and the charge end
threshold was parsed as unsigned into an int.

These patches were developed with the help of an AI coding assistant
(Claude Opus 5.5): it reviewed the drivers, found the issues and drafted
the fixes and changelogs. I reviewed every change and I am responsible
for them.

All patches build cleanly with W=1.

Tested on a Dell Precision M4800 running v7.2.15, using the equivalent
backports built as out-of-tree modules:
 - writing 4424214d / 18989323d to stop_timeout set 4m / 2m before
   patch 1 and is rejected with -EINVAL after it; 5m still works
 - charge_control_end_threshold rejects -1 and 101, accepts 85
The kbd_get_state() failure path of patch 2 was not triggered.


Rosen Penev (3):
  platform/x86: dell-laptop: Reject keyboard backlight timeouts that
    overflow
  platform/x86: dell-laptop: Don't use an unread keyboard state at init
  platform/x86: dell-laptop: Parse the charge end threshold as an int

 drivers/platform/x86/dell/dell-laptop.c | 30 ++++++++++++++++++-------
 1 file changed, 22 insertions(+), 8 deletions(-)


base-commit: 2800868f63227c8237d70c41e70d441c919620d6
-- 
2.56.0

[PATCH 1/2] hwmon: (dell-smm) Clamp the pwm value before scaling it

[Rosen Penev]

dell_smm_write() scales the value written to pwmN with

	DIV_ROUND_CLOSEST(val, data->i8k_pwm_mult)

before clamping the result to the valid fan states. The rounding adds
i8k_pwm_mult / 2 to val first, which overflows for values near
LONG_MAX and wraps to a large negative number. That is then clamped to
0, so writing a huge value turns the fan off instead of running it at
the highest speed. With the default fan_max of 2, writing any value
from LONG_MAX - 63 up to LONG_MAX selects fan state 0.

Clamp val to the pwm range of 0 to 255 first.

Fixes: e7f5f275c62a ("i8k: Implement hwmon based fan speed control")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/hwmon/dell-smm-hwmon.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 5ec487843faa..e8b4e8a29e8b 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -1030,6 +1030,7 @@ static int dell_smm_write(struct device *dev, enum hwmon_sensor_types type, u32
 	case hwmon_pwm:
 		switch (attr) {
 		case hwmon_pwm_input:
+			val = clamp_val(val, 0, 255);
 			pwm = clamp_val(DIV_ROUND_CLOSEST(val, data->i8k_pwm_mult), 0,
 					data->i8k_fan_max);
 
-- 
2.56.0

Re: [PATCH 1/2] hwmon: (dell-smm) Clamp the pwm value before scaling it

[sashiko-bot]

> dell_smm_write() scales the value written to pwmN with
> 
> 	DIV_ROUND_CLOSEST(val, data->i8k_pwm_mult)
> 
> before clamping the result to the valid fan states. The rounding adds
> i8k_pwm_mult / 2 to val first, which overflows for values near
> LONG_MAX and wraps to a large negative number. That is then clamped to
> 0, so writing a huge value turns the fan off instead of running it at
> the highest speed. With the default fan_max of 2, writing any value
> from LONG_MAX - 63 up to LONG_MAX selects fan state 0.
> 
> Clamp val to the pwm range of 0 to 255 first.
> 
> Fixes: e7f5f275c62a ("i8k: Implement hwmon based fan speed control")
> Signed-off-by: Rosen Penev <rosenp@gmail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794633179.git.rosenp@gmail.com?part=1

[PATCH 2/2] hwmon: (dell-smm) Reject a fan_max that the BIOS cannot take

[Rosen Penev]

The fan_max module parameter is used without any range check, unlike
fan_mult. dell_smm_init_hwmon() allocates the nominal speed table with

	devm_kmalloc_array(dev, data->i8k_fan_max + 1, ...)

and fan_max=4294967295 wraps the count to 0. devm_kmalloc() then
returns ZERO_SIZE_PTR, which the first successful nominal speed call
writes through.

Values above 255 are also meaningless: the fan state is passed to the
BIOS as fan | (speed << 8), so the higher bits end up in other parts of
the request. Large values that do not wrap still cost one SMM call per
state and fan while probing.

Fail probing if the maximum fan state does not fit in a byte, like the
existing check for fan_mult.

Fixes: 81474fc2fae9 ("i8k: Add support for configurable maximum fan speed value")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/hwmon/dell-smm-hwmon.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index e8b4e8a29e8b..24a9172b7fed 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -1249,6 +1249,12 @@ static int dell_smm_init_data(struct device *dev, const struct dell_smm_ops *ops
 		return -EINVAL;
 	}
 	data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;
+	/* The BIOS takes the fan state in a single byte */
+	if (data->i8k_fan_max > U8_MAX) {
+		dev_err(dev, "maximum fan state %u is too large (max %u)\n",
+			data->i8k_fan_max, U8_MAX);
+		return -EINVAL;
+	}
 	data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max);
 
 	return 0;
-- 
2.56.0

Re: [PATCH 2/2] hwmon: (dell-smm) Reject a fan_max that the BIOS cannot take

[sashiko-bot]

> The fan_max module parameter is used without any range check, unlike
> fan_mult. dell_smm_init_hwmon() allocates the nominal speed table with
> 
> 	devm_kmalloc_array(dev, data->i8k_fan_max + 1, ...)
> 
> and fan_max=4294967295 wraps the count to 0. devm_kmalloc() then
> returns ZERO_SIZE_PTR, which the first successful nominal speed call
> writes through.
> 
> Values above 255 are also meaningless: the fan state is passed to the
> BIOS as fan | (speed << 8), so the higher bits end up in other parts of
> the request. Large values that do not wrap still cost one SMM call per
> state and fan while probing.
> 
> Fail probing if the maximum fan state does not fit in a byte, like the
> existing check for fan_mult.
> 
> Fixes: 81474fc2fae9 ("i8k: Add support for configurable maximum fan speed value")
> Signed-off-by: Rosen Penev <rosenp@gmail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794633179.git.rosenp@gmail.com?part=2

[PATCH 1/3] platform/x86: dell-laptop: Reject keyboard backlight timeouts that overflow

[Rosen Penev]

kbd_led_timeout_store() converts a timeout that is too large for its
unit to seconds with plain int multiplication. A large enough value
wraps, and if the wrapped number happens to be a valid timeout it is
accepted. On a Precision M4800, writing "4424214d" (over 12000 years)
to stop_timeout sets the timeout to 4 minutes, and "18989323d" sets it
to 2 minutes.

Convert with check_mul_overflow() and return -EINVAL on overflow.

Fixes: 02b2aaaa57ab ("platform: x86: dell-laptop: Add support for keyboard backlight")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/platform/x86/dell/dell-laptop.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
index 89e85c7f7132..bee347e1d60c 100644
--- a/drivers/platform/x86/dell/dell-laptop.c
+++ b/drivers/platform/x86/dell/dell-laptop.c
@@ -25,6 +25,7 @@
 #include <linux/sysfs.h>
 #include <linux/acpi.h>
 #include <linux/mm.h>
+#include <linux/overflow.h>
 #include <linux/i8042.h>
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
@@ -1554,6 +1555,7 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
 	struct kbd_state state;
 	bool convert;
 	int value;
+	int mult;
 	int ret;
 	char ch;
 	u8 unit;
@@ -1602,16 +1604,23 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
 		/* Convert value from current units to seconds */
 		switch (unit) {
 		case KBD_TIMEOUT_DAYS:
-			value *= 24;
-			fallthrough;
+			mult = 24 * 60 * 60;
+			break;
 		case KBD_TIMEOUT_HOURS:
-			value *= 60;
-			fallthrough;
+			mult = 60 * 60;
+			break;
 		case KBD_TIMEOUT_MINUTES:
-			value *= 60;
-			unit = KBD_TIMEOUT_SECONDS;
+			mult = 60;
+			break;
+		default:
+			mult = 1;
+			break;
 		}
 
+		if (check_mul_overflow(value, mult, &value))
+			return -EINVAL;
+		unit = KBD_TIMEOUT_SECONDS;
+
 		if (quirks && quirks->needs_kbd_timeouts) {
 			for (i = 0; quirks->kbd_timeouts[i] != -1; i++) {
 				if (value <= quirks->kbd_timeouts[i]) {
-- 
2.56.0

[PATCH 2/3] platform/x86: dell-laptop: Don't use an unread keyboard state at init

[Rosen Penev]

kbd_init_info() ignores the return value of kbd_get_state(). If the
call fails, the uninitialized state on the stack is used to set
kbd_previous_level and kbd_previous_mode_bit, which are later sent to
the BIOS when triggers are enabled or the backlight is turned back on.

Return the error instead. kbd_get_info() has already filled in
kbd_info at that point, so clear it as well: level control through
kbd_get_state() cannot work, and kbd_init() then still registers the
LED if at least two keyboard backlight tokens are available, which
works without it.

Fixes: 02b2aaaa57ab ("platform: x86: dell-laptop: Add support for keyboard backlight")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/platform/x86/dell/dell-laptop.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
index bee347e1d60c..171728d63c81 100644
--- a/drivers/platform/x86/dell/dell-laptop.c
+++ b/drivers/platform/x86/dell/dell-laptop.c
@@ -1455,7 +1455,12 @@ static inline int kbd_init_info(void)
 	    dell_smbios_find_token(KBD_LED_AC_TOKEN))
 		kbd_timeout_ac_supported = true;
 
-	kbd_get_state(&state);
+	ret = kbd_get_state(&state);
+	if (ret) {
+		/* Leave level control to the SMBIOS tokens, if there are any */
+		memset(&kbd_info, 0, sizeof(kbd_info));
+		return ret;
+	}
 
 	/* NOTE: timeout value is stored in 6 bits so max value is 63 */
 	if (kbd_info.seconds > 63)
-- 
2.56.0

[PATCH 3/3] platform/x86: dell-laptop: Parse the charge end threshold as an int

[Rosen Penev]

charge_control_end_threshold_store() passes a pointer to an int to
kstrtouint(), which only builds because the kernel disables
-Wpointer-sign, and makes the following "end < 0" check dead code.
Use kstrtoint() like charge_control_start_threshold_store(). Values
outside 0 to 100 are rejected either way.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/platform/x86/dell/dell-laptop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
index 171728d63c81..2ebfef8e5bbe 100644
--- a/drivers/platform/x86/dell/dell-laptop.c
+++ b/drivers/platform/x86/dell/dell-laptop.c
@@ -2385,7 +2385,7 @@ static ssize_t charge_control_end_threshold_store(struct device *dev,
 {
 	int ret, end;
 
-	ret = kstrtouint(buf, 10, &end);
+	ret = kstrtoint(buf, 10, &end);
 	if (ret)
 		return ret;
 	if (end < 0 || end > 100)
-- 
2.56.0