Re: [PATCH 2/3] hwmon: lenovo-ec-sensors: Fix NULL pointer dereference when DMI match fails

Re: [PATCH 2/3] hwmon: lenovo-ec-sensors: Fix NULL pointer dereference when DMI match fails

[Kean Ren]

Hi Guenter,

Thank you for the review!

>> 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.
>
> [Severity: Medium]
> Is this description accurate? The patch diff shows that only four
> release_region() calls were removed.
>
>>As far as I can see there are only four calls in the code. Description
>>problem ?
Yes, you are right, thanks for your point out, I will update it.

>> @@ -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;
>>   	}
>
> [Severity: Medium]
> This isn't a bug, but could these curly braces be removed? The commit
> message mentions dropping the unnecessary braces on the single-statement
> blocks, but they appear to have been left intact here.
>
>>Hmm, yes, the description does not match the code changes. Please drop
>>the now unnecessary {}.
Thanks, you are right, it is unnecessary {}, I have removed it in the V2 2/2,
Please review it. Anyway, I will update the next version V3 that will 
include all the modify which kindly pointed out from your review.

Thanks,
Kean

Re: [PATCH 2/3] hwmon: lenovo-ec-sensors: Fix NULL pointer dereference when DMI match fails

[Guenter Roeck]

On 5/19/26 22:52, Kean Ren wrote:
> Hi Guenter,
> 
> Thank you for the review!
> 
>>> 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.
>>
>> [Severity: Medium]
>> Is this description accurate? The patch diff shows that only four
>> release_region() calls were removed.
>>
>>> As far as I can see there are only four calls in the code. Description
>>> problem ?
> Yes, you are right, thanks for your point out, I will update it.
> 
>>> @@ -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;
>>>    	}
>>
>> [Severity: Medium]
>> This isn't a bug, but could these curly braces be removed? The commit
>> message mentions dropping the unnecessary braces on the single-statement
>> blocks, but they appear to have been left intact here.
>>
>>> Hmm, yes, the description does not match the code changes. Please drop
>>> the now unnecessary {}.
> Thanks, you are right, it is unnecessary {}, I have removed it in the V2 2/2,

I have seen that, but that patch only removes one of the now unnecessary {},
and they should be removed when they were made unnecessary (i.e., in this
patch).

Thanks,
Guenter

Re: [PATCH 2/3] hwmon: lenovo-ec-sensors: Fix NULL pointer dereference when DMI match fails

[Guenter Roeck]

On 5/15/26 01:10, Kean wrote:
> Hi Guenter,
> Thank you for the review and for pointing this out!
> 
> You're absolutely right. I realize now that my patch was overly
> cautious — in normal operation dmi_first_match() can never return
> NULL here because lenovo_ec_init() already guards the probe behind:
> 
> 	static int __init lenovo_ec_init(void)
> 	{
> 		if (!dmi_check_system(thinkstation_dmi_table))
> 			return -ENODEV;
> 		...
> 	}
> 
> That said, I tend to follow a defensive programming style — checking
> for errors and returning early whenever something looks even slightly
> unexpected. This is exactly what lenovo_ec_init() itself does with
> dmi_check_system(), and it's also why we often put a return (or break)
> in the default branch of a switch statement. So I added the NULL check
> for dmi_first_match() as an extra sanity guard, even though logically
> it should never trigger.
> 

Maybe other subsystems accept that nowadays. Historically it was considered
waste. I still consider it waste, and I won't accept it.

Guenter

> I should have made this clearer in the commit message. The patch was
> meant as a defensive sanity check, but my description made it sound
> like an actual reachable bug, which it isn't. That's my mistake.
> 
> I'm happy to drop this patch from the series if you'd prefer. Please
> let me know how you'd like me to proceed.
> 
> For other parts and the format issues is my mistake that missed the
> --strict to check the patches file, I will send the V2 version, hope
>   get your review, any problem you can tell me, I will feedback and
> tested as your requested.
> 
> Thanks,
> Kean
> 
> 

Re: [PATCH 2/3] hwmon: lenovo-ec-sensors: Fix NULL pointer dereference when DMI match fails

[Kean]

Hi Guenter,

Just to follow up — if we drop patch 2, patches 1 and 3 remain
independent and should apply cleanly. They don't depend on the
NULL check in any way.

If you have any other concerns or requests for those two patches,
please let me know. I'll address them and send a v2 with just
those two for your review.

>>>    	default:
>>> -		release_region(IO_REGION_START, IO_REGION_LENGTH);
>>> +		dev_err(dev, "Unsupported platform type %ld\n",
>>> +			(long)dmi_id->driver_data);

This part I will remove as your comments is clear and will keep 
the orignal but 
release_region(IO_REGION_START, IO_REGION_LENGTH); still will
be remove for it works with other 2 patches.

Thanks,
Kean

Re: [PATCH 2/3] hwmon: lenovo-ec-sensors: Fix NULL pointer dereference when DMI match fails

