diff mbox series

[v3] mtd: Fix gluebi NULL pointer dereference caused by ftl notifier

Message ID 20231027012033.50280-1-wangzhaolong1@huawei.com
State New
Headers show
Series [v3] mtd: Fix gluebi NULL pointer dereference caused by ftl notifier | expand

Commit Message

ZhaoLong Wang Oct. 27, 2023, 1:20 a.m. UTC 
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_oob
                            mtd_read_oob_std
                              gluebi_read   mtd->read()
                                gluebi->desc - NULL

Detailed reproduction information available at the link[1],

The solution for the gluebi module is to run jffs2 on the UBI
volume without considering working with ftl or mtdblock.[2].
Therefore, this problem can be avoided by preventing gluebi
from creating mtdblock devices.

Fixes: 2ba3d76a1e29 ("UBI: make gluebi a separate module")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=217992 [1]
Link: https://lore.kernel.org/lkml/441107100.23734.1697904580252.JavaMail.zimbra@nod.at/ [2]
Signed-off-by: ZhaoLong Wang <wangzhaolong1@huawei.com>
---
 drivers/mtd/mtd_blkdevs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Zhihao Cheng Oct. 27, 2023, 3:23 a.m. UTC | #1 
在 2023/10/27 9:20, 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_oob
>                              mtd_read_oob_std
>                                gluebi_read   mtd->read()
>                                  gluebi->desc - NULL
>
> Detailed reproduction information available at the link[1],
>
> The solution for the gluebi module is to run jffs2 on the UBI
> volume without considering working with ftl or mtdblock.[2].
> Therefore, this problem can be avoided by preventing gluebi
> from creating mtdblock devices.
>
> Fixes: 2ba3d76a1e29 ("UBI: make gluebi a separate module")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217992 [1]
> Link: https://lore.kernel.org/lkml/441107100.23734.1697904580252.JavaMail.zimbra@nod.at/ [2]
> Signed-off-by: ZhaoLong Wang <wangzhaolong1@huawei.com>
> ---
>   drivers/mtd/mtd_blkdevs.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Zhihao Cheng <chengzhihao1@huawei.com>
Miquel Raynal Oct. 27, 2023, 5:40 p.m. UTC | #2 
Hi ZhaoLong,

wangzhaolong1@huawei.com wrote on Fri, 27 Oct 2023 09:20:33 +0800:

> If both flt.ko and gluebi.ko are loaded, the notiier of ftl

flt ?					notifier

> 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()

Glitches?

>                       scan_header
>                         mtd_read
>                           mtd_read_oob
>                             mtd_read_oob_std
>                               gluebi_read   mtd->read()
>                                 gluebi->desc - NULL
> 
> Detailed reproduction information available at the link[1],
> 
> The solution for the gluebi module is to run jffs2 on the UBI
> volume without considering working with ftl or mtdblock.[2].

I am sorry but ftl, gluebi, mtdblock, jffs2 and ubi in the same report
seem a little bit fuzzy. Are you sure about this sentence?

> Therefore, this problem can be avoided by preventing gluebi
> from creating mtdblock devices.

This sentence sounds wrong :)

> Fixes: 2ba3d76a1e29 ("UBI: make gluebi a separate module")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217992 [1]
> Link: https://lore.kernel.org/lkml/441107100.23734.1697904580252.JavaMail.zimbra@nod.at/ [2]
> Signed-off-by: ZhaoLong Wang <wangzhaolong1@huawei.com>
> ---
>  drivers/mtd/mtd_blkdevs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
> index ff18636e0889..5bc32108ca03 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)
> @@ -503,7 +503,7 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr)
>  	mutex_lock(&mtd_table_mutex);
>  	list_add(&tr->list, &blktrans_majors);
>  	mtd_for_each_device(mtd)
> -		if (mtd->type != MTD_ABSENT)
> +		if (mtd->type != MTD_ABSENT && mtd->type != MTD_UBIVOLUME)
>  			tr->add_mtd(tr, mtd);
>  	mutex_unlock(&mtd_table_mutex);
>  	return 0;


Thanks,
Miquèl
Richard Weinberger Oct. 27, 2023, 8:30 p.m. UTC | #3 
----- Ursprüngliche Mail -----
> Von: "Miquel Raynal" <miquel.raynal@bootlin.com>
>> Detailed reproduction information available at the link[1],
>> 
>> The solution for the gluebi module is to run jffs2 on the UBI
>> volume without considering working with ftl or mtdblock.[2].
> 
> I am sorry but ftl, gluebi, mtdblock, jffs2 and ubi in the same report
> seem a little bit fuzzy. Are you sure about this sentence?

ZhaoLong Wang found an interesting bug while stacking various MTD components.
I suggested to just deny mtdblock on top of gluebi to avoid the whole
problem class instead of adding more duct tape.

Thanks,
//richard

The issue reminds me of one of my favorite Monty Python sketches, https://www.youtube.com/watch?v=LFrdqQZ8FFc
;-)
Miquel Raynal Oct. 29, 2023, 1:50 p.m. UTC | #4 
Hi Richard,

