[U-Boot] [PATCH v3 11/11] mtd: sf: Make sf_mtd.c more robust

[Jagan Teki]

On Tue, Nov 20, 2018 at 2:33 AM Boris Brezillon
<boris.brezillon at bootlin.com> wrote:
>
> 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 <boris.brezillon at bootlin.com>
> ---
> Changes in v3:
> - New patch
> ---
>  drivers/mtd/spi/sf_mtd.c | 39 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/spi/sf_mtd.c b/drivers/mtd/spi/sf_mtd.c
> index aabbc3589435..68c36002bee2 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);

Why do we need this print? can't we do the same thing in MTD core
itself, so-that it can be generic for all flash objects.