platform/x86: wmi: Add Null check for device

[PATCH] platform/x86: wmi: Add Null check for device

Posted by Chenyuan Yang 9 months, 1 week ago

Not all devices have an ACPI companion fwnode, so device might be NULL.
This is similar to the commit cd2fd6eab480
("platform/x86: int3472: Check for adev == NULL").

Add a check for device not being set and return -ENODEV in that case to
avoid a possible NULL pointer deref in parse_wdg().

Note, acpi_wmi_probe() under the same file has such a check.

Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
---
 drivers/platform/x86/wmi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 646370bd6b03..54e697838c1e 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1091,6 +1091,9 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
 	u32 i, total;
 	int retval;
 
+	if (!device)
+		return -ENODEV;
+
 	status = acpi_evaluate_object(device->handle, "_WDG", NULL, &out);
 	if (ACPI_FAILURE(status))
 		return -ENXIO;
-- 
2.34.1

Re: [PATCH] platform/x86: wmi: Add Null check for device

Posted by Ilpo Järvinen 9 months, 1 week ago

On Thu, 13 Mar 2025, Chenyuan Yang wrote:

Hi,

Could you please be consistent in style and write "NULL" also in the 
shortlog in the subject.

> Not all devices have an ACPI companion fwnode, so device might be NULL.
> This is similar to the commit cd2fd6eab480
> ("platform/x86: int3472: Check for adev == NULL").

Please fold the paragraph normally.

> Add a check for device not being set and return -ENODEV in that case to
> avoid a possible NULL pointer deref in parse_wdg().
> 
> Note, acpi_wmi_probe() under the same file has such a check.

Hmm, is this a bogus fix, as parse_wdg() is only called from 
acpi_wmi_probe() so how can ACPI companion turn NULL in between??

How was this problem found??

> Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
> ---
>  drivers/platform/x86/wmi.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index 646370bd6b03..54e697838c1e 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -1091,6 +1091,9 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
>  	u32 i, total;
>  	int retval;
>  
> +	if (!device)
> +		return -ENODEV;
> +
>  	status = acpi_evaluate_object(device->handle, "_WDG", NULL, &out);
>  	if (ACPI_FAILURE(status))
>  		return -ENXIO;
> 

-- 
 i.

Re: [PATCH] platform/x86: wmi: Add Null check for device

Posted by Chenyuan Yang 9 months, 1 week ago

Hi Ilpo,

Thanks for pointing this out.
This was found by our static analyzer.
Sorry that the checker didn't make further reasoning.

-Chenyuan


On Fri, Mar 14, 2025 at 6:41 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Thu, 13 Mar 2025, Chenyuan Yang wrote:
>
> Hi,
>
> Could you please be consistent in style and write "NULL" also in the
> shortlog in the subject.
>
> > Not all devices have an ACPI companion fwnode, so device might be NULL.
> > This is similar to the commit cd2fd6eab480
> > ("platform/x86: int3472: Check for adev == NULL").
>
> Please fold the paragraph normally.
>
> > Add a check for device not being set and return -ENODEV in that case to
> > avoid a possible NULL pointer deref in parse_wdg().
> >
> > Note, acpi_wmi_probe() under the same file has such a check.
>
> Hmm, is this a bogus fix, as parse_wdg() is only called from
> acpi_wmi_probe() so how can ACPI companion turn NULL in between??
>
> How was this problem found??
>
> > Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
> > ---
> >  drivers/platform/x86/wmi.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> > index 646370bd6b03..54e697838c1e 100644
> > --- a/drivers/platform/x86/wmi.c
> > +++ b/drivers/platform/x86/wmi.c
> > @@ -1091,6 +1091,9 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
> >       u32 i, total;
> >       int retval;
> >
> > +     if (!device)
> > +             return -ENODEV;
> > +
> >       status = acpi_evaluate_object(device->handle, "_WDG", NULL, &out);
> >       if (ACPI_FAILURE(status))
> >               return -ENXIO;
> >
>
> --
>  i.
>

Re: [PATCH] platform/x86: wmi: Add Null check for device

Posted by Ilpo Järvinen 9 months ago

On Fri, 14 Mar 2025, Chenyuan Yang wrote:

> Hi Ilpo,
> 
> Thanks for pointing this out.
> This was found by our static analyzer.
> Sorry that the checker didn't make further reasoning.

