[PATCH] drm/tegra: Fix a strange error handling path

[PATCH] drm/tegra: Fix a strange error handling path

[Christophe JAILLET]

The resource freed at the 'put_aux' label is "sor->aux->dev".
However, this resource is taken after devm_tegra_pmc_get(), so there is no
point to release it in this error handling path.

This is harmless because put_device() will be called with a NULL pointer,
but this is confusing.

So, fix the logic and return directly.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Compile tested only.

This is not a bug, so no need for a Fixes tag.
If needed anyway, it was introduced by commit e68c4244136b ("drm/tegra:
Explicitly specify PMC instance to use")
---
 drivers/gpu/drm/tegra/sor.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
index 4032c6ad45bc..e384cbd0cbf7 100644
--- a/drivers/gpu/drm/tegra/sor.c
+++ b/drivers/gpu/drm/tegra/sor.c
@@ -3764,10 +3764,8 @@ static int tegra_sor_probe(struct platform_device *pdev)
 	sor->num_settings = sor->soc->num_settings;
 
 	sor->pmc = devm_tegra_pmc_get(&pdev->dev);
-	if (IS_ERR(sor->pmc)) {
-		err = PTR_ERR(sor->pmc);
-		goto put_aux;
-	}
+	if (IS_ERR(sor->pmc))
+		return PTR_ERR(sor->pmc);
 
 	np = of_parse_phandle(pdev->dev.of_node, "nvidia,dpaux", 0);
 	if (np) {
-- 
2.54.0

Re: [PATCH] drm/tegra: Fix a strange error handling path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Race condition and Use-After-Free due to unreferenced pointer returned from drm_dp_aux_find_by_of_node().
- [High] Ignored get_device() failure leaves a dangling pointer leading to Use-After-Free on error paths.
- [High] Unbalanced i2c_put_adapter() on error path causes I2C adapter reference count underflow.
--

commit 214c0c58066e5c28266a88ecb7dc4e05a84c1ad9
Author: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

drm/tegra: Fix a strange error handling path

The commit fixes an error handling path in tegra_sor_probe() by returning
directly when devm_tegra_pmc_get() fails, instead of jumping to the put_aux
label which would release an uninitialized resource.

> diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
> index 4032c6ad45bc1..e384cbd0cbf72 100644
> --- a/drivers/gpu/drm/tegra/sor.c
> +++ b/drivers/gpu/drm/tegra/sor.c
> @@ -3764,10 +3764,8 @@ static int tegra_sor_probe(struct platform_device *pdev)
>  	sor->num_settings = sor->soc->num_settings;
>  
>  	sor->pmc = devm_tegra_pmc_get(&pdev->dev);
> -	if (IS_ERR(sor->pmc)) {
> -		err = PTR_ERR(sor->pmc);
> -		goto put_aux;
> -	}
> +	if (IS_ERR(sor->pmc))
> +		return PTR_ERR(sor->pmc);
>  
>  	np = of_parse_phandle(pdev->dev.of_node, "nvidia,dpaux", 0);
>  	if (np) {

[Severity: High]
This is a pre-existing issue, but can this lead to a race condition and a 
use-after-free?

Looking at drm_dp_aux_find_by_of_node() in drivers/gpu/drm/tegra/dpaux.c, it
searches dpaux_list while holding dpaux_lock. Upon finding a match, it drops
the lock and returns a pointer to dpaux->aux without taking a reference 
count on the device.

Because tegra_dpaux is a separate platform device, it can be concurrently
unbound, triggering tegra_dpaux_remove() and freeing the memory via devres.

When tegra_sor_probe() receives this pointer and calls
get_device(sor->aux->dev), if tegra_dpaux_remove() executes after the lock
is released but before get_device() is called, will this dereference
freed memory?

[Severity: High]
This isn't a bug introduced by this patch, but does the failure path of
get_device() leave a dangling pointer?

In tegra_sor_probe():

    if (get_device(sor->aux->dev))
        sor->output.ddc = &sor->aux->ddc;

If get_device() fails (returns NULL if the aux device is concurrently
removed), the failure is silently ignored. The output.ddc assignment is
skipped, but sor->aux remains set to the invalid pointer.

If a subsequent initialization step fails, the probe function jumps to
the put_aux label:

put_aux:
    if (sor->aux)
        put_device(sor->aux->dev);

Will this read the device structure from the dangling pointer and call
put_device() on freed memory, leading to a use-after-free?

[Severity: High]
This is a pre-existing issue, but does the error path in tegra_output_probe()
cause an I2C adapter reference count underflow?

In tegra_sor_probe(), sor->output.ddc is aliased to &sor->aux->ddc without
explicitly taking an I2C adapter reference via i2c_get_adapter().

The code then calls tegra_output_probe(&sor->output). If this function
encounters an error, it jumps to the put_i2c label:

drivers/gpu/drm/tegra/output.c:tegra_output_probe() {
    ...
put_i2c:
    if (output->ddc)
        i2c_put_adapter(output->ddc);
    ...
}

Since output->ddc points to the alias, i2c_put_adapter() will drop the
device and module reference without a matching get. Can this cause the
adapter to be prematurely freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/308ea09281bd0b3fb0ec7e90e23012c0783f373c.1781983379.git.christophe.jaillet@wanadoo.fr?part=1