On Mon, Nov 27, 2023 at 3:37 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> The acpi_video code was storing the acpi_video_device as driver-data
> in the acpi_device children of the acpi_video_bus acpi_device.
>
> But the acpi_video driver only binds to the bus acpi_device.
> It uses, but does not bind to, the children. Since it is not
> the driver it should not be using the driver_data of the children's
> acpi_device-s.
>
> Since commit 0d16710146a1 ("ACPI: bus: Set driver_data to NULL every
> time .add() fails") the childen's driver_data ends up getting set
> to NULL after a driver fails to bind to the children leading to a NULL
> pointer deref in video_get_max_state when registering the cooling-dev:
>
> [ 3.148958] BUG: kernel NULL pointer dereference, address: 0000000000000090
> <snip>
> [ 3.149015] Hardware name: Sony Corporation VPCSB2X9R/VAIO, BIOS R2087H4 06/15/2012
> [ 3.149021] RIP: 0010:video_get_max_state+0x17/0x30 [video]
> <snip>
> [ 3.149105] Call Trace:
> [ 3.149110] <TASK>
> [ 3.149114] ? __die+0x23/0x70
> [ 3.149126] ? page_fault_oops+0x171/0x4e0
> [ 3.149137] ? exc_page_fault+0x7f/0x180
> [ 3.149147] ? asm_exc_page_fault+0x26/0x30
> [ 3.149158] ? video_get_max_state+0x17/0x30 [video 9b6f3f0d19d7b4a0e2df17a2d8b43bc19c2ed71f]
> [ 3.149176] ? __pfx_video_get_max_state+0x10/0x10 [video 9b6f3f0d19d7b4a0e2df17a2d8b43bc19c2ed71f]
> [ 3.149192] __thermal_cooling_device_register.part.0+0xf2/0x2f0
> [ 3.149205] acpi_video_bus_register_backlight.part.0.isra.0+0x414/0x570 [video 9b6f3f0d19d7b4a0e2df17a2d8b43bc19c2ed71f]
> [ 3.149227] acpi_video_register_backlight+0x57/0x80 [video 9b6f3f0d19d7b4a0e2df17a2d8b43bc19c2ed71f]
> [ 3.149245] intel_acpi_video_register+0x68/0x90 [i915 1f3a758130b32ef13d301d4f8f78c7d766d57f2a]
> [ 3.149669] intel_display_driver_register+0x28/0x50 [i915 1f3a758130b32ef13d301d4f8f78c7d766d57f2a]
> [ 3.150064] i915_driver_probe+0x790/0xb90 [i915 1f3a758130b32ef13d301d4f8f78c7d766d57f2a]
> [ 3.150402] local_pci_probe+0x45/0xa0
> [ 3.150412] pci_device_probe+0xc1/0x260
> <snip>
>
> Fix this by directly using the acpi_video_device as devdata for
> the cooling-device, which avoids the need to set driver-data on
> the children at all.
>
> Fixes: 0d16710146a1 ("ACPI: bus: Set driver_data to NULL every time .add() fails")
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/9718
> Cc: Michal Wilczynski <michal.wilczynski@xxxxxxxxx>
> Cc: 6.6+ <stable@xxxxxxxxxxxxxxx> # 6.6+
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> drivers/acpi/acpi_video.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> index 5eded14f8853..7cd91e85c62a 100644
> --- a/drivers/acpi/acpi_video.c
> +++ b/drivers/acpi/acpi_video.c
> @@ -253,8 +253,7 @@ static const struct backlight_ops acpi_backlight_ops = {
> static int video_get_max_state(struct thermal_cooling_device *cooling_dev,
> unsigned long *state)
> {
> - struct acpi_device *device = cooling_dev->devdata;
> - struct acpi_video_device *video = acpi_driver_data(device);
> + struct acpi_video_device *video = cooling_dev->devdata;
>
> *state = video->brightness->count - ACPI_VIDEO_FIRST_LEVEL - 1;
> return 0;
> @@ -263,8 +262,7 @@ static int video_get_max_state(struct thermal_cooling_device *cooling_dev,
> static int video_get_cur_state(struct thermal_cooling_device *cooling_dev,
> unsigned long *state)
> {
> - struct acpi_device *device = cooling_dev->devdata;
> - struct acpi_video_device *video = acpi_driver_data(device);
> + struct acpi_video_device *video = cooling_dev->devdata;
> unsigned long long level;
> int offset;
>
> @@ -283,8 +281,7 @@ static int video_get_cur_state(struct thermal_cooling_device *cooling_dev,
> static int
> video_set_cur_state(struct thermal_cooling_device *cooling_dev, unsigned long state)
> {
> - struct acpi_device *device = cooling_dev->devdata;
> - struct acpi_video_device *video = acpi_driver_data(device);
> + struct acpi_video_device *video = cooling_dev->devdata;
> int level;
>
> if (state >= video->brightness->count - ACPI_VIDEO_FIRST_LEVEL)
> @@ -1125,7 +1122,6 @@ static int acpi_video_bus_get_one_device(struct acpi_device *device, void *arg)
>
> strcpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME);
> strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
> - device->driver_data = data;
>
> data->device_id = device_id;
> data->video = video;
> @@ -1747,8 +1743,8 @@ static void acpi_video_dev_register_backlight(struct acpi_video_device *device)
> device->backlight->props.brightness =
> acpi_video_get_brightness(device->backlight);
>
> - device->cooling_dev = thermal_cooling_device_register("LCD",
> - device->dev, &video_cooling_ops);
> + device->cooling_dev = thermal_cooling_device_register("LCD", device,
> + &video_cooling_ops);
> if (IS_ERR(device->cooling_dev)) {
> /*
> * Set cooling_dev to NULL so we don't crash trying to free it.
> --
Applied, thanks!
