Re: [PATCH 1/3] mtd: rawnand: qcom: parse boot partitions before registering MTD

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] mtd: rawnand: qcom: unlocked bam_txn reallocation in
  qcom_nand_attach_chip() [1]
--

commit aecfb39160da37f68bc9b557fcfe8446b871939e
Author: James Hilliard <james.hilliard1@gmail.com>

mtd: rawnand: qcom: parse boot partitions before registering MTD

This commit parses boot partitions in qcom_nand_host_init_and_register()
before calling mtd_device_parse_register() so that parsing errors do not
call nand_cleanup() on an already registered MTD device.

> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index 2d98f79b492d6..f240d1effa043 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -2052,16 +2052,16 @@ static int qcom_nand_host_init_and_register(struct qcom_nand_controller *nandc,
>  	if (ret)
>  		return ret;
>  
> -	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;
>  	}
>  
> +	ret = mtd_device_parse_register(mtd, probes, NULL, NULL, 0);
> +	if (ret)
> +		goto err;

[Severity: Medium]
This problem wasn't introduced by this patch, but can qcom_nand_attach_chip()
free and reallocate nandc->bam_txn [1] while an earlier chip is already
registered and servicing I/O?

qcom_probe_nand_devices() calls qcom_nand_host_init_and_register() for each
child node in turn.  Once the first chip is registered, MTD operations on it
take nandc->controller.lock and use the controller-wide nandc->bam_txn, while
probing a second chip runs qcom_nand_attach_chip() without holding
nandc->controller.lock:

drivers/mtd/nand/raw/qcom_nandc.c:qcom_nand_attach_chip() {
	...
	if (nandc->props->supports_bam)
		qcom_free_bam_transaction(nandc);

	nandc->max_cwperpage = max_t(unsigned int, nandc->max_cwperpage,
				     cwperpage);

	/* Now allocate the BAM transaction based on updated max_cwperpage */
	if (nandc->props->supports_bam) {
		nandc->bam_txn = qcom_alloc_bam_transaction(nandc);
		if (!nandc->bam_txn) {
			dev_err(nandc->dev,
				"failed to allocate bam transaction\n");
			return -ENOMEM;
		}
	}
	...
}

This allows the second chip's scan to free nandc->bam_txn underneath an
in-flight operation on the first chip:

CPU0                                    CPU1
qcom_probe_nand_devices()
  qcom_nand_host_init_and_register()
    mtd_device_parse_register() [chip 0]
                                        nand_read_oob() [chip 0]
                                          nand_get_device()
                                          qcom_nandc_read_page()
                                            ...
  qcom_nand_host_init_and_register() [chip 1]
    nand_scan()
      ...
        qcom_nand_attach_chip()
          qcom_free_bam_transaction()
                                            qcom_clear_bam_transaction()
                                              <-- use-after-free
          qcom_alloc_bam_transaction()

Also, if qcom_alloc_bam_transaction() fails for the second chip,
nandc->bam_txn is left NULL while the first chip remains registered, causing
subsequent operations on the first chip to hit a NULL pointer dereference.

[1] https://sashiko.dev/#/bug/linux-f5ec08db-a039-4be6-ba66-945cfc6d14f5

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-submit-qcom-nand-child-probe-errors-v1-v1-0-509f30a9e14d@gmail.com?part=1

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/