mtd: ubi: behave like a good MTD citizen
cover mtd: ubi: behave like a good MTD citizen
As of now one can easily trigger the existence of "ghost" UBI devices or even worse by removing e.g. the SPI bus driver which previously provided access to an SPI-NAND chip.
Make UBI behave more like other MTD users and create/remove UBI devices based on MTD notifications. As removing could previously fail in case of an ubiblock device still being in use, make sure there are no excuses preventing release of an already gone MTD partition.
This makes it possible to attach UBI devices immediately after the MTD partition becomes available, which will allow using UBI volumes as nvmem-cells provider in the future [1]. Introduce new device tree compatible "linux,ubi" to be used on MTD partitions which should be attached.
In order to make sure ubiblock devices are still created according to the module or kernel parameters, let ubiblock creation from parameters be triggered by UBI_VOLUME_ADDED notifications instead of trying only once during boot.
[1]: https://forum.openwrt.org/t/asus-tuf-ax4200-support/155738/44?u=daniel
Daniel Golle (4):
mtd: ubi: block: don't return on error when removing
mtd: ubi: block: use notifier to create ubiblock from parameter
dt-bindings: mtd: partitions: add linux,ubi
mtd: ubi: attach MTD partition from device-tree
.../bindings/mtd/partitions/ubi.yaml | 49 +++++ drivers/mtd/ubi/block.c | 158 ++++++++------- drivers/mtd/ubi/build.c | 189 ++++++++++++------ drivers/mtd/ubi/cdev.c | 4 +- drivers/mtd/ubi/ubi.h | 6 +- 5 files changed, 271 insertions(+), 135 deletions(-) create mode 100644 Documentation/devicetree/bindings/mtd/partitions/ubi.yaml
base-commit: b5fda08ef213352ac2df7447611eb4d383cce929
Signature · 1 line
2.40.1
PATCH 1/4 mtd: ubi: block: don't return on error when removing
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.
--- 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(-)
drivers/mtd/ubi/block.c +3 -3
@@ -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; }
@@ -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);
drivers/mtd/ubi/cdev.c +1 -1
@@ -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 ; }
drivers/mtd/ubi/ubi.h +2 -2
@@ -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 ; }
Signature · 1 line
2.40.1
PATCH 2/4 mtd: ubi: block: use notifier to create ubiblock from parameter
In reply to Daniel GolleUse UBI_VOLUME_ADDED notification to create ubiblock device specified on kernel cmdline or module parameter.
This makes things more simple and has the advantage that ubiblock devices on volumes which are not present at the time the ubi module is probed will still be created.
--- drivers/mtd/ubi/block.c | 150 ++++++++++++++++++++++------------------ 1 file changed, 82 insertions(+), 68 deletions(-)
drivers/mtd/ubi/block.c +82 -68
@@ -33,6 +33,7 @@#include <linux/kernel.h> #include <linux/list.h> #include <linux/mutex.h> +#include <linux/namei.h> #include <linux/slab.h> #include <linux/mtd/ubi.h> #include <linux/blkdev.h>
@@ -65,10 +66,10 @@ struct ubiblock_pdu { };/* Numbers of elements set in the @ubiblock_param array */ -static int ubiblock_devs__initdata ; +static int ubiblock_devs;/* MTD devices specification parameters */ -static struct ubiblock_param ubiblock_param[UBIBLOCK_MAX_DEVICES ]__initdata ; +static struct ubiblock_param ubiblock_param[UBIBLOCK_MAX_DEVICES ];struct ubiblock {struct ubi_volume_desc *desc;
@@ -533,6 +534,83 @@ static int ubiblock_resize(struct ubi_volume_info *vi)return 0 ; } +static bool +match_volume_desc (struct ubi_volume_info *vi,const char *name,int ubi_num,int vol_id) +{ +int err, len; +struct path path; +struct kstat stat; + +if (ubi_num ==-1 ) { +/* No ubi num, name must be a vol device path */ + err =kern_path (name,LOOKUP_FOLLOW , &path); +if (err) +return false; + + err =vfs_getattr (&path, &stat,STATX_TYPE ,AT_STATX_SYNC_AS_STAT ); +path_put (&path); +if (err) +return false; + +if (!S_ISCHR (stat.mode)) +return false; + +if (vi->ubi_num !=ubi_major2num (MAJOR (stat.rdev))) +return false; + +if (vi->vol_id !=MINOR (stat.rdev) -1 ) +return false; + +return true; + }else if (vol_id ==-1 ) { +if (vi->ubi_num != ubi_num) +return false; + + len =strnlen (name,UBI_VOL_NAME_MAX +1 ); +if (len <1 || vi->name_len != len) +return false; + +if (strcmp (name, vi->name)) +return false; + +return true; + }else { +if (vi->ubi_num != ubi_num) +return false; + +if (vi->vol_id != vol_id) +return false; + +return true; + } +} + +static void +ubiblock_create_from_param (struct ubi_volume_info *vi) +{ +int i, ret =0 ; +struct ubiblock_param *p; + +/* +* Iterate over ubiblock cmdline parameters. If a parameter matches the +* newly added volume create the ubiblock device for it. +*/ +for (i =0 ; i < ubiblock_devs; i++) { + p = &ubiblock_param[i]; + +if (!match_volume_desc (vi, p->name, p->ubi_num, p->vol_id)) +continue ; + + ret =ubiblock_create (vi); +if (ret) { +pr_err ( +"UBI: block: can't add '%s' volume on ubi%d_%d, err=%d\n" , + vi->name, p->ubi_num, p->vol_id, ret); + } +break ; + } +} +static int ubiblock_notify (struct notifier_block *nb,unsigned long notification_type,void *ns_ptr) {
@@ -540,10 +618,7 @@ static int ubiblock_notify(struct notifier_block *nb,switch (notification_type) {case UBI_VOLUME_ADDED : -/* -* We want to enforce explicit block device creation for -* volumes, so when a volume is added we do nothing. -*/ +ubiblock_create_from_param (&nt->vi);break ;case UBI_VOLUME_REMOVED :ubiblock_remove (&nt->vi, true);
@@ -569,56 +644,6 @@ static struct notifier_block ubiblock_notifier = { .notifier_call = ubiblock_notify, }; -static struct ubi_volume_desc *__init -open_volume_desc (const char *name,int ubi_num,int vol_id) -{ -if (ubi_num ==-1 ) -/* No ubi num, name must be a vol device path */ -return ubi_open_volume_path (name,UBI_READONLY ); -else if (vol_id ==-1 ) -/* No vol_id, must be vol_name */ -return ubi_open_volume_nm (ubi_num, name,UBI_READONLY ); -else -return ubi_open_volume (ubi_num, vol_id,UBI_READONLY ); -} - -static void __init ubiblock_create_from_param (void ) -{ -int i, ret =0 ; -struct ubiblock_param *p; -struct ubi_volume_desc *desc; -struct ubi_volume_info vi; - -/* -* If there is an error creating one of the ubiblocks, continue on to -* create the following ubiblocks. This helps in a circumstance where -* the kernel command-line specifies multiple block devices and some -* may be broken, but we still want the working ones to come up. -*/ -for (i =0 ; i < ubiblock_devs; i++) { - p = &ubiblock_param[i]; - - desc =open_volume_desc (p->name, p->ubi_num, p->vol_id); -if (IS_ERR (desc)) { -pr_err ( -"UBI: block: can't open volume on ubi%d_%d, err=%ld\n" , - p->ubi_num, p->vol_id,PTR_ERR (desc)); -continue ; - } - -ubi_get_volume_info (desc, &vi); -ubi_close_volume (desc); - - ret =ubiblock_create (&vi); -if (ret) { -pr_err ( -"UBI: block: can't add '%s' volume on ubi%d_%d, err=%d\n" , - vi.name, p->ubi_num, p->vol_id, ret); -continue ; - } - } -} -static void ubiblock_remove_all (void ) {struct ubiblock *next;
@@ -644,18 +669,7 @@ int __init ubiblock_init(void)if (ubiblock_major <0 )return ubiblock_major; -/* -* Attach block devices from 'block=' module param. -* Even if one block device in the param list fails to come up, -* still allow the module to load and leave any others up. -*/ -ubiblock_create_from_param (); - -/* -* Block devices are only created upon user requests, so we ignore -* existing volumes. -*/ - ret =ubi_register_volume_notifier (&ubiblock_notifier,1 ); + ret =ubi_register_volume_notifier (&ubiblock_notifier,0 );if (ret) goto err_unreg;return 0 ;
Signature · 1 line
2.40.1
PATCH 3/4 dt-bindings: mtd: partitions: add linux,ubi compatible
In reply to Daniel GolleAdd bindings for MTD partitions to be attached as UBI devices.
--- .../bindings/mtd/partitions/ubi.yaml | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 Documentation/devicetree/bindings/mtd/partitions/ubi.yaml
Documentation/devicetree/bindings/mtd/partitions/ubi.yaml new file +49 -0
@@ -0,0 +1,49 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mtd/partitions/ubi.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Unsorted Block Images (UBI) + +description: | + Unsorted Block Images (UBI) is a volume management system typically + used on NAND flash providing bad block management as well as + wear-leveling. + Any partition containing the compatible "linux,ubi" will be attached + as UBI device. + +maintainers: + - Daniel Golle <[email protected]> + +allOf: + - $ref: /schemas/mtd/partitions/partition.yaml# + +properties: + compatible: + const: linux,ubi + +required: + - compatible + +unevaluatedProperties: false + +examples: + - | + partitions { + compatible = "fixed-partitions"; + #address-cells = <1>; + #size-cells = <1>; + + partition@0 { + label = "bootloader"; + reg = <0x000000 0x100000>; + read-only; + }; + + ubi@100000 { + compatible = "linux,ubi"; + label = "ubi"; + reg = <0x100000 0x7f00000>; + }; + };
Signature · 6 lines
2.40.1 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
PATCH 4/4 mtd: ubi: attach MTD partition from device-tree
In reply to Daniel GolleSplit ubi_init() function into early function to be called by device_initcall() and keep cmdline attachment in late_initcall(). (when building ubi as module, both is still done in a single module_init() call)
Register MTD notifier and attach MTD devices which are marked as compatible with 'linux,ubi' in OF device-tree when being added, detach UBI device from MTD device when it is being removed.
For existing users this should not change anything besides automatic removal of (dead) UBI devices when their underlying MTD devices are already gone, e.g. in case of MTD driver module or (SPI) bus driver module being removed.
For new users this opens up the option to attach UBI using device-tree which then happens early and in parallel with other drivers being probed which slightly reduces the total boot time.
Attachment no longer happening late is also a requirement for other drivers to make use of UBI, e.g. drivers/nvmem/u-boot-env.c can now be extended to support U-Boot environment stored in UBI volumes.
--- drivers/mtd/ubi/block.c | 2 +- drivers/mtd/ubi/build.c | 189 ++++++++++++++++++++++++++++------------ drivers/mtd/ubi/cdev.c | 2 +- drivers/mtd/ubi/ubi.h | 2 +- 4 files changed, 134 insertions(+), 61 deletions(-)
drivers/mtd/ubi/block.c +1 -1
@@ -471,7 +471,7 @@ int ubiblock_remove(struct ubi_volume_info *vi, bool force) }/* Found a device, let's lock it so we can check if it's busy */ -mutex_lock (&dev->dev_mutex); +mutex_lock_nested (&dev->dev_mutex,SINGLE_DEPTH_NESTING );if (dev->refcnt >0 && !force) { ret = -EBUSY ; goto out_unlock_dev;
drivers/mtd/ubi/build.c +111 -41
223 lines not on this page · raw message
drivers/mtd/ubi/cdev.c +1 -1
@@ -1065,7 +1065,7 @@ static long ctrl_cdev_ioctl(struct file *file, unsigned int cmd, }mutex_lock (&ubi_devices_mutex); - err =ubi_detach_mtd_dev (ubi_num,0 ); + err =ubi_detach_mtd_dev (ubi_num,0 , false);mutex_unlock (&ubi_devices_mutex);break ; }
drivers/mtd/ubi/ubi.h +1 -1
@@ -939,7 +939,7 @@ int ubi_io_write_vid_hdr(struct ubi_device *ubi, int pnum,int ubi_attach_mtd_dev (struct mtd_info *mtd,int ubi_num,int vid_hdr_offset,int max_beb_per1024,bool disable_fm); -int ubi_detach_mtd_dev (int ubi_num,int anyway); +int ubi_detach_mtd_dev (int ubi_num,int anyway,bool have_lock);struct ubi_device *ubi_get_device (int ubi_num);void ubi_put_device (struct ubi_device *ubi);struct ubi_device *ubi_get_by_major (int major);
Signature · 6 lines
2.40.1 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Zhihao Cheng
reviewerRe: [PATCH 1/4] mtd: ubi: block: don't return on error when removing
In reply to Daniel GolleQuoted 29 lines · Daniel Golle
There is no point on returning the error from
ubiblock_removein 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_removeto go on even in case the ubiblock device is still busy. Use that new option when callingubiblock_removedue to aUBI_VOLUME_REMOVEDevent.Signed-off-by Daniel Golle <[email protected]>--- 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(-)drivers/mtd/ubi/block.c +1
-1@@ -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;
drivers/mtd/ubi/block.c int ubiblock_remove(struct ubi_volume_info *vi) · line 477
Quoted 8 lines
drivers/mtd/ubi/block.c +1
-1@@ -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.
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.
Quoted 45 lines
drivers/mtd/ubi/block.c +1
-1@@ -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);drivers/mtd/ubi/cdev.c +1
-1@@ -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 ; }drivers/mtd/ubi/ubi.h +2
-2@@ -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 ; }
Rob Herring
maintainerRe: [PATCH 3/4] dt-bindings: mtd: partitions: add linux,ubi compatible
In reply to Daniel GolleQuoted 5 lines · Daniel Golle
Add bindings for MTD partitions to be attached as UBI devices.
Signed-off-by Daniel Golle <[email protected]>--- .../bindings/mtd/partitions/ubi.yaml | 49 +++++++++++++++++++
linux,ubi.yaml
Quoted 61 lines
1 file changed, 49 insertions(+) create mode 100644 Documentation/devicetree/bindings/mtd/partitions/ubi.yamlDocumentation/devicetree/bindings/mtd/partitions/ubi.yaml new file +49
-0@@ -0,0 +1,49 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mtd/partitions/ubi.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Unsorted Block Images (UBI) + +description: | + Unsorted Block Images (UBI) is a volume management system typically + used on NAND flash providing bad block management as well as + wear-leveling. + Any partition containing the compatible "linux,ubi" will be attached + as UBI device. + +maintainers: + - Daniel Golle <[email protected]> + +allOf: + - $ref: /schemas/mtd/partitions/partition.yaml# + +properties: + compatible: + const: linux,ubi + +required: + - compatible + +unevaluatedProperties: false + +examples: + - | + partitions { + compatible = "fixed-partitions"; + #address-cells = <1>; + #size-cells = <1>; + + partition@0 { + label = "bootloader"; + reg = <0x000000 0x100000>; + read-only; + }; + + ubi@100000 { + compatible = "linux,ubi"; + label = "ubi"; + reg = <0x100000 0x7f00000>; + }; + };Signature · 1 line
2.40.1
Re: [PATCH 1/4] mtd: ubi: block: don't return on error when removing
In reply to Zhihao ChengQuoted 55 lines · Zhihao Cheng
Quoted 28 lines · Daniel Golle
There is no point on returning the error from
ubiblock_removein 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_removeto go on even in case the ubiblock device is still busy. Use that new option when callingubiblock_removedue to aUBI_VOLUME_REMOVEDevent.Signed-off-by Daniel Golle <[email protected]>--- 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(-)drivers/mtd/ubi/block.c +1
-1@@ -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;drivers/mtd/ubi/block.c int ubiblock_remove(struct ubi_volume_info *vi) · line 476
Quoted 7 lines
drivers/mtd/ubi/block.c +1
-1@@ -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 byubi_notify_remove(), later volume accessing by the application will trigger an UAF problem in kernel.
Appdriver_remove
fd =ubi_open_volumeubi_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!
Quoted 4 lines
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...
Quoted 6 lines
After looking at
nvme_free_ctrl, I found thatnvme_devis released when device refcnt becomes zero, so block device andnvme_devwon'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...
Quoted 36 lines
Quoted 36 lines
drivers/mtd/ubi/block.c +1
-1@@ -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);drivers/mtd/ubi/cdev.c +1
-1@@ -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 ; }drivers/mtd/ubi/ubi.h +1
-1@@ -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) {
Quoted 9 lines
drivers/mtd/ubi/ubi.h static inline int ubiblock_create(struct ubi_volume_info *vi) · line 993
Quoted 8 lines
drivers/mtd/ubi/ubi.h +1
-1return -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 ; }
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Zhihao Cheng
reviewerRe: [PATCH 1/4] mtd: ubi: block: don't return on error when removing
Quoted 56 lines · Daniel Golle
Quoted 55 lines · Zhihao Cheng
Quoted 28 lines · Daniel Golle
There is no point on returning the error from
ubiblock_removein 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_removeto go on even in case the ubiblock device is still busy. Use that new option when callingubiblock_removedue to aUBI_VOLUME_REMOVEDevent.Signed-off-by Daniel Golle <[email protected]>--- 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(-)drivers/mtd/ubi/block.c +1
-1@@ -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;drivers/mtd/ubi/block.c int ubiblock_remove(struct ubi_volume_info *vi) · line 476
Quoted 7 lines
drivers/mtd/ubi/block.c +1
-1@@ -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 byubi_notify_remove(), later volume accessing by the application will trigger an UAF problem in kernel.
Appdriver_remove
fd =ubi_open_volumeubi_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!
Quoted 9 lines
Quoted 4 lines
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.
Yes, the second UAF situation seems to exist a long time, maybe disabling ubi device in ubi_notify_remove is a temp solution? Importing new features based on the framework with known issues looks a little weird, I suggest to solve the problem of mtd lifetime management before applying this new feature. But I'm okay to this feature if maintainer doesn't care about this problem.
Quoted 5 lines
An idea to get closer to a good state would be to try dropping the 'anyway' parameter from
ubi_detach_mtd_devwhich is currently only used in themodule_exit. To avoid this, we should make sure the module's refcnt is increased/decreased together withubi->ref_count.
Yes. Dropping 'anyway' param from ubi_detach_mtd_dev in ubi_notify_remove can avoid the first UAF problem happening.
Quoted 88 lines
When it comes to the to-be-introduced
ubi_notify_removewe still face another problem, see below...Quoted 6 lines
After looking at
nvme_free_ctrl, I found thatnvme_devis released when device refcnt becomes zero, so block device andnvme_devwon'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.hstruct mtd_notifier { void (*add)(struct mtd_info *mtd); void (*remove)(struct mtd_info *mtd); struct list_head list; };Also see
del_mtd_deviceindrivers/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...
Quoted 36 lines
Quoted 36 lines
drivers/mtd/ubi/block.c +1
-1@@ -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);drivers/mtd/ubi/cdev.c +1
-1@@ -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 ; }drivers/mtd/ubi/ubi.h +1
-1@@ -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) {Quoted 9 lines
drivers/mtd/ubi/ubi.h static inline int ubiblock_create(struct ubi_volume_info *vi) · line 993
Quoted 8 lines
drivers/mtd/ubi/ubi.h +1
-1return -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 ; }.