Re: [PATCH v3 11/13] mtd: spinand: run PHY tuning after init and update dirmap frequencies

[sashiko-bot@kernel.org]

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

New issues:
- [High] Mutating `spi_mem_dirmap_desc` fields after creation bypasses SPI controller hardware configuration and validation checks.
- [High] Validated frequencies from `read_cache` and `write_cache` are blindly applied to mismatched or unvalidated secondary operation templates.

Pre-existing issues:
- [High] Teardown symmetry violation in `spinand_remove` leads to a Use-After-Free of the `spinand` device and memory leaks if `mtd_device_unregister` fails.
--

commit 82e2a165fb470e6047a759242be19d68a59ffe43
Author: Santhosh Kumar K <s-k6@ti.com>

mtd: spinand: run PHY tuning after init and update dirmap frequencies

This commit modifies spinand_probe() to run PHY tuning after spinand_init()
has completed. It then directly updates the dirmaps with the newly validated
maximum frequencies for both primary and secondary operation templates.

> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c

[ ... ]

> @@ -2050,6 +2050,41 @@ static int spinand_probe(struct spi_mem *mem)

[ ... ]

> +	if (!ret) {
> +		struct nand_device *nand = spinand_to_nand(spinand);
> +		int i;
> +
> +		for (i = 0; i < nand->memorg.planes_per_lun; i++) {
> +			if (spinand->dirmaps[i].rdesc) {
> +				spinand->dirmaps[i].rdesc->info.primary_op_tmpl.max_freq =
> +					spinand->max_read_op.max_freq;

[Severity: High]
Does mutating spi_mem_dirmap_desc fields after creation bypass the SPI
controller hardware configuration and validation checks?

The dirmap is fully initialized earlier during spinand_init(), where
controllers execute their dirmap_create callbacks to calculate clock
dividers and cache hardware registers based on the initial max_freq.

If we directly mutate the frequency here without notifying the controller,
will the hardware remain configured for the old, un-tuned frequency while
the SPI core assumes the new frequency, potentially causing a state
desynchronization or data corruption?

> +				spinand->dirmaps[i].rdesc->info.secondary_op_tmpl.max_freq =
> +					spinand->max_read_op.max_freq;
> +			}
> +			if (spinand->dirmaps[i].wdesc)
> +				spinand->dirmaps[i].wdesc->info.primary_op_tmpl.max_freq =
> +					spinand->max_write_op.max_freq;

[Severity: High]
Is it safe to apply the validated frequencies from the read_cache and
write_cache templates directly to these other operation templates?

The PHY tuning is executed specifically on the write_cache and read_cache
templates. However, wdesc->info.primary_op_tmpl is based on update_cache,
and rdesc->info.secondary_op_tmpl is used for cont_read_cache.

Since these operations utilize different opcodes and potentially different
dummy cycles, could applying the frequency of one tuned operation to a
different unvalidated operation cause silent data corruption?

> +		}
> +	}
> +
>  	ret = mtd_device_register(mtd, NULL, 0);

[Severity: High]
This is a pre-existing issue, but can spinand_remove() cause a use-after-free
and memory leak if mtd_device_unregister() fails?

If mtd_device_unregister() returns an error code, spinand_remove()
immediately returns this error and skips spinand_cleanup(), leaking
databuf and scratchbuf.

Because the SPI framework's wrapper spi_mem_remove() returns void and ignores
the error, the driver core will proceed to unbind the device and free all
devm_kzalloc allocated memory, including the spinand structure.

Since the MTD device wasn't unregistered, the MTD subsystem retains active
pointers to the freed spinand structure. Could this lead to a use-after-free
vulnerability upon subsequent accesses?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527175527.2247679-1-s-k6@ti.com?part=11