[PATCH] drm/amd/display: Don't free devres-owned i2c adapter on init failure

[PATCH] drm/amd/display: Don't free devres-owned i2c adapter on init failure

[yaolu]

From: Lu Yao <yaolu@kylinos.cn>

amdgpu_dm_connector_init() registers the i2c adapter with
devm_i2c_add_adapter().It will be released by devres on teardown, so
we don't need to free it when failed.

Fixes: 5b3eca05cfb0 ("drm/amd/display: Use devm_i2c_add_adapter to simplify i2c cleanup logic")
Signed-off-by: Lu Yao <yaolu@kylinos.cn>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 b498fc5fbb14..ccbda54c3add 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
@@ -3514,7 +3514,7 @@ int amdgpu_dm_connector_init(struct amdgpu_display_manager *dm,
 	if (res) {
 		drm_err(adev_to_drm(dm->adev), "connector_init failed\n");
 		aconnector->connector_id = -1;
-		goto out_free;
+		return res;
 	}
 
 	drm_connector_helper_add(
-- 
2.25.1

Re: [PATCH] drm/amd/display: Don't free devres-owned i2c adapter on init failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/amd/display: leak and dangling ddc_service in
  amdgpu_dm_connector_init()
--

commit cd13d71ccba1fac1ae6d56da56c40ca922c6fa4a
Author: Lu Yao <yaolu@kylinos.cn>

drm/amd/display: Don't free devres-owned i2c adapter on init failure

This commit changes amdgpu_dm_connector_init() to return directly instead of
jumping to out_free when drm_connector_init_with_ddc() fails.

> 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..ccbda54c3add8 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
> @@ -3514,7 +3514,7 @@ int amdgpu_dm_connector_init(struct amdgpu_display_manager *dm,
>  	if (res) {
>  		drm_err(adev_to_drm(dm->adev), "connector_init failed\n");
>  		aconnector->connector_id = -1;
> -		goto out_free;
> +		return res;
>  	}

[Severity: Medium]
Does returning here leak the struct amdgpu_i2c_adapter and leave the adapter
registered with a freed i2c->ddc_service during error unwind?

amdgpu_dm_create_i2c() allocates i2c with kzalloc_obj(), while
devm_i2c_add_adapter() only schedules i2c_del_adapter(&i2c->base) via devres
and never frees the enclosing struct amdgpu_i2c_adapter.  When
amdgpu_dm_connector_init() fails, amdgpu_dm_initialize_drm_device() frees
aconnector and loses the only pointer to i2c.

In addition, amdgpu_dm_init() calls amdgpu_dm_fini() -> dc_destroy(), which
frees link->ddc (i2c->ddc_service) before dm_hw_init() reaches
devres_release_group() to unregister &i2c->base.  Unregistering the adapter
and freeing i2c (or allocating i2c with devm_kzalloc() and releasing the
devres group before dc_destroy()) would avoid both the leak and the dangling
ddc_service window.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110070204.307887-1-yaolu@kylinos.cn?part=1

[PATCH v2] drm/amd/display: Fix dm i2c adapter lifetime on teardown

[yaolu]

From: Lu Yao <yaolu@kylinos.cn>

devm_i2c_add_adapter() only calls i2c_del_adapter() on teardown, so
struct amdgpu_i2c_adapter is never freed.  Since
commit 5b3eca05cfb0 ("drm/amd/display: Use devm_i2c_add_adapter to simplify i2c cleanup logic")
every dm i2c adapter leaks on unload.

The connector_init error path cannot kfree() it either: the adapter is
still registered, so devres release would causing UAF.

On the init failure path dc_destroy() also frees i2c->ddc_service before
dm_hw_init() releases the devres group, so there also may causing UAF.

Allocate the adapter with devm_kzalloc_obj() and release the devres group
in amdgpu_dm_fini() right before dc_destroy().

Fixes: 5b3eca05cfb0 ("drm/amd/display: Use devm_i2c_add_adapter to simplify i2c cleanup logic")
Signed-off-by: Lu Yao <yaolu@kylinos.cn>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 15 ++++++---------
 .../amd/display/amdgpu_dm/amdgpu_dm_connector.c   | 13 ++++---------
 2 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 0ea45ab12119..5dbc00e6ee63 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -961,6 +961,11 @@ static void amdgpu_dm_fini(struct amdgpu_device *adev)
 
 	amdgpu_dm_irq_fini(adev);
 
+	if (adev->dm.i2c_devres_group) {
+		devres_release_group(adev->dev, adev->dm.i2c_devres_group);
+		adev->dm.i2c_devres_group = NULL;
+	}
+
 	/* DC Destroy TODO: Replace destroy DAL */
 	if (adev->dm.dc)
 		dc_destroy(&adev->dm.dc);
@@ -1364,7 +1369,6 @@ STATIC_IFN_KUNIT int dm_oem_i2c_hw_init(struct amdgpu_device *adev)
 		r = devm_i2c_add_adapter(adev->dev, &oem_i2c->base);
 		if (r) {
 			drm_info(adev_to_drm(adev), "Failed to register oem i2c\n");
-			kfree(oem_i2c);
 			return r;
 		}
 		dm->oem_i2c = oem_i2c;
@@ -1406,7 +1410,7 @@ static int dm_hw_init(struct amdgpu_ip_block *ip_block)
 	/* Create DAL display manager */
 	r = amdgpu_dm_init(adev);
 	if (r)
-		goto err_release_i2c;
+		return r;
 	amdgpu_dm_hpd_init(adev);
 
 	r = dm_oem_i2c_hw_init(adev);
@@ -1415,10 +1419,6 @@ static int dm_hw_init(struct amdgpu_ip_block *ip_block)
 
 	devres_close_group(adev->dev, adev->dm.i2c_devres_group);
 	return 0;
-
-err_release_i2c:
-	devres_release_group(adev->dev, adev->dm.i2c_devres_group);
-	return r;
 }
 
 /**
@@ -1433,9 +1433,6 @@ static int dm_hw_fini(struct amdgpu_ip_block *ip_block)
 {
 	struct amdgpu_device *adev = ip_block->adev;
 
-	if (adev->dm.i2c_devres_group)
-		devres_release_group(adev->dev, adev->dm.i2c_devres_group);
-
 	amdgpu_dm_hpd_fini(adev);
 
 	amdgpu_dm_fini(adev);
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 b498fc5fbb14..2e262f8ffc5f 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
@@ -3427,7 +3427,7 @@ amdgpu_dm_create_i2c(struct ddc_service *ddc_service, bool oem)
 	struct amdgpu_device *adev = ddc_service->ctx->driver_context;
 	struct amdgpu_i2c_adapter *i2c;
 
-	i2c = kzalloc_obj(struct amdgpu_i2c_adapter);
+	i2c = devm_kzalloc_obj(adev->dev, struct amdgpu_i2c_adapter);
 	if (!i2c)
 		return NULL;
 	i2c->base.owner = THIS_MODULE;
@@ -3499,7 +3499,7 @@ int amdgpu_dm_connector_init(struct amdgpu_display_manager *dm,
 
 	if (res) {
 		drm_err(adev_to_drm(dm->adev), "Failed to register hw i2c %d\n", link->link_index);
-		goto out_free;
+		return res;
 	}
 
 	connector_type = to_drm_connector_type(link->connector_signal, link->link_id.id);
@@ -3514,7 +3514,7 @@ int amdgpu_dm_connector_init(struct amdgpu_display_manager *dm,
 	if (res) {
 		drm_err(adev_to_drm(dm->adev), "connector_init failed\n");
 		aconnector->connector_id = -1;
-		goto out_free;
+		return res;
 	}
 
 	drm_connector_helper_add(
@@ -3538,12 +3538,7 @@ int amdgpu_dm_connector_init(struct amdgpu_display_manager *dm,
 	if (dc_is_dp_signal(link->connector_signal))
 		amdgpu_dm_initialize_dp_connector(dm, aconnector, link->link_index);
 
-out_free:
-	if (res) {
-		kfree(i2c);
-		aconnector->i2c = NULL;
-	}
-	return res;
+	return 0;
 }
 EXPORT_IF_KUNIT(amdgpu_dm_connector_init);
 
-- 
2.25.1

Re: [PATCH v2] drm/amd/display: Fix dm i2c adapter lifetime on teardown

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/amd/display: NULL deref and bad kfree() in
  amdgpu_dm_create_i2c() tests
--

commit 34d2cc7ca5025a414952478c53098d172f5f6cc4
Author: Lu Yao <yaolu@kylinos.cn>

drm/amd/display: Fix dm i2c adapter lifetime on teardown

This commit allocates struct amdgpu_i2c_adapter with devm_kzalloc_obj()
and moves the release of dm.i2c_devres_group into amdgpu_dm_fini() before
dc_destroy().

> 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 8db7956c01d32..ae99058dafba4 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
> @@ -3437,7 +3437,7 @@ amdgpu_dm_create_i2c(struct ddc_service *ddc_service, bool oem)
>  	struct amdgpu_device *adev = ddc_service->ctx->driver_context;
>  	struct amdgpu_i2c_adapter *i2c;
>  
> -	i2c = kzalloc_obj(struct amdgpu_i2c_adapter);
> +	i2c = devm_kzalloc_obj(adev->dev, struct amdgpu_i2c_adapter);

[Severity: Medium]
Does switching amdgpu_dm_create_i2c() to devm_kzalloc_obj() break the
dm_test_create_i2c_oem() and dm_test_create_i2c_hw_bus() KUnit tests in
drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c?

dm_test_create_i2c_setup() leaves ctx->adev->dev as NULL, so
devm_kzalloc_obj(adev->dev, ...) triggers a NULL pointer dereference in
devres_node_add() when locking dev->devres_lock.  Both tests also call
raw kfree(i2c) on the returned pointer, which is no longer valid for a
devres-managed allocation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111021925.63610-1-yaolu@kylinos.cn?part=1