linux-kernel - Re: camss: Missing cleanup on probe error

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ae3d5dd0-8d68-9d4e-c98c-7284d1ebeccc@nexus-software.ie>
Date:   Wed, 25 Jan 2023 11:34:33 +0000
From:   Bryan O'Donoghue <pure.logic@...us-software.ie>
To:     Luca Weiss <luca@...tu.xyz>, Todor Tomov <todor.too@...il.com>,
        Robert Foss <robert.foss@...aro.org>
Cc:     linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: camss: Missing cleanup on probe error

On 24/01/2023 20:02, Luca Weiss wrote:
> Hi all,
> 
> I hit an issue that was already fixed with upstream commit 3d658980e6da
> ("media: camss: Do not attach an already attached power domain on MSM8916
> platform") but this showed that some error handling is missing in camss_probe.
> 
> After these errors the resources aren't cleaned up and e.g. sensor drivers
> still try to probe but fail quite badly because some things are NULL or
> whatever.
> 
> [    1.979098] qcom-camss fda0ac00.camss: Failed to configure power domains:
> -17
> [    1.989327] qcom-camss: probe of fda0ac00.camss failed with error -17
> 
> The commit causing this (or at least part of this) is 2f6f8af67203 ("media:
> camss: Refactor VFE power domain toggling")
> 
> I tried a bit to add some error handling but in this case it now fails at
> v4l2_device_unregister_subdev -> media_device_unregister_entity ->
> __media_device_unregister_entity -> ida_free.
> And I'm not really sure how to fix (and honestly not super motivated).
> 
> See following patch (formatting probably messed up, sorry). Maybe someone who
> knows a bit more about the driver or the media subsystem can fix this, thanks!
> 
> Regards
> Luca
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/
> platform/qcom/camss/camss.c
> index 16545cecc4f4..0ba3c378d241 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1836,21 +1836,26 @@ static int camss_probe(struct platform_device *pdev)
>   		}
>   	}
>   
> -	ret = camss_configure_pd(camss);
> +	//ret = camss_configure_pd(camss);
> +	ret = -17;
>   	if (ret < 0) {
>   		dev_err(dev, "Failed to configure power domains: %d\n",
> ret);
> -		return ret;
> +		goto err_configure_pd;
>   	}
>   
>   	pm_runtime_enable(dev);
>   
>   	return 0;
>   
> +err_configure_pd:
> +	media_device_unregister(&camss->media_dev);
> +	media_device_cleanup(&camss->media_dev);
>   err_register_subdevs:
>   	camss_unregister_entities(camss);
>   err_register_entities:
>   	v4l2_device_unregister(&camss->v4l2_dev);
>   err_cleanup:
> +	v4l2_async_nf_unregister(&camss->notifier);
>   	v4l2_async_nf_cleanup(&camss->notifier);
>   
>   	return ret;
> 
> 
> 

I'll take a look