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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> >>> >>> 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@xxxxxxxxxx/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
