[PATCH] ACPI: scan: Make acpi_bus_get_device() clear the adev ptr on error

[PATCH] ACPI: scan: Make acpi_bus_get_device() clear the adev ptr on error

[Hans de Goede]

Set the acpi_device pointer which acpi_bus_get_device()
returns-by-reference to NULL on error.

We've recently had 2 cases where callers of acpi_bus_get_device()
did not properly error check the return value, using the
returned-by-reference acpi_device pointer blindly, set it to NULL
so that this will lead to an immediate oops, rather then following
a pointer to who knows what.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/scan.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index c575c9b67f76..c53e88deee1d 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -585,6 +585,8 @@ static int acpi_get_device_data(acpi_handle handle, struct acpi_device **device,
 	if (!device)
 		return -EINVAL;
 
+	*device = NULL;
+
 	status = acpi_get_data_full(handle, acpi_scan_drop_device,
 				    (void **)device, callback);
 	if (ACPI_FAILURE(status) || !*device) {
-- 
2.28.0

Re: [PATCH] ACPI: scan: Make acpi_bus_get_device() clear the adev ptr on error

[Rafael J. Wysocki]

On Fri, Jan 15, 2021 at 10:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Set the acpi_device pointer which acpi_bus_get_device()
> returns-by-reference to NULL on error.
>
> We've recently had 2 cases where callers of acpi_bus_get_device()
> did not properly error check the return value, using the
> returned-by-reference acpi_device pointer blindly, set it to NULL
> so that this will lead to an immediate oops, rather then following
> a pointer to who knows what.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

This should fix the crash reported by Pierre-Louis, so let me apply it
instead of the two debug changes posted by me
(https://lore.kernel.org/linux-acpi/98e6ed8e-884e-adb4-a146-a66daefa94a7@redhat.com/T/#md5add2fe554a30e3a929d87a66b435f4cc8bf628).

Pierre-Louis, can you please double check that the issue goes away
with this patch applied?

> ---
>  drivers/acpi/scan.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index c575c9b67f76..c53e88deee1d 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -585,6 +585,8 @@ static int acpi_get_device_data(acpi_handle handle, struct acpi_device **device,
>         if (!device)
>                 return -EINVAL;
>
> +       *device = NULL;
> +
>         status = acpi_get_data_full(handle, acpi_scan_drop_device,
>                                     (void **)device, callback);
>         if (ACPI_FAILURE(status) || !*device) {
> --
> 2.28.0
>

Re: [PATCH] ACPI: scan: Make acpi_bus_get_device() clear the adev ptr on error

[Hans de Goede]

Hi,

On 1/18/21 2:58 PM, Rafael J. Wysocki wrote:
> On Fri, Jan 15, 2021 at 10:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Set the acpi_device pointer which acpi_bus_get_device()
>> returns-by-reference to NULL on error.
>>
>> We've recently had 2 cases where callers of acpi_bus_get_device()
>> did not properly error check the return value, using the
>> returned-by-reference acpi_device pointer blindly, set it to NULL
>> so that this will lead to an immediate oops, rather then following
>> a pointer to who knows what.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> This should fix the crash reported by Pierre-Louis,

Ack, sounds good.

> so let me apply it
> instead of the two debug changes posted by me
> (https://lore.kernel.org/linux-acpi/98e6ed8e-884e-adb4-a146-a66daefa94a7@redhat.com/T/#md5add2fe554a30e3a929d87a66b435f4cc8bf628).

Note we should still fix the USB case, my patch will make failure
there more obvious, but the code can theoretically still dereference
a NULL pointer in drivers/usb/core/usb-acpi.c.

And we probably also want this change:

--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1867,7 +1867,8 @@ static u32 acpi_scan_check_dep(acpi_hand
 	 * 2. ACPI nodes describing USB ports.
 	 * Still, checking for _HID catches more then just these cases ...
 	 */
-	if (!acpi_has_method(handle, "_DEP") || !acpi_has_method(handle, "_HID"))
+	if (!acpi_has_method(handle, "_DEP") || acpi_has_method(handle, "_ADR")
+	    || !acpi_has_method(handle, "_HID"))
 		return 0;
 
 	status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_devices);

To reduce the amount of work we do checking _DEP-s.

If you want I can submit patches for both ?

Regards,

Hans




> 
> Pierre-Louis, can you please double check that the issue goes away
> with this patch applied?
> 
>> ---
>>  drivers/acpi/scan.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index c575c9b67f76..c53e88deee1d 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -585,6 +585,8 @@ static int acpi_get_device_data(acpi_handle handle, struct acpi_device **device,
>>         if (!device)
>>                 return -EINVAL;
>>
>> +       *device = NULL;
>> +
>>         status = acpi_get_data_full(handle, acpi_scan_drop_device,
>>                                     (void **)device, callback);
>>         if (ACPI_FAILURE(status) || !*device) {
>> --
>> 2.28.0
>>
> 

Re: [PATCH] ACPI: scan: Make acpi_bus_get_device() clear the adev ptr on error

[Rafael J. Wysocki]

On Mon, Jan 18, 2021 at 5:09 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 1/18/21 2:58 PM, Rafael J. Wysocki wrote:
> > On Fri, Jan 15, 2021 at 10:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Set the acpi_device pointer which acpi_bus_get_device()
> >> returns-by-reference to NULL on error.
> >>
> >> We've recently had 2 cases where callers of acpi_bus_get_device()
> >> did not properly error check the return value, using the
> >> returned-by-reference acpi_device pointer blindly, set it to NULL
> >> so that this will lead to an immediate oops, rather then following
> >> a pointer to who knows what.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >
> > This should fix the crash reported by Pierre-Louis,
>
> Ack, sounds good.
>
> > so let me apply it
> > instead of the two debug changes posted by me
> > (https://lore.kernel.org/linux-acpi/98e6ed8e-884e-adb4-a146-a66daefa94a7@redhat.com/T/#md5add2fe554a30e3a929d87a66b435f4cc8bf628).
>
> Note we should still fix the USB case, my patch will make failure
> there more obvious, but the code can theoretically still dereference
> a NULL pointer in drivers/usb/core/usb-acpi.c.

Because usb_acpi_find_port() checks the acpi_device pointer passed to
it against NULL, we're safe here as well.

> And we probably also want this change:
>
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -1867,7 +1867,8 @@ static u32 acpi_scan_check_dep(acpi_hand
>          * 2. ACPI nodes describing USB ports.
>          * Still, checking for _HID catches more then just these cases ...
>          */
> -       if (!acpi_has_method(handle, "_DEP") || !acpi_has_method(handle, "_HID"))
> +       if (!acpi_has_method(handle, "_DEP") || acpi_has_method(handle, "_ADR")
> +           || !acpi_has_method(handle, "_HID"))
>                 return 0;
>
>         status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_devices);
>
> To reduce the amount of work we do checking _DEP-s.

So I was thinking about that, but I'd leave it as is unless we have a
use case in which looking for _ADR is really necessary.

In the majority of cases the device objects with both _ADR and _HID
really are _HID devices and _ADR returns 0.  Of course, that could be
treated as a special case, but unless it is necessary to add a check
for this special case, I'd rather avoid doing that.

Re: [PATCH] ACPI: scan: Make acpi_bus_get_device() clear the adev ptr on error

[Hans de Goede]

Hi,

On 1/18/21 5:21 PM, Rafael J. Wysocki wrote:
> On Mon, Jan 18, 2021 at 5:09 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 1/18/21 2:58 PM, Rafael J. Wysocki wrote:
>>> On Fri, Jan 15, 2021 at 10:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Set the acpi_device pointer which acpi_bus_get_device()
>>>> returns-by-reference to NULL on error.
>>>>
>>>> We've recently had 2 cases where callers of acpi_bus_get_device()
>>>> did not properly error check the return value, using the
>>>> returned-by-reference acpi_device pointer blindly, set it to NULL
>>>> so that this will lead to an immediate oops, rather then following
>>>> a pointer to who knows what.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>
>>> This should fix the crash reported by Pierre-Louis,
>>
>> Ack, sounds good.
>>
>>> so let me apply it
>>> instead of the two debug changes posted by me
>>> (https://lore.kernel.org/linux-acpi/98e6ed8e-884e-adb4-a146-a66daefa94a7@redhat.com/T/#md5add2fe554a30e3a929d87a66b435f4cc8bf628).
>>
>> Note we should still fix the USB case, my patch will make failure
>> there more obvious, but the code can theoretically still dereference
>> a NULL pointer in drivers/usb/core/usb-acpi.c.
> 
> Because usb_acpi_find_port() checks the acpi_device pointer passed to
> it against NULL, we're safe here as well.

Ah, good :)

>> And we probably also want this change:
>>
>> --- linux-pm.orig/drivers/acpi/scan.c
>> +++ linux-pm/drivers/acpi/scan.c
>> @@ -1867,7 +1867,8 @@ static u32 acpi_scan_check_dep(acpi_hand
>>          * 2. ACPI nodes describing USB ports.
>>          * Still, checking for _HID catches more then just these cases ...
>>          */
>> -       if (!acpi_has_method(handle, "_DEP") || !acpi_has_method(handle, "_HID"))
>> +       if (!acpi_has_method(handle, "_DEP") || acpi_has_method(handle, "_ADR")
>> +           || !acpi_has_method(handle, "_HID"))
>>                 return 0;
>>
>>         status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_devices);
>>
>> To reduce the amount of work we do checking _DEP-s.
> 
> So I was thinking about that, but I'd leave it as is unless we have a
> use case in which looking for _ADR is really necessary.
> 
> In the majority of cases the device objects with both _ADR and _HID
> really are _HID devices and _ADR returns 0.  Of course, that could be
> treated as a special case, but unless it is necessary to add a check
> for this special case, I'd rather avoid doing that.

Ok, that works for me.

Regards,

Hans

Re: [PATCH] ACPI: scan: Make acpi_bus_get_device() clear the adev ptr on error

[Pierre-Louis Bossart]

On 1/18/21 7:58 AM, Rafael J. Wysocki wrote:
> On Fri, Jan 15, 2021 at 10:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Set the acpi_device pointer which acpi_bus_get_device()
>> returns-by-reference to NULL on error.
>>
>> We've recently had 2 cases where callers of acpi_bus_get_device()
>> did not properly error check the return value, using the
>> returned-by-reference acpi_device pointer blindly, set it to NULL
>> so that this will lead to an immediate oops, rather then following
>> a pointer to who knows what.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> This should fix the crash reported by Pierre-Louis, so let me apply it
> instead of the two debug changes posted by me
> (https://lore.kernel.org/linux-acpi/98e6ed8e-884e-adb4-a146-a66daefa94a7@redhat.com/T/#md5add2fe554a30e3a929d87a66b435f4cc8bf628).
> 
> Pierre-Louis, can you please double check that the issue goes away
> with this patch applied?

yep, tested twice

Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Thanks Hans and Rafael for your work, much appreciated.

>> ---
>>   drivers/acpi/scan.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index c575c9b67f76..c53e88deee1d 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -585,6 +585,8 @@ static int acpi_get_device_data(acpi_handle handle, struct acpi_device **device,
>>          if (!device)
>>                  return -EINVAL;
>>
>> +       *device = NULL;
>> +
>>          status = acpi_get_data_full(handle, acpi_scan_drop_device,
>>                                      (void **)device, callback);
>>          if (ACPI_FAILURE(status) || !*device) {
>> --
>> 2.28.0
>>