Re: [PATCH] soundwire: intel: fix possible crash when no device is detected (was Re: Crash in acpi_ns_validate_handle triggered by soundwire on Linux 5.10)

Pierre-Louis Bossart
2021-02-05T17:28:28Z

Thanks Marcin for the patch, much appreciated.

> acpi_walk_namespace can return success without executing our
> callback which initializes info->handle.
> If the random value in this structure is a valid address (which
> is on the stack, so it's quite possible), then nothing bad will
> happen, because:
> sdw_intel_scan_controller
> -> acpi_bus_get_device
> -> acpi_get_device_data
> -> acpi_get_data_full
> -> acpi_ns_validate_handle
> will reject this handle.
>
> However, if the value from the stack doesn't point to a valid
> address, we get this:
>
> BUG: kernel NULL pointer dereference, address: 0000000000000050

[...]

> diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
> index cabdadb09a1b..bc8520eb385e 100644
> --- a/drivers/soundwire/intel_init.c
> +++ b/drivers/soundwire/intel_init.c
> @@ -405,11 +405,12 @@ int sdw_intel_acpi_scan(acpi_handle *parent_handle,
> {
> acpi_status status;
>
> + info->handle = NULL;
> status = acpi_walk_namespace(ACPI_TYPE_DEVICE,
> parent_handle, 1,
> sdw_intel_acpi_cb,
> NULL, info, NULL);
> - if (ACPI_FAILURE(status))
> + if (ACPI_FAILURE(status) || info->handle == NULL)
> return -ENODEV;
>
> return sdw_intel_scan_controller(info);

It does seem like a required code pattern if I look at I2C and SPI. I
had no idea. Maybe worth documenting?

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