From 492151b2093ee4df173f7e10da4938ef7c2a2156 Mon Sep 17 00:00:00 2001 From: Boris Brezillon Date: Sun, 2 Dec 2018 10:54:25 +0100 Subject: mtd: sf: Make sure we don't register the same device twice spi_flash_mtd_register() can be called several times and each time it will register the same mtd_info instance like if it was a new one. The MTD ID allocation gets crazy when that happens, so let's track the status of the sf_mtd_info object to avoid that. Fixes: 9fe6d8716e09 ("mtd, spi: Add MTD layer driver") Signed-off-by: Boris Brezillon Tested-by: Heiko Schocher Reviewed-by: Jagan Teki --- drivers/mtd/spi/sf_mtd.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) (limited to 'drivers/mtd/spi') diff --git a/drivers/mtd/spi/sf_mtd.c b/drivers/mtd/spi/sf_mtd.c index 58d7e443990..aabbc358943 100644 --- a/drivers/mtd/spi/sf_mtd.c +++ b/drivers/mtd/spi/sf_mtd.c @@ -10,6 +10,7 @@ #include static struct mtd_info sf_mtd_info; +static bool sf_mtd_registered; static char sf_mtd_name[8]; static int spi_flash_mtd_erase(struct mtd_info *mtd, struct erase_info *instr) @@ -73,6 +74,12 @@ static int spi_flash_mtd_number(void) int spi_flash_mtd_register(struct spi_flash *flash) { + int ret; + + if (sf_mtd_registered) + del_mtd_device(&sf_mtd_info); + + sf_mtd_registered = false; memset(&sf_mtd_info, 0, sizeof(sf_mtd_info)); sprintf(sf_mtd_name, "nor%d", spi_flash_mtd_number()); @@ -94,7 +101,11 @@ int spi_flash_mtd_register(struct spi_flash *flash) sf_mtd_info.numeraseregions = 0; sf_mtd_info.erasesize = flash->sector_size; - return add_mtd_device(&sf_mtd_info); + ret = add_mtd_device(&sf_mtd_info); + if (!ret) + sf_mtd_registered = true; + + return ret; } void spi_flash_mtd_unregister(void) -- cgit v1.2.3 From 7371944a71690abafd0717b5d5f72c67e9f0f414 Mon Sep 17 00:00:00 2001 From: Boris Brezillon Date: Sun, 2 Dec 2018 10:54:31 +0100 Subject: mtd: sf: Unregister the MTD device prior to removing the spi_flash obj The DM implementation of spi_flash_free() does not unregister the MTD device before removing the spi dev object. This leads to a use-after-free bug when the MTD device is later accessed by a MTD user (observed when attaching the device to UBI after env_sf_load() has called spi_flash_free()). Implement ->remove() and call spi_flash_mtd_unregister() from there. Fixes: 9fe6d8716e09 ("mtd, spi: Add MTD layer driver") Signed-off-by: Boris Brezillon Tested-by: Heiko Schocher Reviewed-by: Jagan Teki --- drivers/mtd/spi/sf_probe.c | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'drivers/mtd/spi') diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c index 94fde2ae7a3..4d7320fe8c1 100644 --- a/drivers/mtd/spi/sf_probe.c +++ b/drivers/mtd/spi/sf_probe.c @@ -137,6 +137,14 @@ static int spi_flash_std_probe(struct udevice *dev) return spi_flash_probe_slave(flash); } +static int spi_flash_std_remove(struct udevice *dev) +{ +#ifdef CONFIG_SPI_FLASH_MTD + spi_flash_mtd_unregister(); +#endif + return 0; +} + static const struct dm_spi_flash_ops spi_flash_std_ops = { .read = spi_flash_std_read, .write = spi_flash_std_write, @@ -153,6 +161,7 @@ U_BOOT_DRIVER(spi_flash_std) = { .id = UCLASS_SPI_FLASH, .of_match = spi_flash_std_ids, .probe = spi_flash_std_probe, + .remove = spi_flash_std_remove, .priv_auto_alloc_size = sizeof(struct spi_flash), .ops = &spi_flash_std_ops, }; -- cgit v1.2.3 From 08898e8b22d74a4511eadee9b06b11aab43e809c Mon Sep 17 00:00:00 2001 From: Boris Brezillon Date: Sun, 2 Dec 2018 10:54:32 +0100 Subject: mtd: sf: Make sf_mtd.c more robust SPI flash based MTD devs can be registered/unregistered at any time through the sf probe command or the spi_flash_free() function. This commit does not try to fix the root cause as it would probably require rewriting most of the code and have an mtd_info object instance per spi_flash object (not to mention that the the spi-flash layer is likely to be replaced by a spi-nor layer ported from Linux). Instead, we try to be as safe as can be by checking the code returned by del_mtd_device() and complain loudly when there's nothing we can do about the deregistration failure. When that happens we also reset sf_mtd_info.priv to NULL, and check for NULL pointer in the mtd hooks so that -ENODEV is returned instead of hitting a NULL pointer dereference exception when the MTD instance is later accessed by a user. Signed-off-by: Boris Brezillon Tested-by: Heiko Schocher --- drivers/mtd/spi/sf_mtd.c | 39 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-) (limited to 'drivers/mtd/spi') diff --git a/drivers/mtd/spi/sf_mtd.c b/drivers/mtd/spi/sf_mtd.c index aabbc358943..68c36002bee 100644 --- a/drivers/mtd/spi/sf_mtd.c +++ b/drivers/mtd/spi/sf_mtd.c @@ -18,6 +18,9 @@ static int spi_flash_mtd_erase(struct mtd_info *mtd, struct erase_info *instr) struct spi_flash *flash = mtd->priv; int err; + if (!flash) + return -ENODEV; + instr->state = MTD_ERASING; err = spi_flash_erase(flash, instr->addr, instr->len); @@ -39,6 +42,9 @@ static int spi_flash_mtd_read(struct mtd_info *mtd, loff_t from, size_t len, struct spi_flash *flash = mtd->priv; int err; + if (!flash) + return -ENODEV; + err = spi_flash_read(flash, from, len, buf); if (!err) *retlen = len; @@ -52,6 +58,9 @@ static int spi_flash_mtd_write(struct mtd_info *mtd, loff_t to, size_t len, struct spi_flash *flash = mtd->priv; int err; + if (!flash) + return -ENODEV; + err = spi_flash_write(flash, to, len, buf); if (!err) *retlen = len; @@ -76,8 +85,13 @@ int spi_flash_mtd_register(struct spi_flash *flash) { int ret; - if (sf_mtd_registered) - del_mtd_device(&sf_mtd_info); + if (sf_mtd_registered) { + ret = del_mtd_device(&sf_mtd_info); + if (ret) + return ret; + + sf_mtd_registered = false; + } sf_mtd_registered = false; memset(&sf_mtd_info, 0, sizeof(sf_mtd_info)); @@ -110,5 +124,24 @@ int spi_flash_mtd_register(struct spi_flash *flash) void spi_flash_mtd_unregister(void) { - del_mtd_device(&sf_mtd_info); + int ret; + + if (!sf_mtd_registered) + return; + + ret = del_mtd_device(&sf_mtd_info); + if (!ret) { + sf_mtd_registered = false; + return; + } + + /* + * Setting mtd->priv to NULL is the best we can do. Thanks to that, + * the MTD layer can still call mtd hooks without risking a + * use-after-free bug. Still, things should be fixed to prevent the + * spi_flash object from being destroyed when del_mtd_device() fails. + */ + sf_mtd_info.priv = NULL; + printf("Failed to unregister MTD %s and the spi_flash object is going away: you're in deep trouble!", + sf_mtd_info.name); } -- cgit v1.2.3