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

[Zhihao Cheng]

在 2023/10/12 17:31, ZhaoLong Wang 写道:
> > > > 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.
>
> Setting the desc to NULL works because
> mutex_lock "mtd_table_mutex" is held on all paths where
> ftl_add_mtd() is called.

Oh, you're right. Just one nit below:


> @@ -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;

This 'unlikey' can be removed.

Rename 'isnt_get' as 'has_desc' ?

> +    /**
> +     * 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;
> +        }
> +    }