Hi Chenyuan,

Then you should be the one who does that further reasoning before sending 
the patch out. :-) Please don't assume tools couldn't return also false 
positives. It's good to study all the code related to the lines and 
functions changed beyond just the patch context so you can understand 
whether the change makes sense and explain how the problem can manifest 
for real.

Please also name the tool in future in the changelog when problems are 
found by some code analysis tool (as is also required by the submission 
guidelines under Documentation/process/).


-- 
 i.


> On Fri, Mar 14, 2025 at 6:41 AM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Thu, 13 Mar 2025, Chenyuan Yang wrote:
> >
> > Hi,
> >
> > Could you please be consistent in style and write "NULL" also in the
> > shortlog in the subject.
> >
> > > Not all devices have an ACPI companion fwnode, so device might be NULL.
> > > This is similar to the commit cd2fd6eab480
> > > ("platform/x86: int3472: Check for adev == NULL").
> >
> > Please fold the paragraph normally.
> >
> > > Add a check for device not being set and return -ENODEV in that case to
> > > avoid a possible NULL pointer deref in parse_wdg().
> > >
> > > Note, acpi_wmi_probe() under the same file has such a check.
> >
> > Hmm, is this a bogus fix, as parse_wdg() is only called from
> > acpi_wmi_probe() so how can ACPI companion turn NULL in between??
> >
> > How was this problem found??
> >
> > > Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
> > > ---
> > >  drivers/platform/x86/wmi.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> > > index 646370bd6b03..54e697838c1e 100644
> > > --- a/drivers/platform/x86/wmi.c
> > > +++ b/drivers/platform/x86/wmi.c
> > > @@ -1091,6 +1091,9 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
> > >       u32 i, total;
> > >       int retval;
> > >
> > > +     if (!device)
> > > +             return -ENODEV;
> > > +
> > >       status = acpi_evaluate_object(device->handle, "_WDG", NULL, &out);
> > >       if (ACPI_FAILURE(status))
> > >               return -ENXIO;
> > >
> >
> > --
> >  i.
> >
>

Re: [PATCH] platform/x86: wmi: Add Null check for device

Posted by Armin Wolf 9 months ago

Am 14.03.25 um 17:29 schrieb Chenyuan Yang:

> Hi Ilpo,
>
> Thanks for pointing this out.
> This was found by our static analyzer.
> Sorry that the checker didn't make further reasoning.
>
> -Chenyuan

Please avoid sending patches based solely on warnings produced by static checkers. They often do not fully
understand the various side effects and semantics of individual parts of the code and can thus produce bogus
warnings. Please verify that a warning produced by a static checker is actually valid before sending a patch
next time.

Still i have to admit that the source code of the parse_wdg() function might benefit from some refactoring
in the far future.

Thanks,
Armin Wolf

>
> On Fri, Mar 14, 2025 at 6:41 AM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
>> On Thu, 13 Mar 2025, Chenyuan Yang wrote:
>>
>> Hi,
>>
>> Could you please be consistent in style and write "NULL" also in the
>> shortlog in the subject.
>>
>>> Not all devices have an ACPI companion fwnode, so device might be NULL.
>>> This is similar to the commit cd2fd6eab480
>>> ("platform/x86: int3472: Check for adev == NULL").
>> Please fold the paragraph normally.
>>
>>> Add a check for device not being set and return -ENODEV in that case to
>>> avoid a possible NULL pointer deref in parse_wdg().
>>>
>>> Note, acpi_wmi_probe() under the same file has such a check.
>> Hmm, is this a bogus fix, as parse_wdg() is only called from
>> acpi_wmi_probe() so how can ACPI companion turn NULL in between??
>>
>> How was this problem found??
>>
>>> Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
>>> ---
>>>   drivers/platform/x86/wmi.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
>>> index 646370bd6b03..54e697838c1e 100644
>>> --- a/drivers/platform/x86/wmi.c
>>> +++ b/drivers/platform/x86/wmi.c
>>> @@ -1091,6 +1091,9 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
>>>        u32 i, total;
>>>        int retval;
>>>
>>> +     if (!device)
>>> +             return -ENODEV;
>>> +
>>>        status = acpi_evaluate_object(device->handle, "_WDG", NULL, &out);
>>>        if (ACPI_FAILURE(status))
>>>                return -ENXIO;
>>>
>> --
>>   i.
>>