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

[sashiko-bot@kernel.org]

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