| Message ID | 20210115215752.389656-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | ACPI: scan: Make acpi_bus_get_device() clear the adev ptr on error | expand |
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 >
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 >> >
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.
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
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 >>
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) {
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(+)