linux-kernel - Re: [PATCH v2] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <441107100.23734.1697904580252.JavaMail.zimbra@nod.at>
Date:   Sat, 21 Oct 2023 18:09:40 +0200 (CEST)
From:   Richard Weinberger <richard@....at>
To:     chengzhihao1 <chengzhihao1@...wei.com>
Cc:     ZhaoLong Wang <wangzhaolong1@...wei.com>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Vignesh Raghavendra <vigneshr@...com>,
        dpervushin <dpervushin@...eddedalley.com>,
        Artem Bityutskiy <Artem.Bityutskiy@...ia.com>,
        linux-mtd <linux-mtd@...ts.infradead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        yi zhang <yi.zhang@...wei.com>,
        yangerkun <yangerkun@...wei.com>
Subject: Re: [PATCH v2] ubi: gluebi: Fix NULL pointer dereference caused by
 ftl notifier

----- Ursprüngliche Mail -----
> Von: "chengzhihao1" <chengzhihao1@...wei.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