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