Re: [PATCH 1/4] mtd: ubi: block: don't return on error when removing
From: Daniel Golle
Date: Wed May 24 2023 - 05:42:34 EST
On Wed, May 03, 2023 at 09:09:49PM +0800, Zhihao Cheng wrote:
> 在 2023/5/3 0:48, Daniel Golle 写道:
> > There is no point on returning the error from ubiblock_remove in case
> > it is being called due to a volume removal event -- the volume is gone,
> > we should destroy and remove the ubiblock device no matter what.
> >
> > Introduce a new boolean parameter 'force' to tell ubiblock_remove to go
> > on even in case the ubiblock device is still busy. Use that new option
> > when calling ubiblock_remove due to a UBI_VOLUME_REMOVED event.
> >
> > Signed-off-by: Daniel Golle <daniel@xxxxxxxxxxxxxx>
> > ---
> > drivers/mtd/ubi/block.c | 6 +++---
> > drivers/mtd/ubi/cdev.c | 2 +-
> > drivers/mtd/ubi/ubi.h | 4 ++--
> > 3 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
> > index 3711d7f746003..6f5804f4b8f55 100644
> > --- a/drivers/mtd/ubi/block.c
> > +++ b/drivers/mtd/ubi/block.c
> > @@ -457,7 +457,7 @@ static void ubiblock_cleanup(struct ubiblock *dev)
> > idr_remove(&ubiblock_minor_idr, dev->gd->first_minor);
> > }
> > -int ubiblock_remove(struct ubi_volume_info *vi)
> > +int ubiblock_remove(struct ubi_volume_info *vi, bool force)
> > {
> > struct ubiblock *dev;
> > int ret;
> > @@ -471,7 +471,7 @@ int ubiblock_remove(struct ubi_volume_info *vi)
> > /* Found a device, let's lock it so we can check if it's busy */
> > mutex_lock(&dev->dev_mutex);
> > - if (dev->refcnt > 0) {
> > + if (dev->refcnt > 0 && !force) {
> > ret = -EBUSY;
> > goto out_unlock_dev;
> > }
>
> After looking through this series, I think we should pay attention to one
> problem: The lifetime of mtd device and ubi things(ubi device/volume/block
> device). It's difficult to decide whether or not to destroy ubi things when
> mtd driver is removed.
> If we destroy ubi things, one application may have opened an ubi volume
> early, then ubi device and all its volumes are destroyed by
> ubi_notify_remove(), later volume accessing by the application will trigger
> an UAF problem in kernel.
> App driver_remove
> fd = ubi_open_volume
> ubi_notify_remove
> ubi_detach_mtd_dev
> vfree(ubi->vtbl)
> ioctl(fd, UBI_IOCVOLUP)
> ubi_start_update
> set_update_marker
> vtbl_rec = ubi->vtbl[vol->vol_id] // UAF!
>
> If we reserve ubi things even mtd driver is removed. There exists mtd
> drivers releasing mtd device (eg. phram_remove), then upper application
> could accessing released mtd device by the ubi device, which also triggers
> UAF in kernel.
I agree this is a problem, and I also agree it is not a new problem
introduced by this series, but rather already exists in the kernel for
many years.
An idea to get closer to a good state would be to try dropping the
'anyway' parameter from ubi_detach_mtd_dev which is currently only
used in the module_exit. To avoid this, we should make sure the
module's refcnt is increased/decreased together with ubi->ref_count.
When it comes to the to-be-introduced ubi_notify_remove we still
face another problem, see below...
>
> After looking at nvme_free_ctrl, I found that nvme_dev is released when
> device refcnt becomes zero, so block device and nvme_dev won't be freed
> immediately when pci driver removed if upper filesystem being mounted on
> nvme device. And the mtd device's refcnt is held by ubi too, we may follow
> this method, but investigating all mtd drivers looks like unrealistic.
A good start would be deciding on and defining the way it should be.
I agree with your suggestion above, however, also note that in case of
MTD (in contrast to block devices) we have only a 'remove' notification
call returning void, see include/linux/mtd/mtd.h
struct mtd_notifier {
void (*add)(struct mtd_info *mtd);
void (*remove)(struct mtd_info *mtd);
struct list_head list;
};
Also see del_mtd_device in drivers/mtd/mtdcore.c:
[...]
/* No need to get a refcount on the module containing
the notifier, since we hold the mtd_table_mutex */
list_for_each_entry(not, &mtd_notifiers, list)
not->remove(mtd);
if (mtd->usecount) {
printk(KERN_NOTICE "Removing MTD device #%d (%s) with use count %d\n",
mtd->index, mtd->name, mtd->usecount);
ret = -EBUSY;
} else {
[...]
So remove is called despite usecount could still be > 0.
Looks a bit like I've opened a can of worms...
>
> > @@ -546,7 +546,7 @@ static int ubiblock_notify(struct notifier_block *nb,
> > */
> > break;
> > case UBI_VOLUME_REMOVED:
> > - ubiblock_remove(&nt->vi);
> > + ubiblock_remove(&nt->vi, true);
> > break;
> > case UBI_VOLUME_RESIZED:
> > ubiblock_resize(&nt->vi);
> > diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
> > index f43430b9c1e65..bb55e863dd296 100644
> > --- a/drivers/mtd/ubi/cdev.c
> > +++ b/drivers/mtd/ubi/cdev.c
> > @@ -572,7 +572,7 @@ static long vol_cdev_ioctl(struct file *file, unsigned int cmd,
> > struct ubi_volume_info vi;
> > ubi_get_volume_info(desc, &vi);
> > - err = ubiblock_remove(&vi);
> > + err = ubiblock_remove(&vi, false);
> > break;
> > }
> > diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
> > index c8f1bd4fa1008..44c0eeaf1e1b0 100644
> > --- a/drivers/mtd/ubi/ubi.h
> > +++ b/drivers/mtd/ubi/ubi.h
> > @@ -979,7 +979,7 @@ static inline void ubi_fastmap_destroy_checkmap(struct ubi_volume *vol) {}
> > int ubiblock_init(void);
> > void ubiblock_exit(void);
> > int ubiblock_create(struct ubi_volume_info *vi);
> > -int ubiblock_remove(struct ubi_volume_info *vi);
> > +int ubiblock_remove(struct ubi_volume_info *vi, bool force);
> > #else
> > static inline int ubiblock_init(void) { return 0; }
> > static inline void ubiblock_exit(void) {}
> > @@ -987,7 +987,7 @@ static inline int ubiblock_create(struct ubi_volume_info *vi)
> > {
> > return -ENOSYS;
> > }
> > -static inline int ubiblock_remove(struct ubi_volume_info *vi)
> > +static inline int ubiblock_remove(struct ubi_volume_info *vi, bool force)
> > {
> > return -ENOSYS;
> > }
> >
>