| Message ID | 20231127143741.5229-2-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Commit | 172c48caed91a978bca078042222d09baea13717 |
| Headers | show |
| Series | ACPI: video: Use acpi_video_device for cooling-dev driver data | expand |
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 --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.
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(-)