Re: [PATCH RFC] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier

[Zhihao Cheng]

在 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@xxxxxxxxxx>
> ---
>   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);