| Message ID | 20231010142925.545238-1-wangzhaolong1@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier | expand |
> This patch assumes that the gluebi module is not designed to work with > the ftl module. In this case, the patch only needs to prevent the ftl > notifier operation. > > Add some correctness check for gluebi->desc in gluebi_read/write/erase(), > If the pointer is invalid, the -EINVAL is returned. > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217992 [1] > Signed-off-by: ZhaoLong Wang <wangzhaolong1@huawei.com> > --- > drivers/mtd/ubi/gluebi.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c > index 1b980d15d9fb..189ecc0eacd1 100644 > --- a/drivers/mtd/ubi/gluebi.c > +++ b/drivers/mtd/ubi/gluebi.c > @@ -157,6 +157,9 @@ static int gluebi_read(struct mtd_info *mtd, loff_t from, size_t len, > struct gluebi_device *gluebi; > > gluebi = container_of(mtd, struct gluebi_device, mtd); > + if (IS_ERR_OR_NULL(gluebi->desc)) > + return -EINVAL; > + > lnum = div_u64_rem(from, mtd->erasesize, &offs); > bytes_left = len; > while (bytes_left) { > @@ -197,6 +200,9 @@ static int gluebi_write(struct mtd_info *mtd, loff_t to, size_t len, > struct gluebi_device *gluebi; > > gluebi = container_of(mtd, struct gluebi_device, mtd); > + if (IS_ERR_OR_NULL(gluebi->desc)) > + return -EINVAL; > + > lnum = div_u64_rem(to, mtd->erasesize, &offs); > > if (len % mtd->writesize || offs % mtd->writesize) > @@ -242,6 +248,8 @@ static int gluebi_erase(struct mtd_info *mtd, struct erase_info *instr) > lnum = mtd_div_by_eb(instr->addr, mtd); > count = mtd_div_by_eb(instr->len, mtd); > gluebi = container_of(mtd, struct gluebi_device, mtd); > + if (IS_ERR_OR_NULL(gluebi->desc)) > + return -EINVAL; > > for (i = 0; i < count - 1; i++) { > err = ubi_leb_unmap(gluebi->desc, lnum + i); This modification attempts another solution. Always check the validity of gluebi->desc. If the gluebi->desc pointer is invalid, try to get MTD device. diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c index 1b980d15d9fb..f1a74ccf1718 100644 --- a/drivers/mtd/ubi/gluebi.c +++ b/drivers/mtd/ubi/gluebi.c @@ -154,9 +154,19 @@ 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 not_get = IS_ERR_OR_NULL(gluebi->desc); + + if (not_get) { + 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 +186,9 @@ static int gluebi_read(struct mtd_info *mtd, loff_t from, size_t len, } *retlen = len - bytes_left; + + if (not_get) + __put_mtd_device(mtd); return err; } @@ -194,9 +207,19 @@ static int gluebi_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen, const u_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 not_get = IS_ERR_OR_NULL(gluebi->desc); + + if (not_get) { + 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(to, mtd->erasesize, &offs); if (len % mtd->writesize || offs % mtd->writesize) @@ -220,6 +243,9 @@ static int gluebi_write(struct mtd_info *mtd, loff_t to, size_t len, } *retlen = len - bytes_left; + + if (not_get) + __put_mtd_device(mtd); return err; } @@ -234,14 +260,24 @@ static int gluebi_write(struct mtd_info *mtd, loff_t to, size_t len, static int gluebi_erase(struct mtd_info *mtd, struct erase_info *instr) { int err, i, lnum, count; - struct gluebi_device *gluebi; + struct gluebi_device *gluebi = container_of(mtd, struct gluebi_device, + mtd); + int not_get = IS_ERR_OR_NULL(gluebi->desc); + + if (not_get) { + 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; + } + } if (mtd_mod_by_ws(instr->addr, mtd) || mtd_mod_by_ws(instr->len, mtd)) return -EINVAL; lnum = mtd_div_by_eb(instr->addr, mtd); count = mtd_div_by_eb(instr->len, mtd); - gluebi = container_of(mtd, struct gluebi_device, mtd); for (i = 0; i < count - 1; i++) { err = ubi_leb_unmap(gluebi->desc, lnum + i); @@ -259,10 +295,14 @@ static int gluebi_erase(struct mtd_info *mtd, struct erase_info *instr) if (err) goto out_err; + if (not_get) + __put_mtd_device(mtd); return 0; out_err: instr->fail_addr = (long long)lnum * mtd->erasesize; + if (not_get) + __put_mtd_device(mtd); return err; }
在 2023/10/10 22:29, ZhaoLong Wang 写道: > If both flt.ko and gluebi.ko are loaded, the notiier of ftl > triggers NULL pointer dereference when trying to visit > ‘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. > > This patch assumes that the gluebi module is not designed to work with > the ftl module. In this case, the patch only needs to prevent the ftl > notifier operation. We can't refuse ftl when gluebi is loaded, it looks weird: mtd0(nand) -> ftl0 mtd1(gluebi) has no ftl1? When FTL is loaded, it should make sure each mtd device correspond to a block device, no matter which type the mtd device is(nand or gluebi). So I prefer the root cause is gluebi->desc is not initialized in gluebi_create->mtd_device_register. > > Add some correctness check for gluebi->desc in gluebi_read/write/erase(), > If the pointer is invalid, the -EINVAL is returned. > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217992 [1] > Signed-off-by: ZhaoLong Wang <wangzhaolong1@huawei.com> > --- > drivers/mtd/ubi/gluebi.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c > index 1b980d15d9fb..189ecc0eacd1 100644 > --- a/drivers/mtd/ubi/gluebi.c > +++ b/drivers/mtd/ubi/gluebi.c > @@ -157,6 +157,9 @@ static int gluebi_read(struct mtd_info *mtd, loff_t from, size_t len, > struct gluebi_device *gluebi; > > gluebi = container_of(mtd, struct gluebi_device, mtd); > + if (IS_ERR_OR_NULL(gluebi->desc)) > + return -EINVAL; > + > lnum = div_u64_rem(from, mtd->erasesize, &offs); > bytes_left = len; > while (bytes_left) { > @@ -197,6 +200,9 @@ static int gluebi_write(struct mtd_info *mtd, loff_t to, size_t len, > struct gluebi_device *gluebi; > > gluebi = container_of(mtd, struct gluebi_device, mtd); > + if (IS_ERR_OR_NULL(gluebi->desc)) > + return -EINVAL; > + > lnum = div_u64_rem(to, mtd->erasesize, &offs); > > if (len % mtd->writesize || offs % mtd->writesize) > @@ -242,6 +248,8 @@ static int gluebi_erase(struct mtd_info *mtd, struct erase_info *instr) > lnum = mtd_div_by_eb(instr->addr, mtd); > count = mtd_div_by_eb(instr->len, mtd); > gluebi = container_of(mtd, struct gluebi_device, mtd); > + if (IS_ERR_OR_NULL(gluebi->desc)) > + return -EINVAL; > > for (i = 0; i < count - 1; i++) { > err = ubi_leb_unmap(gluebi->desc, lnum + i); >
在 2023/10/11 10:32, ZhaoLong Wang 写道: > >> This patch assumes that the gluebi module is not designed to work with >> the ftl module. In this case, the patch only needs to prevent the ftl >> notifier operation. >> >> Add some correctness check for gluebi->desc in gluebi_read/write/erase(), >> If the pointer is invalid, the -EINVAL is returned. >> >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217992 [1] >> Signed-off-by: ZhaoLong Wang <wangzhaolong1@huawei.com> >> --- >> drivers/mtd/ubi/gluebi.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c >> index 1b980d15d9fb..189ecc0eacd1 100644 >> --- a/drivers/mtd/ubi/gluebi.c >> +++ b/drivers/mtd/ubi/gluebi.c >> @@ -157,6 +157,9 @@ static int gluebi_read(struct mtd_info *mtd, >> loff_t from, size_t len, >> struct gluebi_device *gluebi; >> gluebi = container_of(mtd, struct gluebi_device, mtd); >> + if (IS_ERR_OR_NULL(gluebi->desc)) >> + return -EINVAL; >> + >> lnum = div_u64_rem(from, mtd->erasesize, &offs); >> bytes_left = len; >> while (bytes_left) { >> @@ -197,6 +200,9 @@ static int gluebi_write(struct mtd_info *mtd, >> loff_t to, size_t len, >> struct gluebi_device *gluebi; >> gluebi = container_of(mtd, struct gluebi_device, mtd); >> + if (IS_ERR_OR_NULL(gluebi->desc)) >> + return -EINVAL; >> + >> lnum = div_u64_rem(to, mtd->erasesize, &offs); >> if (len % mtd->writesize || offs % mtd->writesize) >> @@ -242,6 +248,8 @@ static int gluebi_erase(struct mtd_info *mtd, >> struct erase_info *instr) >> lnum = mtd_div_by_eb(instr->addr, mtd); >> count = mtd_div_by_eb(instr->len, mtd); >> gluebi = container_of(mtd, struct gluebi_device, mtd); >> + if (IS_ERR_OR_NULL(gluebi->desc)) >> + return -EINVAL; >> for (i = 0; i < count - 1; i++) { >> err = ubi_leb_unmap(gluebi->desc, lnum + i); > > > This modification attempts another solution. Always check the validity > of gluebi->desc. If the gluebi->desc pointer is invalid, try to get MTD > device. > > > diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c > index 1b980d15d9fb..f1a74ccf1718 100644 > --- a/drivers/mtd/ubi/gluebi.c > +++ b/drivers/mtd/ubi/gluebi.c > @@ -154,9 +154,19 @@ 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 not_get = IS_ERR_OR_NULL(gluebi->desc); > + > + if (not_get) { > + 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 +186,9 @@ static int gluebi_read(struct mtd_info *mtd, loff_t > from, size_t len, > } > > *retlen = len - bytes_left; > + > + if (not_get) > + __put_mtd_device(mtd); > return err; > } > I'm afraid that this patch won't cover following three situations completely: 1. gluebi_create -> ftl_add_mtd -> mtd_read -> gluebi_read: gluebi->desc is NULL. (√) 2. fd = open(/dev/ubi0_0, O_WRONLY) ubi_open_volume // vol->writers = 1 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 gluebi->desc = ERR_PTR(EBUSY) ftl_add_mtd mtd_read gluebi_read gluebi->desc is ERR_PTR (√) 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 (×) > @@ -194,9 +207,19 @@ static int gluebi_write(struct mtd_info *mtd, > loff_t to, size_t len, > size_t *retlen, const u_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 not_get = IS_ERR_OR_NULL(gluebi->desc); > + > + if (not_get) { > + 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(to, mtd->erasesize, &offs); > > if (len % mtd->writesize || offs % mtd->writesize) > @@ -220,6 +243,9 @@ static int gluebi_write(struct mtd_info *mtd, loff_t > to, size_t len, > } > > *retlen = len - bytes_left; > + > + if (not_get) > + __put_mtd_device(mtd); > return err; > } > > @@ -234,14 +260,24 @@ static int gluebi_write(struct mtd_info *mtd, > loff_t to, size_t len, > static int gluebi_erase(struct mtd_info *mtd, struct erase_info *instr) > { > int err, i, lnum, count; > - struct gluebi_device *gluebi; > + struct gluebi_device *gluebi = container_of(mtd, struct gluebi_device, > + mtd); > + int not_get = IS_ERR_OR_NULL(gluebi->desc); > + > + if (not_get) { > + 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; > + } > + } > > if (mtd_mod_by_ws(instr->addr, mtd) || mtd_mod_by_ws(instr->len, > mtd)) > return -EINVAL; > > lnum = mtd_div_by_eb(instr->addr, mtd); > count = mtd_div_by_eb(instr->len, mtd); > - gluebi = container_of(mtd, struct gluebi_device, mtd); > > for (i = 0; i < count - 1; i++) { > err = ubi_leb_unmap(gluebi->desc, lnum + i); > @@ -259,10 +295,14 @@ static int gluebi_erase(struct mtd_info *mtd, > struct erase_info *instr) > if (err) > goto out_err; > > + if (not_get) > + __put_mtd_device(mtd); > return 0; > > out_err: > instr->fail_addr = (long long)lnum * mtd->erasesize; > + if (not_get) > + __put_mtd_device(mtd); > return err; > } > No need to modify 'gluebi_write' and 'gluebi_erase'.
I'm very happy to receive a reply to the review. > 2. fd = open(/dev/ubi0_0, O_WRONLY) > ubi_open_volume // vol->writers = 1 > > 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 > gluebi->desc = ERR_PTR(EBUSY) > > ftl_add_mtd > mtd_read > gluebi_read > gluebi->desc is ERR_PTR (√) The reproduction steps for situations 2 and 3 have been added to link[1]. Link: https://bugzilla.kernel.org/show_bug.cgi?id=217992 [1] > 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. > > No need to modify 'gluebi_write' and 'gluebi_erase'. > The patch is as follows: diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c index 1b980d15d9fb..8fc6017d1155 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 isnt_get = unlikely(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 pointe dereference. + * Therefore, try to get the MTD device here. + */ + if (unlikely(isnt_get)) { + 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(isnt_get)) + __put_mtd_device(mtd); return err; }
在 2023/10/12 16:04, ZhaoLong Wang 写道: > I'm very happy to receive a reply to the review. > >> 2. fd = open(/dev/ubi0_0, O_WRONLY) >> ubi_open_volume // vol->writers = 1 >> >> 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 >> gluebi->desc = ERR_PTR(EBUSY) >> >> ftl_add_mtd >> mtd_read >> gluebi_read >> gluebi->desc is ERR_PTR (√) > > The reproduction steps for situations 2 and 3 have been added to link[1]. > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217992 [1] > >> 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. > >> >> No need to modify 'gluebi_write' and 'gluebi_erase'. >> > > The patch is as follows: > > diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c > index 1b980d15d9fb..8fc6017d1155 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 isnt_get = unlikely(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 pointe dereference. > + * Therefore, try to get the MTD device here. > + */ > + if (unlikely(isnt_get)) { > + 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(isnt_get)) > + __put_mtd_device(mtd); > return err; > } > > > > .
>> >>> 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);
在 2023/10/12 17:31, 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. > Oh, you're right. Just one nit below: > @@ -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 isnt_get = unlikely(gluebi->desc == NULL) ? 1 : 0; This 'unlikey' can be removed. Rename 'isnt_get' as 'has_desc' ? > + /** > + * 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 pointe dereference. > + * Therefore, try to get the MTD device here. > + */ > + if (unlikely(isnt_get)) { > + 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; > + } > + }
Friendly ping ... > If both flt.ko and gluebi.ko are loaded, the notiier of ftl > triggers NULL pointer dereference when trying to visit > ‘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.
diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c index 1b980d15d9fb..189ecc0eacd1 100644 --- a/drivers/mtd/ubi/gluebi.c +++ b/drivers/mtd/ubi/gluebi.c @@ -157,6 +157,9 @@ static int gluebi_read(struct mtd_info *mtd, loff_t from, size_t len, struct gluebi_device *gluebi; gluebi = container_of(mtd, struct gluebi_device, mtd); + if (IS_ERR_OR_NULL(gluebi->desc)) + return -EINVAL; + lnum = div_u64_rem(from, mtd->erasesize, &offs); bytes_left = len; while (bytes_left) { @@ -197,6 +200,9 @@ static int gluebi_write(struct mtd_info *mtd, loff_t to, size_t len, struct gluebi_device *gluebi; gluebi = container_of(mtd, struct gluebi_device, mtd); + if (IS_ERR_OR_NULL(gluebi->desc)) + return -EINVAL; + lnum = div_u64_rem(to, mtd->erasesize, &offs); if (len % mtd->writesize || offs % mtd->writesize) @@ -242,6 +248,8 @@ static int gluebi_erase(struct mtd_info *mtd, struct erase_info *instr) lnum = mtd_div_by_eb(instr->addr, mtd); count = mtd_div_by_eb(instr->len, mtd); gluebi = container_of(mtd, struct gluebi_device, mtd); + if (IS_ERR_OR_NULL(gluebi->desc)) + return -EINVAL; for (i = 0; i < count - 1; i++) { err = ubi_leb_unmap(gluebi->desc, lnum + i);
If both flt.ko and gluebi.ko are loaded, the notiier of ftl triggers NULL pointer dereference when trying to visit ‘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. This patch assumes that the gluebi module is not designed to work with the ftl module. In this case, the patch only needs to prevent the ftl notifier operation. Add some correctness check for gluebi->desc in gluebi_read/write/erase(), If the pointer is invalid, the -EINVAL is returned. Link: https://bugzilla.kernel.org/show_bug.cgi?id=217992 [1] Signed-off-by: ZhaoLong Wang <wangzhaolong1@huawei.com> --- drivers/mtd/ubi/gluebi.c | 8 ++++++++ 1 file changed, 8 insertions(+)