Re: [PATCH RFC] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier

[Zhihao Cheng]

在 2023/10/12 16:04, ZhaoLong Wang 写道:
> I'm very happy to receive a reply to the review.
>
> > 2. fd = open(/dev/ubi0_0, O_WRONLY)
> >      ubi_open_volume  // vol->writers = 1
> >
> >           P1                    P2
> >     gluebi_create -> mtd_device_register -> add_mtd_device:
> >     device_register   // dev/mtd1 is visible
> >
> >                       fd = open(/dev/mtd1, O_WRONLY)
> >                        gluebi_get_device
> >                         gluebi->desc = ubi_open_volume
> >                          gluebi->desc = ERR_PTR(EBUSY)
> >
> >     ftl_add_mtd
> >      mtd_read
> >       gluebi_read
> >        gluebi->desc is ERR_PTR       (√)
>
> The reproduction steps for situations 2 and 3 have been added to link[1].
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217992 [1]
>
> > 3.         P1                    P2
> >     gluebi_create -> mtd_device_register -> add_mtd_device:
> >     device_register   // dev/mtd1 is visible
> >
> >                       fd = open(/dev/mtd1, O_WRONLY)
> >                        gluebi_get_device
> >                         gluebi->desc = ubi_open_volume
> >
> >     ftl_add_mtd
> >      mtd_read
> >       gluebi_read
> >        gluebi->desc is not ERR_PTR/NULL
> >
> >                      close(fd)
> >                       gluebi_put_device
> >                        ubi_close_volume
> >                         kfree(desc)
> >        ubi_read(gluebi->desc)   // UAF  (×)
>
> Yes, it's also a problem. Perhaps it should be set to NULL after
> destroying gluebi->desc.

The key point is that 'gluebi->desc' check & usage is not atomic in 
gluebi_read. So following patch still can't handle situation 3.

> > No need to modify 'gluebi_write' and 'gluebi_erase'.
>
> The patch is as follows:
>
> diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c
> index 1b980d15d9fb..8fc6017d1155 100644
> --- a/drivers/mtd/ubi/gluebi.c
> +++ b/drivers/mtd/ubi/gluebi.c
> @@ -85,6 +85,7 @@ static int gluebi_get_device(struct mtd_info *mtd)
>   {
>       struct gluebi_device *gluebi;
>       int ubi_mode = UBI_READONLY;
> +    struct ubi_volume_desc *vdesc;
>
>       if (mtd->flags & MTD_WRITEABLE)
>           ubi_mode = UBI_READWRITE;
> @@ -109,12 +110,14 @@ static int gluebi_get_device(struct mtd_info *mtd)
>        * This is the first reference to this UBI volume via the MTD device
>        * interface. Open the corresponding volume in read-write mode.
>        */
> -    gluebi->desc = ubi_open_volume(gluebi->ubi_num, gluebi->vol_id,
> +    vdesc = ubi_open_volume(gluebi->ubi_num, gluebi->vol_id,
>                          ubi_mode);
> -    if (IS_ERR(gluebi->desc)) {
> +    if (IS_ERR(vdesc)) {
> +        gluebi->desc = NULL;
>           mutex_unlock(&devices_mutex);
> -        return PTR_ERR(gluebi->desc);
> +        return PTR_ERR(vdesc);
>       }
> +    gluebi->desc = vdesc;
>       gluebi->refcnt += 1;
>       mutex_unlock(&devices_mutex);
>       return 0;
> @@ -134,8 +137,10 @@ static void gluebi_put_device(struct mtd_info *mtd)
>       gluebi = container_of(mtd, struct gluebi_device, mtd);
>       mutex_lock(&devices_mutex);
>       gluebi->refcnt -= 1;
> -    if (gluebi->refcnt == 0)
> +    if (gluebi->refcnt == 0) {
>           ubi_close_volume(gluebi->desc);
> +        gluebi->desc = NULL;
> +    }
>       mutex_unlock(&devices_mutex);
>   }
>
> @@ -154,9 +159,26 @@ static int gluebi_read(struct mtd_info *mtd, loff_t 
> from, size_t len,
>                  size_t *retlen, unsigned char *buf)
>   {
>       int err = 0, lnum, offs, bytes_left;
> -    struct gluebi_device *gluebi;
> +    struct gluebi_device *gluebi = container_of(mtd, struct gluebi_device,
> +                            mtd);
> +    int isnt_get = unlikely(gluebi->desc == NULL) ? 1 : 0;
> +
> +    /**
> +     * In normal case, the UBI volume desc has been initialized by
> +     * ->_get_device(). However, in the ftl notifier process, the
> +     * ->_get_device() is not executed in advance and the MTD device
> +     * is directly scanned  which cause null pointe dereference.
> +     * Therefore, try to get the MTD device here.
> +     */
> +    if (unlikely(isnt_get)) {
> +        err = __get_mtd_device(mtd);
> +        if (err) {
> +            err_msg("cannot get MTD device %d, UBI device %d, volume 
> %d, error %d",
> +                mtd->index, gluebi->ubi_num, gluebi->vol_id, err);
> +            return err;
> +        }
> +    }
>
> -    gluebi = container_of(mtd, struct gluebi_device, mtd);
>       lnum = div_u64_rem(from, mtd->erasesize, &offs);
>       bytes_left = len;
>       while (bytes_left) {
> @@ -176,6 +198,9 @@ static int gluebi_read(struct mtd_info *mtd, loff_t 
> from, size_t len,
>       }
>
>       *retlen = len - bytes_left;
> +
> +    if (unlikely(isnt_get))
> +        __put_mtd_device(mtd);
>       return err;
>   }
>
>
>
> .