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

[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.


ubi_gluebi_init
  ubi_register_volume_notifier
    ubi_enumerate_volumes
      ubi_notify_all
        gluebi_notify    nb->notifier_call()
          gluebi_create
            mtd_device_register
              mtd_device_parse_register
                add_mtd_device
                  mutex_lock(&mtd_table_mutex);
                  blktrans_notify_add   not->add()
                    ftl_add_mtd         tr->add_mtd()
                      scan_header
                        mtd_read
                          mtd_read
                            mtd_read_oob
                              gluebi_read   mtd->read()
                                gluebi->desc - NULL
                  mutex_unlock(&mtd_table_mutex);

ubi_cdev_ioctl
  ubi_create_volume
    ubi_volume_notify
      blocking_notifier_call_chain  [kernel/notifier.c]
        notifier_call_chain
          gluebi_notify   nb->notifier_call()
            gluebi_create
              mtd_device_register
                mtd_device_parse_register
                  add_mtd_device
                    mutex_lock(&mtd_table_mutex);
                    blktrans_notify_add   not->add()
                      ftl_add_mtd     tr->add_mtd()
                        scan_header
                          mtd_read(part->mbd.mtd,
                            mtd_read_oob
                              gluebi_read   mtd->read()
                                ubi_read(gluebi->desc
                    mutex_unlock(&mtd_table_mutex);

load_module ftl
  register_mtd_blktrans
    mutex_lock(&mtd_table_mutex);
    ftl_add_mtd  not->add()
      scan_header
        mtd_read(part->mbd.mtd,
          mtd_read_oob
            gluebi_read   mtd->read()
              ubi_read(gluebi->desc
    mutex_unlock(&mtd_table_mutex);

This lock is also held during the process of closing the file.

close(fd)
  mtdchar_close
    put_mtd_device
      mutex_lock(&mtd_table_mutex);
      __put_mtd_device
        gluebi_put_device
          ubi_close_volume
            kfree(desc)
            // desc == NULL
      mutex_unlock(&mtd_table_mutex);