diff mbox series

[1/1] ACPI: video: Use acpi_video_device for cooling-dev driver data

Message ID 20231127143741.5229-2-hdegoede@redhat.com (mailing list archive)
State Mainlined, archived
Headers show
Series ACPI: video: Use acpi_video_device for cooling-dev driver data | expand

Commit Message

Hans de Goede Nov. 27, 2023, 2:37 p.m. UTC
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@intel.com>
Cc: 6.6+ <stable@vger.kernel.org> # 6.6+
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpi_video.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

Comments

Rafael J. Wysocki Nov. 27, 2023, 8:31 p.m. UTC | #1
On Mon, Nov 27, 2023 at 3:37 PM Hans de Goede <hdegoede@redhat.com> 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@intel.com>
> Cc: 6.6+ <stable@vger.kernel.org> # 6.6+
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  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!
diff mbox series

Patch

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.