Messages in this thread | | | Subject | Re: [PATCH RFC] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier | From | Zhihao Cheng <> | Date | Thu, 12 Oct 2023 19:25:39 +0800 |
| |
在 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; > + } > + }
| |