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

[Hans de Goede <hdegoede@redhat.com>]

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
>>
>