[Kean]

Hi Guenter,
Thank you for the review and for pointing this out!

You're absolutely right. I realize now that my patch was overly
cautious — in normal operation dmi_first_match() can never return
NULL here because lenovo_ec_init() already guards the probe behind:

	static int __init lenovo_ec_init(void)
	{
		if (!dmi_check_system(thinkstation_dmi_table))
			return -ENODEV;
		...
	}

That said, I tend to follow a defensive programming style — checking
for errors and returning early whenever something looks even slightly
unexpected. This is exactly what lenovo_ec_init() itself does with
dmi_check_system(), and it's also why we often put a return (or break)
in the default branch of a switch statement. So I added the NULL check
for dmi_first_match() as an extra sanity guard, even though logically
it should never trigger.

I should have made this clearer in the commit message. The patch was
meant as a defensive sanity check, but my description made it sound
like an actual reachable bug, which it isn't. That's my mistake.

I'm happy to drop this patch from the series if you'd prefer. Please
let me know how you'd like me to proceed.

For other parts and the format issues is my mistake that missed the 
--strict to check the patches file, I will send the V2 version, hope
 get your review, any problem you can tell me, I will feedback and 
tested as your requested.

Thanks,
Kean

Re: [PATCH 2/3] hwmon: lenovo-ec-sensors: Fix NULL pointer dereference when DMI match fails

[Guenter Roeck]

On Thu, May 14, 2026 at 09:14:10AM +0800, Kean wrote:
> dmi_first_match() returns NULL if the running system does not match any
> entry in thinkstation_dmi_table. Without a NULL check, the subsequent
> dmi_id->driver_data access dereferences a NULL pointer, causing a kernel
> oops or panic.
> 
> Add a NULL check and return -ENODEV to gracefully fail the probe when
> the driver is loaded on an unsupported platform.
> 
> Signed-off-by: Kean <rh_king@163.com>
> 
> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>

ERROR: trailing whitespace
#104: FILE: drivers/hwmon/lenovo-ec-sensors.c:540:
+^Iif ((inb_p(MCHP_EMI0_EC_DATA_BYTE0) != 'M') || $

ERROR: trailing whitespace
#105: FILE: drivers/hwmon/lenovo-ec-sensors.c:541:
+^I    (inb_p(MCHP_EMI0_EC_DATA_BYTE1) != 'C') || $

total: 2 errors, 0 warnings, 0 checks, 12 lines checked

> ---
>  drivers/hwmon/lenovo-ec-sensors.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
> index a32b1f2c6a3a..b0f2a04ce679 100644
> --- a/drivers/hwmon/lenovo-ec-sensors.c
> +++ b/drivers/hwmon/lenovo-ec-sensors.c
> @@ -546,6 +546,8 @@ static int lenovo_ec_probe(struct platform_device *pdev)
>  	}
>  
>  	dmi_id = dmi_first_match(thinkstation_dmi_table);
> +	if (!dmi_id)
> +		return -ENODEV;
>  
>  	switch ((long)dmi_id->driver_data) {
>  	case 0:

Re: [PATCH 2/3] hwmon: lenovo-ec-sensors: Fix NULL pointer dereference when DMI match fails

[Guenter Roeck]

On 5/13/26 18:14, Kean wrote:
> dmi_first_match() returns NULL if the running system does not match any
> entry in thinkstation_dmi_table. Without a NULL check, the subsequent
> dmi_id->driver_data access dereferences a NULL pointer, causing a kernel
> oops or panic.
> 
> Add a NULL check and return -ENODEV to gracefully fail the probe when
> the driver is loaded on an unsupported platform.
> 

How would that happen in practice ? The driver init code has

         if (!dmi_check_system(thinkstation_dmi_table))
                 return -ENODEV;

Please provide a reproducer.

Thanks,
Guenter

[PATCH 2/3] hwmon: lenovo-ec-sensors: Fix NULL pointer dereference when DMI match fails

[Kean]

dmi_first_match() returns NULL if the running system does not match any
entry in thinkstation_dmi_table. Without a NULL check, the subsequent
dmi_id->driver_data access dereferences a NULL pointer, causing a kernel
oops or panic.

Add a NULL check and return -ENODEV to gracefully fail the probe when
the driver is loaded on an unsupported platform.

Signed-off-by: Kean <rh_king@163.com>

Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
 drivers/hwmon/lenovo-ec-sensors.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
index a32b1f2c6a3a..b0f2a04ce679 100644
--- a/drivers/hwmon/lenovo-ec-sensors.c
+++ b/drivers/hwmon/lenovo-ec-sensors.c
@@ -546,6 +546,8 @@ static int lenovo_ec_probe(struct platform_device *pdev)
 	}
 
 	dmi_id = dmi_first_match(thinkstation_dmi_table);
+	if (!dmi_id)
+		return -ENODEV;
 
 	switch ((long)dmi_id->driver_data) {
 	case 0:
-- 
2.47.3