richard@nod.at wrote on Fri, 27 Oct 2023 22:30:10 +0200 (CEST):

> ----- Ursprüngliche Mail -----
> > Von: "Miquel Raynal" <miquel.raynal@bootlin.com>  
> >> Detailed reproduction information available at the link[1],
> >> 
> >> The solution for the gluebi module is to run jffs2 on the UBI
> >> volume without considering working with ftl or mtdblock.[2].  
> > 
> > I am sorry but ftl, gluebi, mtdblock, jffs2 and ubi in the same report
> > seem a little bit fuzzy. Are you sure about this sentence?  
> 
> ZhaoLong Wang found an interesting bug while stacking various MTD components.
> I suggested to just deny mtdblock on top of gluebi to avoid the whole
> problem class instead of adding more duct tape.

Ack. I'll queue this once the log fixed and your official Ack-by
received for mtd/next after -rc1 is out.

Thanks,
Miquèl

> 
> Thanks,
> //richard
> 
> The issue reminds me of one of my favorite Monty Python sketches, https://www.youtube.com/watch?v=LFrdqQZ8FFc
> ;-)

:-)
Richard Weinberger Oct. 29, 2023, 3:39 p.m. UTC | #5 
----- Ursprüngliche Mail -----
> Von: "Miquel Raynal" <miquel.raynal@bootlin.com>
> Ack. I'll queue this once the log fixed and your official Ack-by
> received for mtd/next after -rc1 is out.

Acked-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard
ZhaoLong Wang Dec. 19, 2023, 2:36 p.m. UTC | #6 
Friendly ping...

I'd like to know what the current status of this patch is. Is there a 
need for further programme improvements?

Bast regards,
ZhaoLong Wang


> ----- Ursprüngliche Mail -----
>> Von: "Miquel Raynal" <miquel.raynal@bootlin.com>
>> Ack. I'll queue this once the log fixed and your official Ack-by
>> received for mtd/next after -rc1 is out.
> 
> Acked-by: Richard Weinberger <richard@nod.at>
> 
> Thanks,
> //richard
>
Miquel Raynal Dec. 19, 2023, 2:44 p.m. UTC | #7 
Hi ZhaoLong,

wangzhaolong1@huawei.com wrote on Tue, 19 Dec 2023 22:36:50 +0800:

> Friendly ping...
> 
> I'd like to know what the current status of this patch is. Is there a need for further programme improvements?

Easy, see below.

> 
> Bast regards,
> ZhaoLong Wang
> 
> 
> > ----- Ursprüngliche Mail -----  
> >> Von: "Miquel Raynal" <miquel.raynal@bootlin.com>
> >> Ack. I'll queue this once the log fixed and your official Ack-by
> >> received for mtd/next after -rc1 is out.  

Here I depict two conditions.

> > Acked-by: Richard Weinberger <richard@nod.at>

And here one of them is fulfilled, which means the other one is still
not AFAIK :)

Thanks,
Miquèl
ZhaoLong Wang Dec. 20, 2023, 2:15 a.m. UTC | #8 
Oh, sorry, I had missed a crucial part of your previous message,
specifically the segment about the `flt ? notifier,This oversight
led to my misunderstanding of the situation, and for that, I am
truly sorry.

Thank you for your understanding and patience. I am committed to
maintaining clear and accurate communication going forward.

I'll send a v4 version of the log fixed patch soon.

Warmest thanks,
ZhaoLong Wang

> Hi ZhaoLong,
> 
> wangzhaolong1@huawei.com wrote on Tue, 19 Dec 2023 22:36:50 +0800:
> 
>> Friendly ping...
>>
>> I'd like to know what the current status of this patch is. Is there a need for further programme improvements?
> 
> Easy, see below.
> 
>>
>> Bast regards,
>> ZhaoLong Wang
>>
>>
>>> ----- Ursprüngliche Mail -----
>>>> Von: "Miquel Raynal" <miquel.raynal@bootlin.com>
>>>> Ack. I'll queue this once the log fixed and your official Ack-by
>>>> received for mtd/next after -rc1 is out.
> 
> Here I depict two conditions.
> 
>>> Acked-by: Richard Weinberger <richard@nod.at>
> 
> And here one of them is fulfilled, which means the other one is still
> not AFAIK :)
> 
> Thanks,
> Miquèl
diff mbox series

Patch

diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index ff18636e0889..5bc32108ca03 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)
@@ -503,7 +503,7 @@  int register_mtd_blktrans(struct mtd_blktrans_ops *tr)
 	mutex_lock(&mtd_table_mutex);
 	list_add(&tr->list, &blktrans_majors);
 	mtd_for_each_device(mtd)
-		if (mtd->type != MTD_ABSENT)
+		if (mtd->type != MTD_ABSENT && mtd->type != MTD_UBIVOLUME)
 			tr->add_mtd(tr, mtd);
 	mutex_unlock(&mtd_table_mutex);
 	return 0;