Re: [PATCH] mtd: maps: add INT0800 firmware-flash map driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] mtd: memory and mapping leak on mtd_device_register() failure
- [Medium] mtd: shared state corruption via global variables in probe
- [Low] mtd: format string mismatch for resource_size_t in dev_info
--

commit a217a15f9e29300a155e6f6c8138325d4f178f7b
Author: Stephen Bancroft <stevereaver@gmail.com>

mtd: maps: add INT0800 firmware-flash map driver

This commit adds a read-only MTD mapping driver for the ACPI INT0800
firmware hub device on x86. It exposes the system firmware flash as an
MTD ROM device to allow userspace to read the firmware flash contents safely.

> diff --git a/drivers/mtd/maps/int0800.c b/drivers/mtd/maps/int0800.c
> new file mode 100644
> index 0000000000000..2438d970b9fa4
> --- /dev/null
> +++ b/drivers/mtd/maps/int0800.c
[ ... ]
> +static struct map_info int0800_map = {
> +	.name		= "int0800",
> +	.bankwidth	= 1,
> +};
> +
> +static struct mtd_info *int0800_mtd;
> +
> +static int int0800_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	int0800_map.virt = ioremap(int0800_map.phys, int0800_map.size);

[Severity: Medium]
Since int0800_map and int0800_mtd are global variables, what happens if
multiple INT0800 devices are enumerated?

It looks like this unconditionally overwrites the global state during
int0800_probe(). Later, int0800_remove() will free these globally stored
pointers, which could destroy the state for all active instances or cause a
double-free.

[ ... ]
> +	dev_info(&pdev->dev, "mapped firmware window 0x%lx-0x%lx\n",
> +		 int0800_map.phys,
> +		 int0800_map.phys + int0800_map.size - 1);

[Severity: Low]
Does this format string cause issues on 32-bit architectures where
resource_size_t is a 64-bit integer?

Using %lx for resource_size_t can consume only half the value and misalign
subsequent arguments on the stack. Should this use %pa or an explicit cast
to u64 with %llx instead?

> +
> +	return mtd_device_register(int0800_mtd, NULL, 0);
> +}

[Severity: Medium]
If mtd_device_register() fails here, do we leak the memory mapping and the
MTD instance?

Normally, we would expect to see map_destroy() and iounmap() on this failure
path, similar to what is done in int0800_remove(), but here the error is
returned directly.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917010431.29714-1-stevereaver@gmail.com?part=1

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