Re: [PATCH 2/4] mtd: rawnand: qcom: Make "aon" clock optional

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free due to missing MTD device unregistration on boot partition parsing failure.
- [High] Use-After-Free due to missing MTD unregistration for successful child nodes on probe failure.
--

commit 36076ea85a481eb0cc14bb0f987501c4cc6b4d63
Author: Stephan Gerhold <stephan.gerhold@linaro.org>
Subject: mtd: rawnand: qcom: Make "aon" clock optional

Some SoCs (e.g. MDM9607, SDX55) have only a single separately controllable
clock for the NAND controller. The actual clocks in the hardware are
managed by the firmware and turned on all together when needed. In this
case, there is no separate "aon" clock that can be described in the device
tree. Make the second "aon" clock optional to avoid an error when it is
missing.

> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index 4b80ce084d9ac..0251dd591d40a 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -2280,7 +2280,7 @@ static int qcom_nandc_probe(struct platform_device *pdev)
>  	if (IS_ERR(nandc->core_clk))
>  		return PTR_ERR(nandc->core_clk);
>  
> -	nandc->aon_clk = devm_clk_get(dev, "aon");
> +	nandc->aon_clk = devm_clk_get_optional(dev, "aon");
>  	if (IS_ERR(nandc->aon_clk))
>  		return PTR_ERR(nandc->aon_clk);

[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free in
qcom_probe_nand_devices() that could occur during probe failure.

If an earlier child node is successfully registered, but a later one fails
to allocate memory, the function returns directly:

drivers/mtd/nand/raw/qcom_nandc.c:qcom_probe_nand_devices() {
	...
	for_each_available_child_of_node_scoped(dn, child) {
		host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
		if (!host)
			return -ENOMEM;
	...
}

Are the previously successful children left registered with the MTD
subsystem? Since they are never unregistered before the error is returned,
the driver core will automatically free their memory via devres, which
might leave dangling pointers in the MTD lists.

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be another
missing unregistration path in qcom_nand_host_init_and_register().

If mtd_device_parse_register() succeeds, the device is fully registered:

drivers/mtd/nand/raw/qcom_nandc.c:qcom_nand_host_init_and_register() {
	...
	ret = mtd_device_parse_register(mtd, probes, NULL, NULL, 0);
	if (ret)
		goto err;

	if (nandc->props->use_codeword_fixup) {
		ret = qcom_nand_host_parse_boot_partitions(nandc, host, dn);
		if (ret)
			goto err;
	...
err:
	nand_cleanup(chip);
	return ret;
}

If qcom_nand_host_parse_boot_partitions() fails, the code jumps to the err
label and performs nand_cleanup(), but doesn't call mtd_device_unregister().

When qcom_probe_nand_devices() subsequently frees the host memory via
devm_kfree(), could this leave the MTD device registered while freeing
its backing memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608-qcom-nandc-mdm9607-v1-0-4639a0492274@linaro.org?part=2