Messages in this thread Patch in this message | | | Date | Wed, 24 May 2023 10:41:24 +0100 | From | Daniel Golle <> | Subject | Re: [PATCH 1/4] mtd: ubi: block: don't return on error when removing |
| |
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@makrotopia.org> > > --- > > 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; > > } > > >
| |