[PATCH] drm/amd/display: fix NULL deref when opening a closed DDC

[PATCH] drm/amd/display: fix NULL deref when opening a closed DDC

[Arthur Liberman]

dal_ddc_open() uses gpio->pin after dal_gpio_service_open() installs
it. Close clears that pointer. HPD sink detection and a KWin atomic
check can both open the same DDC: the check reads DPCD for DSC while
the HPD worker closes the pin. The load of hw_gpio.store.en then
oopses at offset 0x28 and kwin_wayland exits with IRQs disabled.

If either pin is gone after open, close what this call holds and
return GPIO_RESULT_NULL_HANDLE so the AUX transfer fails instead.
This does not serialize the two threads.

Cc: stable@vger.kernel.org
Assisted-by: Cursor:xai-grok-4.7
Signed-off-by: Arthur Liberman <arthur_liberman@hotmail.com>
---
 drivers/gpu/drm/amd/display/dc/gpio/gpio_service.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/dc/gpio/gpio_service.c b/drivers/gpu/drm/amd/display/dc/gpio/gpio_service.c
index 95f8b7c7d657..a3145cd82d8b 100644
--- a/drivers/gpu/drm/amd/display/dc/gpio/gpio_service.c
+++ b/drivers/gpu/drm/amd/display/dc/gpio/gpio_service.c
@@ -577,6 +577,17 @@ enum gpio_result dal_ddc_open(
 		goto failure;
 	}
 
+	/*
+	 * dal_gpio_service_open() installs gpio->pin, and close clears it.
+	 * MST bring-up can AUX-read for DSC while HPD teardown closes the
+	 * same DDC. pin is NULL again in that window; do not dereference it.
+	 */
+	if (!ddc->pin_data->pin || !ddc->pin_clock->pin) {
+		dal_gpio_close(ddc->pin_clock);
+		dal_gpio_close(ddc->pin_data);
+		return GPIO_RESULT_NULL_HANDLE;
+	}
+
 	/* DDC clock and data pins should belong
 	 * to the same DDC block id,
 	 * we use the data pin to set the pad mode. */

base-commit: 41505ac433cc4c5179deee8860d197b04f6d1c1d
-- 
2.55.0

Re: [PATCH] drm/amd/display: fix NULL deref when opening a closed DDC

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dc/gpio: TOCTOU race and double-close in dal_ddc_open lockless fix
--

commit ad94ac206173ba28053892dd31bd38232f256278
Author: Arthur Liberman <arthur_liberman@hotmail.com>

drm/amd/display: fix NULL deref when opening a closed DDC

The patch aims to fix a NULL pointer dereference in dal_ddc_open() by
adding a lockless check to see if either pin was closed by a concurrent
HPD teardown thread. It returns an error if a pin is missing rather
than trying to serialize the two threads.

> diff --git a/drivers/gpu/drm/amd/display/dc/gpio/gpio_service.c b/drivers/gpu/drm/amd/display/dc/gpio/gpio_service.c
> index 95f8b7c7d657a..a3145cd82d8bd 100644
> --- a/drivers/gpu/drm/amd/display/dc/gpio/gpio_service.c
> +++ b/drivers/gpu/drm/amd/display/dc/gpio/gpio_service.c
> @@ -577,6 +577,17 @@ enum gpio_result dal_ddc_open(
>  		goto failure;
>  	}
>  
> +	/*
> +	 * dal_gpio_service_open() installs gpio->pin, and close clears it.
> +	 * MST bring-up can AUX-read for DSC while HPD teardown closes the
> +	 * same DDC. pin is NULL again in that window; do not dereference it.
> +	 */
> +	if (!ddc->pin_data->pin || !ddc->pin_clock->pin) {
> +		dal_gpio_close(ddc->pin_clock);
> +		dal_gpio_close(ddc->pin_data);

[Severity: High]
Does this error path introduce a concurrent double-close?

If the check detects one pin is NULL while the other is not (because the HPD
thread is closing them sequentially), calling dal_gpio_close() on both
pins races with the HPD thread's own close operations.

Could this cause unsynchronized MMIO writes in the hardware teardown paths?

> +		return GPIO_RESULT_NULL_HANDLE;
> +	}
> +
>  	/* DDC clock and data pins should belong
>  	 * to the same DDC block id,
>  	 * we use the data pin to set the pad mode. */

[Severity: High]
Since this does not serialize the threads and the pointers are not cached
via READ_ONCE(), can the compiler re-read the pointers from memory further
down in dal_ddc_open()?

drivers/gpu/drm/amd/display/dc/gpio/gpio_service.c:dal_ddc_open() {
    ...
	hw_data = FROM_HW_GPIO_PIN(ddc->pin_data->pin);
	hw_clock = FROM_HW_GPIO_PIN(ddc->pin_clock->pin);

	config_data.config.ddc.data_en_bit_present = hw_data->store.en != 0;
    ...
}

If the HPD thread sets the pointer to NULL between the earlier check and this
read, hw_data would become NULL, and hw_data->store.en != 0 would oops
at offset 0x28, defeating the fix.

[Severity: High]
Does dal_gpio_set_config(), which is called right after, repeat the same
check-then-dereference pattern?

drivers/gpu/drm/amd/display/dc/gpio/gpio_base.c:dal_gpio_set_config() {
    ...
	if (!gpio->pin) {
		BREAK_TO_DEBUGGER();
		return GPIO_RESULT_NULL_HANDLE;
	}

	return gpio->pin->funcs->set_config(gpio->pin, config_data);
}

If the pin is cleared after the check but before the funcs dereference,
will this oops as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/DB9PR03MB73881C96FE86D964037ABEDB8A942@DB9PR03MB7388.eurprd03.prod.outlook.com?part=1