drivers/hwmon/lenovo-ec-sensors.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
This series fixes several bugs in the probe() error handling path of
the lenovo-ec-sensors driver found during code review.
Patch 1 fixes a logic error in the EC signature check where && was
used instead of ||, causing the signature verification to be
effectively bypassed.
Patch 2 adds a missing NULL pointer check for dmi_first_match(),
which can return NULL on unsupported platforms.
Patch 3 converts manual request_region()/release_region() to the
devm-managed variant, fixing a double-release and a resource leak
in the probe error paths.
Kean (3):
hwmon: lenovo-ec-sensors: Fix EC signature check logic in probe
hwmon: lenovo-ec-sensors: Fix NULL pointer dereference when DMI match
fails
hwmon: lenovo-ec-sensors: Use devm_request_region for automatic
cleanup
drivers/hwmon/lenovo-ec-sensors.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
--
2.47.3
Hi Guenter,
Thank you for your patient review.
This is v3 of the lenovo-ec-sensors fix series.
Changes from v2:
- Corrected bug description and Remove unnecessary {}.
- Reproduce patch 0002 based 0001 changed.
Patch 1 converts manual request_region()/release_region() calls to
devm_request_region(), eliminating a double-release on the probe error
path and a release-after-use window on module exit.
Patch 2 fixes the EC "MCHP" signature check that used && instead of ||,
which caused the validation to accept a non-Microchip EC if any single
byte of the expected 4-byte signature happened to match.
Both patches apply cleanly against v7.1-rc3 and pass checkpatch with
0 errors, 0 warnings.
Kean Ren (2):
hwmon: (lenovo-ec-sensors): Convert to devm_request_region()
hwmon: (lenovo-ec-sensors): Fix EC "MCHP" signature validation logic
drivers/hwmon/lenovo-ec-sensors.c | 20 +++++++-------------
1 file changed, 7 insertions(+), 13 deletions(-)
--
2.53.0
Replace manual request_region()/release_region() with
devm_request_region(). This lets the device-managed framework
handle I/O region lifetime automatically and fixes:
- A double release_region() when probe fails after acquiring the
I/O region: the probe error path releases it, and then
lenovo_ec_init() releases it again on the same error path.
- A release-after-use window in lenovo_ec_exit() where
release_region() was called before platform_device_unregister(),
leaving the hwmon device active with a released I/O region.
- Missing release_region() in lenovo_ec_probe() if
devm_hwmon_device_register_with_info() fails.
Remove all four manual release_region() calls that are now handled
automatically and replace request_region with
devm_request_region, use dev_err replace pr_err.
Also remove the now-unnecessary braces around the single-statement
if body.
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Kean Ren <rh_king@163.com>
---
Changes in v3: Corrected bug description.
drivers/hwmon/lenovo-ec-sensors.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
index 8681bbf6665b..a16cc5e4053a 100644
--- a/drivers/hwmon/lenovo-ec-sensors.c
+++ b/drivers/hwmon/lenovo-ec-sensors.c
@@ -519,8 +519,8 @@ static int lenovo_ec_probe(struct platform_device *pdev)
if (!ec_data)
return -ENOMEM;
- if (!request_region(IO_REGION_START, IO_REGION_LENGTH, "LNV-WKS")) {
- pr_err(":request fail\n");
+ if (!devm_request_region(dev, IO_REGION_START, IO_REGION_LENGTH, "LNV-WKS")) {
+ dev_err(dev, "Failed to request I/O region\n");
return -EIO;
}
@@ -540,10 +540,8 @@ static int lenovo_ec_probe(struct platform_device *pdev)
if ((inb_p(MCHP_EMI0_EC_DATA_BYTE0) != 'M') &&
(inb_p(MCHP_EMI0_EC_DATA_BYTE1) != 'C') &&
(inb_p(MCHP_EMI0_EC_DATA_BYTE2) != 'H') &&
- (inb_p(MCHP_EMI0_EC_DATA_BYTE3) != 'P')) {
- release_region(IO_REGION_START, IO_REGION_LENGTH);
+ (inb_p(MCHP_EMI0_EC_DATA_BYTE3) != 'P'))
return -ENODEV;
- }
dmi_id = dmi_first_match(thinkstation_dmi_table);
@@ -577,7 +575,6 @@ static int lenovo_ec_probe(struct platform_device *pdev)
lenovo_ec_chip_info.info = lenovo_ec_hwmon_info_p8;
break;
default:
- release_region(IO_REGION_START, IO_REGION_LENGTH);
return -ENODEV;
}
@@ -606,10 +603,8 @@ static int __init lenovo_ec_init(void)
platform_create_bundle(&lenovo_ec_sensors_platform_driver,
lenovo_ec_probe, NULL, 0, NULL, 0);
- if (IS_ERR(lenovo_ec_sensors_platform_device)) {
- release_region(IO_REGION_START, IO_REGION_LENGTH);
+ if (IS_ERR(lenovo_ec_sensors_platform_device))
return PTR_ERR(lenovo_ec_sensors_platform_device);
- }
return 0;
}
@@ -617,7 +612,6 @@ module_init(lenovo_ec_init);
static void __exit lenovo_ec_exit(void)
{
- release_region(IO_REGION_START, IO_REGION_LENGTH);
platform_device_unregister(lenovo_ec_sensors_platform_device);
platform_driver_unregister(&lenovo_ec_sensors_platform_driver);
}
--
2.53.0On Thu, May 21, 2026 at 11:52:27AM +0800, Kean Ren wrote: > Replace manual request_region()/release_region() with > devm_request_region(). This lets the device-managed framework > handle I/O region lifetime automatically and fixes: > > - A double release_region() when probe fails after acquiring the > I/O region: the probe error path releases it, and then > lenovo_ec_init() releases it again on the same error path. > > - A release-after-use window in lenovo_ec_exit() where > release_region() was called before platform_device_unregister(), > leaving the hwmon device active with a released I/O region. > > - Missing release_region() in lenovo_ec_probe() if > devm_hwmon_device_register_with_info() fails. > > Remove all four manual release_region() calls that are now handled > automatically and replace request_region with > devm_request_region, use dev_err replace pr_err. > > Also remove the now-unnecessary braces around the single-statement > if body. > > Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca> > Signed-off-by: Kean Ren <rh_king@163.com> Applied. Thanks, Guenter
The EC signature check uses && instead of || between the four
byte comparisons. With &&, the condition is true only when ALL
four bytes fail to match simultaneously, meaning the driver
accepts a device as a valid Microchip EC if ANY single byte of
the 4-byte "MCHP" signature happens to match.
Due to short-circuit evaluation, if the first byte reads back as
'M' (0x4D, a very common register value), the remaining three
comparisons are skipped entirely and the device is accepted.
Change && to || so the check rejects devices that do not fully
match the expected EC signature, as originally intended.
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Kean Ren <rh_king@163.com>
---
Changes in v3: Corrected bug description and regenerated.
drivers/hwmon/lenovo-ec-sensors.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
index a16cc5e4053a..24a182abf9a3 100644
--- a/drivers/hwmon/lenovo-ec-sensors.c
+++ b/drivers/hwmon/lenovo-ec-sensors.c
@@ -537,9 +537,9 @@ static int lenovo_ec_probe(struct platform_device *pdev)
outw_p(MCHP_SING_IDX, MCHP_EMI0_EC_ADDRESS);
mutex_unlock(&ec_data->mec_mutex);
- if ((inb_p(MCHP_EMI0_EC_DATA_BYTE0) != 'M') &&
- (inb_p(MCHP_EMI0_EC_DATA_BYTE1) != 'C') &&
- (inb_p(MCHP_EMI0_EC_DATA_BYTE2) != 'H') &&
+ if ((inb_p(MCHP_EMI0_EC_DATA_BYTE0) != 'M') ||
+ (inb_p(MCHP_EMI0_EC_DATA_BYTE1) != 'C') ||
+ (inb_p(MCHP_EMI0_EC_DATA_BYTE2) != 'H') ||
(inb_p(MCHP_EMI0_EC_DATA_BYTE3) != 'P'))
return -ENODEV;
--
2.53.0On Thu, May 21, 2026 at 11:52:28AM +0800, Kean Ren wrote: > The EC signature check uses && instead of || between the four > byte comparisons. With &&, the condition is true only when ALL > four bytes fail to match simultaneously, meaning the driver > accepts a device as a valid Microchip EC if ANY single byte of > the 4-byte "MCHP" signature happens to match. > > Due to short-circuit evaluation, if the first byte reads back as > 'M' (0x4D, a very common register value), the remaining three > comparisons are skipped entirely and the device is accepted. > > Change && to || so the check rejects devices that do not fully > match the expected EC signature, as originally intended. > > Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca> > Signed-off-by: Kean Ren <rh_king@163.com> Applied. Thanks, Guenter
Hi Guenter, Thanks for your patient review and approval. Kean At 2026-05-21 21:46:00, "Guenter Roeck" <linux@roeck-us.net> wrote: >On Thu, May 21, 2026 at 11:52:28AM +0800, Kean Ren wrote: >> The EC signature check uses && instead of || between the four >> byte comparisons. With &&, the condition is true only when ALL >> four bytes fail to match simultaneously, meaning the driver >> accepts a device as a valid Microchip EC if ANY single byte of >> the 4-byte "MCHP" signature happens to match. >> >> Due to short-circuit evaluation, if the first byte reads back as >> 'M' (0x4D, a very common register value), the remaining three >> comparisons are skipped entirely and the device is accepted. >> >> Change && to || so the check rejects devices that do not fully >> match the expected EC signature, as originally intended. >> >> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca> >> Signed-off-by: Kean Ren <rh_king@163.com> > >Applied. > >Thanks, >Guenter
Hi Guenter, This is v2 of the lenovo-ec-sensors fix series. The original v1 [1] included a combined patch for both the EC signature fix and the I/O resource conversion, along with other driver fixes. Based on your feedback, this v2 makes the following changes: - Dropped the "Fix NULL pointer dereference" patch (previously patch 2). - Keep the original case of default case, remove the dev_err. Patch 1 converts manual request_region()/release_region() calls to devm_request_region(), eliminating a double-release on the probe error path and a release-after-use window on module exit. Patch 2 fixes the EC "MCHP" signature check that used && instead of ||, which caused the validation to accept a non-Microchip EC if any single byte of the expected 4-byte signature happened to match. Both patches apply cleanly against v7.1-rc3 and pass checkpatch with 0 errors, 0 warnings. [1] https://sashiko.dev/#/patchset/20260514011411.4167069-1-rh_king@163.com Kean (2): hwmon: (lenovo-ec-sensors): Convert to devm_request_region() hwmon: (lenovo-ec-sensors): Fix EC "MCHP" signature validation logic drivers/hwmon/lenovo-ec-sensors.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) -- 2.53.0
Replace manual request_region()/release_region() with
devm_request_region(). This lets the device-managed framework
handle I/O region lifetime automatically and fixes:
- A double release_region() when probe fails after acquiring the
I/O region: the probe error path releases it, and then
lenovo_ec_init() releases it again on the same error path.
- A release-after-use window in lenovo_ec_exit() where
release_region() was called before platform_device_unregister(),
leaving the hwmon device active with a released I/O region.
- Missing release_region() in lenovo_ec_probe() if
devm_hwmon_device_register_with_info() fails.
Remove all five manual release_region() calls that are now handled
automatically, and drop the unnecessary braces on the single-statement
blocks that previously contained them.
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Kean Ren <rh_king@163.com>
---
Changes in v2: Removed unnecessary error messages
drivers/hwmon/lenovo-ec-sensors.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
index 8681bbf6665b..45db49e189d3 100644
--- a/drivers/hwmon/lenovo-ec-sensors.c
+++ b/drivers/hwmon/lenovo-ec-sensors.c
@@ -519,8 +519,8 @@ static int lenovo_ec_probe(struct platform_device *pdev)
if (!ec_data)
return -ENOMEM;
- if (!request_region(IO_REGION_START, IO_REGION_LENGTH, "LNV-WKS")) {
- pr_err(":request fail\n");
+ if (!devm_request_region(dev, IO_REGION_START, IO_REGION_LENGTH, "LNV-WKS")) {
+ dev_err(dev, "Failed to request I/O region\n");
return -EIO;
}
@@ -541,7 +541,6 @@ static int lenovo_ec_probe(struct platform_device *pdev)
(inb_p(MCHP_EMI0_EC_DATA_BYTE1) != 'C') &&
(inb_p(MCHP_EMI0_EC_DATA_BYTE2) != 'H') &&
(inb_p(MCHP_EMI0_EC_DATA_BYTE3) != 'P')) {
- release_region(IO_REGION_START, IO_REGION_LENGTH);
return -ENODEV;
}
@@ -577,7 +576,6 @@ static int lenovo_ec_probe(struct platform_device *pdev)
lenovo_ec_chip_info.info = lenovo_ec_hwmon_info_p8;
break;
default:
- release_region(IO_REGION_START, IO_REGION_LENGTH);
return -ENODEV;
}
@@ -607,7 +605,6 @@ static int __init lenovo_ec_init(void)
lenovo_ec_probe, NULL, 0, NULL, 0);
if (IS_ERR(lenovo_ec_sensors_platform_device)) {
- release_region(IO_REGION_START, IO_REGION_LENGTH);
return PTR_ERR(lenovo_ec_sensors_platform_device);
}
@@ -617,7 +614,6 @@ module_init(lenovo_ec_init);
static void __exit lenovo_ec_exit(void)
{
- release_region(IO_REGION_START, IO_REGION_LENGTH);
platform_device_unregister(lenovo_ec_sensors_platform_device);
platform_driver_unregister(&lenovo_ec_sensors_platform_driver);
}
--
2.53.0© 2016 - 2026 Red Hat, Inc.