Re: [PATCH v2] drm/amd/display: Route connector DDC over AUX for native DP sinks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] drm/amd/display: UAF race on link->local_sink in amdgpu_dm_i2c_xfer()
- [High] drm/amd/display: NULL link dereference in amdgpu_dm_i2c_xfer()
- [Medium] drm/amd/display: amdgpu_dm_i2c_xfer() misses MST and force_to_use_aux links
--

commit 7a7dcab86c8948b21ebe53eebc9df866754fd915
Author: Mario Limonciello <superm1@kernel.org>

drm/amd/display: Route connector DDC over AUX for native DP sinks

This commit updates amdgpu_dm_i2c_xfer() to route I2C transfers over the
DisplayPort AUX DDC adapter when a native DisplayPort, eDP, or MST sink is
detected on the link, while keeping the hardware I2C engine for DVI/HDMI
sinks.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
> index b498fc5fbb140..678caffa3d120 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
> @@ -3373,6 +3373,22 @@ STATIC_IFN_KUNIT int amdgpu_dm_i2c_xfer(struct i2c_adapter *i2c_adap,
>  	int i;
>  	int result = -EIO;
>  
> +	if (!i2c->oem) {
> +		struct dc_link *link = ddc_service->link;
> +		struct amdgpu_dm_connector *aconnector = link->priv;

[Severity: High]
Can ddc_service->link be NULL here?

Previously, amdgpu_dm_i2c_xfer() checked ddc_service->ddc_pin before
dereferencing ddc_service->link. Moving this block ahead of the
ddc_service->ddc_pin check without verifying that link is non-NULL causes a
NULL pointer dereference in the existing KUnit test
dm_test_i2c_xfer_no_ddc_pin():

drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c:dm_test_i2c_xfer_no_ddc_pin() {
	...
	i2c->ddc_service = ddc;
	i2c_set_adapdata(&i2c->base, i2c);

	/* ddc->ddc_pin is NULL -> transfer is rejected with -EIO. */
	KUNIT_EXPECT_EQ(test, amdgpu_dm_i2c_xfer(&i2c->base, NULL, 0), -EIO);
}

In that test, i2c->oem is false and ddc->link is NULL, so evaluating
link->priv crashes immediately.

> +
> +		/*
> +		 * DDC to a native DisplayPort sink is tunneled over the AUX
> +		 * channel; the physical i2c pins only reach DVI/HDMI sinks
> +		 * (e.g. DP++ with a passive adapter).  Route accordingly so
> +		 * the connector's single "ddc" bus works for either sink.
> +		 */
> +		if (aconnector && link->local_sink &&
> +		    dc_is_dp_signal(link->local_sink->sink_signal) &&

[Severity: Critical]
What protects link->local_sink against concurrent hotplug or link detection
while amdgpu_dm_i2c_xfer() reads and dereferences it?

Userspace I2C transfers on /dev/i2c-N invoke amdgpu_dm_i2c_xfer() holding
only i2c_adap->bus_lock, without acquiring aconnector->hpd_lock or
adev->dm.dc_lock and without taking a sink reference via dc_sink_retain().

Concurrently, link detection and disconnect paths release and free
link->local_sink before clearing or updating the pointer:

drivers/gpu/drm/amd/display/dc/link/link_detection.c:link_disconnect_sink() {
	if (link->local_sink) {
		dc_sink_release(link->local_sink);
		link->local_sink = NULL;
	}
	...
}

drivers/gpu/drm/amd/display/dc/link/link_detection.c:link_disconnect_remap() {
	dc_sink_release(link->local_sink);
	link->local_sink = prev_sink;
}

Similar unserialized releases happen in amdgpu_dm_connector_poll(),
amdgpu_dm_emulated_link_detect(), and detect_link_and_local_sink() (for
example when a temporary local_sink with refcount 1 is released on EDID
read failure, same_edid remap, or MST topology discovery). Could a
concurrent I2C transfer observe a non-NULL link->local_sink and suffer a
use-after-free or NULL pointer dereference when reading
link->local_sink->sink_signal?

> +		    aconnector->dm_dp_aux.aux.ddc.algo)
> +			return i2c_transfer(&aconnector->dm_dp_aux.aux.ddc, msgs, num);
> +	}

[Severity: Medium]
Will checking link->local_sink and dc_is_dp_signal() work for DP MST links
or links with force_to_use_aux set?

When an MST topology is started in discover_dp_mst_topology(),
link_disconnect_sink() is called immediately:

drivers/gpu/drm/amd/display/dc/link/link_detection.c:discover_dp_mst_topology() {
	...
	if (dm_helpers_dp_mst_start_top_mgr(link->ctx,
			link, (reason == DETECT_REASON_BOOT || reason == DETECT_REASON_RESUMEFROMS3S4))) {
		link_disconnect_sink(link);
	} else {
	...
}

This sets link->local_sink to NULL while downstream sinks are stored in
link->remote_sinks[], so link->local_sink is NULL on active MST links and
transfers will not be routed to the AUX adapter as described in the commit
message.

Additionally, on connectors with link->force_to_use_aux set (such as ASICs
with no_ddc_pin where ddc_service->ddc_pin is NULL), detect_dp() sets the
transaction type to DDC_TRANSACTION_TYPE_I2C_OVER_AUX (setting
link->aux_mode = true) while dp_passive_dongle_detection() sets
sink_caps->signal to HDMI or DVI:

drivers/gpu/drm/amd/display/dc/link/link_detection.c:detect_dp() {
	...
		if (!detect_dp_sink_caps(link)) {
			if (link->force_to_use_aux) {
				sink_caps->signal = dp_passive_dongle_detection(link->ddc, sink_caps, audio_support);
	...
}

In that case dc_is_dp_signal(link->local_sink->sink_signal) is false and
the transfer falls through to the hardware I2C path (or fails with -EIO
when ddc_pin is NULL). Would checking link->aux_mode (similar to other DDC
selection paths in amdgpu_dm) handle native DP, MST, and force_to_use_aux
links without dereferencing link->local_sink?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-superm1-amdgpu-ddc-v2-1-7bde6d004382@kernel.org?part=1