| Message ID | 20231018121618.778385-1-wangzhaolong1@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier | expand |
在 2023/10/18 20:16, ZhaoLong Wang 写道: > If both flt.ko and gluebi.ko are loaded, the notiier of ftl > triggers NULL pointer dereference when trying to access > ‘gluebi->desc’ in gluebi_read(). > > 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 > 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 > > Detailed reproduction information available at the link[1], > > In the normal case, obtain gluebi->desc in the gluebi_get_device(), > and accesses gluebi->desc in the gluebi_read(). However, > gluebi_get_device() is not executed in advance in the > ftl_add_mtd() process, which leads to NULL pointer dereference. > > The value of gluebi->desc may also be a negative error code, which > triggers the page fault error. > > This patch has the following modifications: > > 1. Do not assign gluebi->desc to the error code. Use the NULL instead. > > 2. Always check the validity of gluebi->desc in gluebi_read() If the > gluebi->desc is NULL, try to get MTD device. > > Such a modification currently works because the mutex "mtd_table_mutex" > is held on all necessary paths, including the ftl_add_mtd() call path, > open and close paths. Therefore, many race condition can be avoided. > > Fixes: 2ba3d76a1e29 ("UBI: make gluebi a separate module") > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217992 [1] > Signed-off-by: ZhaoLong Wang <wangzhaolong1@huawei.com> > --- > drivers/mtd/ubi/gluebi.c | 37 +++++++++++++++++++++++++++++++------ > 1 file changed, 31 insertions(+), 6 deletions(-) > Reviewed-by: Zhihao Cheng <chengzhihao1@huawei.com> > diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c > index 1b980d15d9fb..0ca7f104adbf 100644 > --- a/drivers/mtd/ubi/gluebi.c > +++ b/drivers/mtd/ubi/gluebi.c > @@ -85,6 +85,7 @@ static int gluebi_get_device(struct mtd_info *mtd) > { > struct gluebi_device *gluebi; > int ubi_mode = UBI_READONLY; > + struct ubi_volume_desc *vdesc; > > if (mtd->flags & MTD_WRITEABLE) > ubi_mode = UBI_READWRITE; > @@ -109,12 +110,14 @@ static int gluebi_get_device(struct mtd_info *mtd) > * This is the first reference to this UBI volume via the MTD device > * interface. Open the corresponding volume in read-write mode. > */ > - gluebi->desc = ubi_open_volume(gluebi->ubi_num, gluebi->vol_id, > + vdesc = ubi_open_volume(gluebi->ubi_num, gluebi->vol_id, > ubi_mode); > - if (IS_ERR(gluebi->desc)) { > + if (IS_ERR(vdesc)) { > + gluebi->desc = NULL; > mutex_unlock(&devices_mutex); > - return PTR_ERR(gluebi->desc); > + return PTR_ERR(vdesc); > } > + gluebi->desc = vdesc; > gluebi->refcnt += 1; > mutex_unlock(&devices_mutex); > return 0; > @@ -134,8 +137,10 @@ static void gluebi_put_device(struct mtd_info *mtd) > gluebi = container_of(mtd, struct gluebi_device, mtd); > mutex_lock(&devices_mutex); > gluebi->refcnt -= 1; > - if (gluebi->refcnt == 0) > + if (gluebi->refcnt == 0) { > ubi_close_volume(gluebi->desc); > + gluebi->desc = NULL; > + } > mutex_unlock(&devices_mutex); > } > > @@ -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 no_desc = gluebi->desc == NULL ? 1 : 0; > + > + /** > + * 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 pointer dereference. > + * Therefore, try to get the MTD device here. > + */ > + if (unlikely(no_desc)) { > + 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; > + } > + } > > - gluebi = container_of(mtd, struct gluebi_device, mtd); > lnum = div_u64_rem(from, mtd->erasesize, &offs); > bytes_left = len; > while (bytes_left) { > @@ -176,6 +198,9 @@ static int gluebi_read(struct mtd_info *mtd, loff_t from, size_t len, > } > > *retlen = len - bytes_left; > + > + if (unlikely(no_desc)) > + __put_mtd_device(mtd); > return err; > } > >
----- Ursprüngliche Mail ----- > Von: "ZhaoLong Wang" <wangzhaolong1@huawei.com> > An: "richard" <richard@nod.at>, "Miquel Raynal" <miquel.raynal@bootlin.com>, "Vignesh Raghavendra" <vigneshr@ti.com>, > dpervushin@embeddedalley.com, "Artem Bityutskiy" <Artem.Bityutskiy@nokia.com> > CC: "linux-mtd" <linux-mtd@lists.infradead.org>, "linux-kernel" <linux-kernel@vger.kernel.org>, "chengzhihao1" > <chengzhihao1@huawei.com>, "ZhaoLong Wang" <wangzhaolong1@huawei.com>, "yi zhang" <yi.zhang@huawei.com>, "yangerkun" > <yangerkun@huawei.com> > Gesendet: Mittwoch, 18. Oktober 2023 14:16:18 > Betreff: [PATCH v2] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier > If both flt.ko and gluebi.ko are loaded, the notiier of ftl > triggers NULL pointer dereference when trying to access > ‘gluebi->desc’ in gluebi_read(). > > 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 > 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 > > Detailed reproduction information available at the link[1], > > In the normal case, obtain gluebi->desc in the gluebi_get_device(), > and accesses gluebi->desc in the gluebi_read(). However, > gluebi_get_device() is not executed in advance in the > ftl_add_mtd() process, which leads to NULL pointer dereference. > > The value of gluebi->desc may also be a negative error code, which > triggers the page fault error. > > This patch has the following modifications: > > 1. Do not assign gluebi->desc to the error code. Use the NULL instead. > > 2. Always check the validity of gluebi->desc in gluebi_read() If the > gluebi->desc is NULL, try to get MTD device. > > Such a modification currently works because the mutex "mtd_table_mutex" > is held on all necessary paths, including the ftl_add_mtd() call path, > open and close paths. Therefore, many race condition can be avoided. I see the problem, but I'm not really satisfied by the solution. Adding this hack to gluebi_read() is not nice at all. Is there a strong reason why have to defer ubi_open_volume() to gluebi_get_device()? Miquel, what do you think? Thanks, //richard
在 2023/10/20 4:27, Richard Weinberger 写道: > ----- Ursprüngliche Mail ----- >> Von: "ZhaoLong Wang" <wangzhaolong1@huawei.com> >> An: "richard" <richard@nod.at>, "Miquel Raynal" <miquel.raynal@bootlin.com>, "Vignesh Raghavendra" <vigneshr@ti.com>, >> dpervushin@embeddedalley.com, "Artem Bityutskiy" <Artem.Bityutskiy@nokia.com> >> CC: "linux-mtd" <linux-mtd@lists.infradead.org>, "linux-kernel" <linux-kernel@vger.kernel.org>, "chengzhihao1" >> <chengzhihao1@huawei.com>, "ZhaoLong Wang" <wangzhaolong1@huawei.com>, "yi zhang" <yi.zhang@huawei.com>, "yangerkun" >> <yangerkun@huawei.com> >> Gesendet: Mittwoch, 18. Oktober 2023 14:16:18 >> Betreff: [PATCH v2] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier > >> If both flt.ko and gluebi.ko are loaded, the notiier of ftl >> triggers NULL pointer dereference when trying to access >> ‘gluebi->desc’ in gluebi_read(). >> >> 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 >> 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 >> >> Detailed reproduction information available at the link[1], >> >> In the normal case, obtain gluebi->desc in the gluebi_get_device(), >> and accesses gluebi->desc in the gluebi_read(). However, >> gluebi_get_device() is not executed in advance in the >> ftl_add_mtd() process, which leads to NULL pointer dereference. >> >> The value of gluebi->desc may also be a negative error code, which >> triggers the page fault error. >> >> This patch has the following modifications: >> >> 1. Do not assign gluebi->desc to the error code. Use the NULL instead. >> >> 2. Always check the validity of gluebi->desc in gluebi_read() If the >> gluebi->desc is NULL, try to get MTD device. >> >> Such a modification currently works because the mutex "mtd_table_mutex" >> is held on all necessary paths, including the ftl_add_mtd() call path, >> open and close paths. Therefore, many race condition can be avoided. > > I see the problem, but I'm not really satisfied by the solution. > Adding this hack to gluebi_read() is not nice at all. Yes, it's jsut a workaround. At the begining, I prefer that increasing volume refcnt (by ubi_open_volume) in gluebi_create and releasing volume refcnt in gluebi_remove. It looks more reasonable that holding a refcnt of UBI volume when gluebi is alive. After looking through the code, the creation/destroying of gluebi is triggered by volume actions(UBI_VOLUME_ADDED/UBI_VOLUME_REMOVED), which means that: 1. gluebi_remove is depended on UBI_VOLUME_REMOVED(triggered by ubi_remove_volume) 2. ubi_remove_volume won't be executed until the refcnt of volume becomes 0(released by gluebi_remove) If we add new ioctls to control creation/destroying of gluebi, then gluebi mtd won't be automatically created when UBI volume is added. I'm not certain whether this change will effect existing startup process that depends on gluebi. > > Is there a strong reason why have to defer ubi_open_volume() to > gluebi_get_device()? > > Miquel, what do you think? > > Thanks, > //richard > > . >
----- Ursprüngliche Mail ----- > Von: "chengzhihao1" <chengzhihao1@huawei.com> >>> Such a modification currently works because the mutex "mtd_table_mutex" >>> is held on all necessary paths, including the ftl_add_mtd() call path, >>> open and close paths. Therefore, many race condition can be avoided. >> >> I see the problem, but I'm not really satisfied by the solution. >> Adding this hack to gluebi_read() is not nice at all. > > Yes, it's jsut a workaround. At the begining, I prefer that increasing > volume refcnt (by ubi_open_volume) in gluebi_create and releasing volume > refcnt in gluebi_remove. It looks more reasonable that holding a refcnt > of UBI volume when gluebi is alive. After looking through the code, the > creation/destroying of gluebi is triggered by volume > actions(UBI_VOLUME_ADDED/UBI_VOLUME_REMOVED), which means that: > 1. gluebi_remove is depended on UBI_VOLUME_REMOVED(triggered by > ubi_remove_volume) > 2. ubi_remove_volume won't be executed until the refcnt of volume > becomes 0(released by gluebi_remove) > > If we add new ioctls to control creation/destroying of gluebi, then > gluebi mtd won't be automatically created when UBI volume is added. I'm > not certain whether this change will effect existing startup process > that depends on gluebi. Let's take a stack back. The sole purpose of gluebi is providing a way to run JFFS2 on top of UBI. IMHO there is no need to run an FTL on top of UBI or even mtdblock. This kind of stacking does not make sense. So, I'd go so far and propose the following: diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c index ff18636e08897..b362a64411ebd 100644 --- a/drivers/mtd/mtd_blkdevs.c +++ b/drivers/mtd/mtd_blkdevs.c @@ -463,7 +463,7 @@ static void blktrans_notify_add(struct mtd_info *mtd) { struct mtd_blktrans_ops *tr; - if (mtd->type == MTD_ABSENT) + if (mtd->type == MTD_ABSENT || mtd->type == MTD_UBIVOLUME) return; list_for_each_entry(tr, &blktrans_majors, list) IOW, no mtdblock (hence, also no FTLs) on top of gluebi. What do you guys think? Thanks, //richard
> > IOW, no mtdblock (hence, also no FTLs) on top of gluebi. > > What do you guys think? > > Thanks, > //richard > JFFS2 needs to work on the block device, so the mtdblock needs to work on the top of gluebi. This is directly reflected in the jffs2 mount command, such as # mount -t jffs2 /dev/mtdblockX /mnt/jffs2
----- Ursprüngliche Mail ----- > Von: "ZhaoLong Wang" <wangzhaolong1@huawei.com> > An: "richard" <richard@nod.at>, "chengzhihao1" <chengzhihao1@huawei.com> > CC: "Miquel Raynal" <miquel.raynal@bootlin.com>, "Vignesh Raghavendra" <vigneshr@ti.com>, "dpervushin" > <dpervushin@embeddedalley.com>, "Artem Bityutskiy" <Artem.Bityutskiy@nokia.com>, "linux-mtd" > <linux-mtd@lists.infradead.org>, "linux-kernel" <linux-kernel@vger.kernel.org>, "yi zhang" <yi.zhang@huawei.com>, > "yangerkun" <yangerkun@huawei.com> > Gesendet: Montag, 23. Oktober 2023 08:41:12 > Betreff: Re: [PATCH v2] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier >> >> IOW, no mtdblock (hence, also no FTLs) on top of gluebi. >> >> What do you guys think? >> >> Thanks, >> //richard >> > > > JFFS2 needs to work on the block device, so the mtdblock needs to work > on the top of gluebi. > > This is directly reflected in the jffs2 mount command, such as > > # mount -t jffs2 /dev/mtdblockX /mnt/jffs2 The limitation is long gone. It comes from the dark and old times where Linux was only able to mount block devices. You can do: mount -t jffs2 mtdX /mnt/jffs2 Thanks, //richard
在 2023/10/22 0:09, Richard Weinberger 写道: > ----- Ursprüngliche Mail ----- >> Von: "chengzhihao1" <chengzhihao1@huawei.com> >>>> Such a modification currently works because the mutex "mtd_table_mutex" >>>> is held on all necessary paths, including the ftl_add_mtd() call path, >>>> open and close paths. Therefore, many race condition can be avoided. >>> >>> I see the problem, but I'm not really satisfied by the solution. >>> Adding this hack to gluebi_read() is not nice at all. >> >> Yes, it's jsut a workaround. At the begining, I prefer that increasing >> volume refcnt (by ubi_open_volume) in gluebi_create and releasing volume >> refcnt in gluebi_remove. It looks more reasonable that holding a refcnt >> of UBI volume when gluebi is alive. After looking through the code, the >> creation/destroying of gluebi is triggered by volume >> actions(UBI_VOLUME_ADDED/UBI_VOLUME_REMOVED), which means that: >> 1. gluebi_remove is depended on UBI_VOLUME_REMOVED(triggered by >> ubi_remove_volume) >> 2. ubi_remove_volume won't be executed until the refcnt of volume >> becomes 0(released by gluebi_remove) >> >> If we add new ioctls to control creation/destroying of gluebi, then >> gluebi mtd won't be automatically created when UBI volume is added. I'm >> not certain whether this change will effect existing startup process >> that depends on gluebi. > > Let's take a stack back. The sole purpose of gluebi is providing > a way to run JFFS2 on top of UBI. Is it possible that someone runs ext4 on mtdblock based on gluebi, for the advantage of wear-leveling? > IMHO there is no need to run an FTL on top of UBI or even mtdblock. > This kind of stacking does not make sense. > > So, I'd go so far and propose the following: > diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c > index ff18636e08897..b362a64411ebd 100644 > --- a/drivers/mtd/mtd_blkdevs.c > +++ b/drivers/mtd/mtd_blkdevs.c > @@ -463,7 +463,7 @@ static void blktrans_notify_add(struct mtd_info *mtd) > { > struct mtd_blktrans_ops *tr; > > - if (mtd->type == MTD_ABSENT) > + if (mtd->type == MTD_ABSENT || mtd->type == MTD_UBIVOLUME) > return; > > list_for_each_entry(tr, &blktrans_majors, list) > > IOW, no mtdblock (hence, also no FTLs) on top of gluebi. > > What do you guys think? > > Thanks, > //richard > > . >
> ----- Ursprüngliche Mail ----- >> Von: "ZhaoLong Wang" <wangzhaolong1@huawei.com> >> An: "richard" <richard@nod.at>, "chengzhihao1" <chengzhihao1@huawei.com> >> CC: "Miquel Raynal" <miquel.raynal@bootlin.com>, "Vignesh Raghavendra" <vigneshr@ti.com>, "dpervushin" >> <dpervushin@embeddedalley.com>, "Artem Bityutskiy" <Artem.Bityutskiy@nokia.com>, "linux-mtd" >> <linux-mtd@lists.infradead.org>, "linux-kernel" <linux-kernel@vger.kernel.org>, "yi zhang" <yi.zhang@huawei.com>, >> "yangerkun" <yangerkun@huawei.com> >> Gesendet: Montag, 23. Oktober 2023 08:41:12 >> Betreff: Re: [PATCH v2] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier > >>> >>> IOW, no mtdblock (hence, also no FTLs) on top of gluebi. >>> >>> What do you guys think? >>> >>> Thanks, >>> //richard >>> >> >> >> JFFS2 needs to work on the block device, so the mtdblock needs to work >> on the top of gluebi. >> >> This is directly reflected in the jffs2 mount command, such as >> >> # mount -t jffs2 /dev/mtdblockX /mnt/jffs2 > > The limitation is long gone. It comes from the dark and old times > where Linux was only able to mount block devices. > > You can do: mount -t jffs2 mtdX /mnt/jffs2 > > Thanks, > //richard > Yes, I tried it and it worked. Thanks for the reminder. But this modification rejects this usage. and rejects other block device file systems (such as ext4) from working on gluebi. Thank you. Zhaolong
----- Ursprüngliche Mail ----- > Von: "chengzhihao1" <chengzhihao1@huawei.com> >>> If we add new ioctls to control creation/destroying of gluebi, then >>> gluebi mtd won't be automatically created when UBI volume is added. I'm >>> not certain whether this change will effect existing startup process >>> that depends on gluebi. >> >> Let's take a stack back. The sole purpose of gluebi is providing >> a way to run JFFS2 on top of UBI. > > Is it possible that someone runs ext4 on mtdblock based on gluebi, for > the advantage of wear-leveling? Unless they want to kill their NAND immediately, no. UBIblock hat initially an RW-Mode but it was rejected because UBI is not an FTL and will all dangerous setups. What I'm trying to say is, I'd like to avoid adding complicated solutions or workarounds just to make artificial stacking of outdated MTD components possible. Let's keep the big picture in mind. Thanks, //richard
----- Ursprüngliche Mail ----- > Von: "ZhaoLong Wang" <wangzhaolong1@huawei.com> > But this modification rejects this usage. and rejects other block device > file systems (such as ext4) from working on gluebi. Are you using ext4 on top of gluebi? Please explain in detail why. Thanks, //richard
在 2023/10/23 15:15, Richard Weinberger 写道: > ----- Ursprüngliche Mail ----- >> Von: "chengzhihao1" <chengzhihao1@huawei.com> >>>> If we add new ioctls to control creation/destroying of gluebi, then >>>> gluebi mtd won't be automatically created when UBI volume is added. I'm >>>> not certain whether this change will effect existing startup process >>>> that depends on gluebi. >>> Let's take a stack back. The sole purpose of gluebi is providing >>> a way to run JFFS2 on top of UBI. >> Is it possible that someone runs ext4 on mtdblock based on gluebi, for >> the advantage of wear-leveling? > Unless they want to kill their NAND immediately, no. > UBIblock hat initially an RW-Mode but it was rejected because > UBI is not an FTL and will all dangerous setups. > > What I'm trying to say is, I'd like to avoid adding complicated solutions > or workarounds just to make artificial stacking of outdated MTD components > possible. > Let's keep the big picture in mind. Makes sense. Zhaolong and I didn't make clear the boundary between FTL and ubiblock/gluebi. Now, I believe that UBI needn't be responsible much for the extension mechanism(eg. mtdblock based on gluebi). So, let's pick your solution. > Thanks, > //richard > > .
diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c index 1b980d15d9fb..0ca7f104adbf 100644 --- a/drivers/mtd/ubi/gluebi.c +++ b/drivers/mtd/ubi/gluebi.c @@ -85,6 +85,7 @@ static int gluebi_get_device(struct mtd_info *mtd) { struct gluebi_device *gluebi; int ubi_mode = UBI_READONLY; + struct ubi_volume_desc *vdesc; if (mtd->flags & MTD_WRITEABLE) ubi_mode = UBI_READWRITE; @@ -109,12 +110,14 @@ static int gluebi_get_device(struct mtd_info *mtd) * This is the first reference to this UBI volume via the MTD device * interface. Open the corresponding volume in read-write mode. */ - gluebi->desc = ubi_open_volume(gluebi->ubi_num, gluebi->vol_id, + vdesc = ubi_open_volume(gluebi->ubi_num, gluebi->vol_id, ubi_mode); - if (IS_ERR(gluebi->desc)) { + if (IS_ERR(vdesc)) { + gluebi->desc = NULL; mutex_unlock(&devices_mutex); - return PTR_ERR(gluebi->desc); + return PTR_ERR(vdesc); } + gluebi->desc = vdesc; gluebi->refcnt += 1; mutex_unlock(&devices_mutex); return 0; @@ -134,8 +137,10 @@ static void gluebi_put_device(struct mtd_info *mtd) gluebi = container_of(mtd, struct gluebi_device, mtd); mutex_lock(&devices_mutex); gluebi->refcnt -= 1; - if (gluebi->refcnt == 0) + if (gluebi->refcnt == 0) { ubi_close_volume(gluebi->desc); + gluebi->desc = NULL; + } mutex_unlock(&devices_mutex); } @@ -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 no_desc = gluebi->desc == NULL ? 1 : 0; + + /** + * 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 pointer dereference. + * Therefore, try to get the MTD device here. + */ + if (unlikely(no_desc)) { + 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; + } + } - gluebi = container_of(mtd, struct gluebi_device, mtd); lnum = div_u64_rem(from, mtd->erasesize, &offs); bytes_left = len; while (bytes_left) { @@ -176,6 +198,9 @@ static int gluebi_read(struct mtd_info *mtd, loff_t from, size_t len, } *retlen = len - bytes_left; + + if (unlikely(no_desc)) + __put_mtd_device(mtd); return err; }
If both flt.ko and gluebi.ko are loaded, the notiier of ftl triggers NULL pointer dereference when trying to access ‘gluebi->desc’ in gluebi_read(). 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 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 Detailed reproduction information available at the link[1], In the normal case, obtain gluebi->desc in the gluebi_get_device(), and accesses gluebi->desc in the gluebi_read(). However, gluebi_get_device() is not executed in advance in the ftl_add_mtd() process, which leads to NULL pointer dereference. The value of gluebi->desc may also be a negative error code, which triggers the page fault error. This patch has the following modifications: 1. Do not assign gluebi->desc to the error code. Use the NULL instead. 2. Always check the validity of gluebi->desc in gluebi_read() If the gluebi->desc is NULL, try to get MTD device. Such a modification currently works because the mutex "mtd_table_mutex" is held on all necessary paths, including the ftl_add_mtd() call path, open and close paths. Therefore, many race condition can be avoided. Fixes: 2ba3d76a1e29 ("UBI: make gluebi a separate module") Link: https://bugzilla.kernel.org/show_bug.cgi?id=217992 [1] Signed-off-by: ZhaoLong Wang <wangzhaolong1@huawei.com> --- drivers/mtd/ubi/gluebi.c | 37 +++++++++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 6 deletions(-)