mtd

mtd

[Christian Brauner]

I just looked through every single kill_sb once more with an eye
specifically on the bug we just fixed. While doing so I realized that
mtd devices are borked. Taking jffs2 as an example we have:

static void jffs2_kill_sb(struct super_block *sb)
{
        struct jffs2_sb_info *c = JFFS2_SB_INFO(sb);
        if (c && !sb_rdonly(sb))
                jffs2_stop_garbage_collect_thread(c);
        kill_mtd_super(sb);
        kfree(c);
}

kill_mtd_super() calls generic_shutdown_super() which shuts the sb down
but leaves the superblock on fs_supers - which is what we want as the
devices are still in use. But then afterwards it puts the mtd device and
cleans out sb->s_mtd:

void kill_mtd_super(struct super_block *sb)
{
        generic_shutdown_super(sb);
        put_mtd_device(sb->s_mtd);
        sb->s_mtd = NULL;
}

But as you can see in

static int mtd_get_sb()
{
         fc->sget_key = mtd;
         sb = sget_fc(fc, mtd_test_super, mtd_set_super);
}

static int mtd_test_super(struct super_block *sb, struct fs_context *fc)
{
        struct mtd_info *mtd = fc->sget_key;

        if (sb->s_mtd == fc->sget_key) {
                pr_debug("MTDSB: Match on device %d (\"%s\")\n",
                         mtd->index, mtd->name);
                return 1;
        }

        pr_debug("MTDSB: No match, device %d (\"%s\"), device %d (\"%s\")\n",
                 sb->s_mtd->index, sb->s_mtd->name, mtd->index, mtd->name);
        return 0;
}

it can UAF if s_mtd is freed during put_mtd_device(). Yes, there's also
a data race but that's not that problematic.

Of course, the simple hotfix is to notify from kill_mtd_super() and
fixup cramfs and romfs but the proper fix is to do what we did for
get_tree_bdev() and friends and key mtd devices by dev_t. The patch
should be fairly small, I think.

Anyone has cycles to tackle this or should I try?

Something like the following might already be enough (IT'S A DRAFT, AND
UNTESTED, AND PROBABLY BROKEN)?

diff --git a/drivers/mtd/mtdsuper.c b/drivers/mtd/mtdsuper.c
index 5ff001140ef4..992a65d4b90b 100644
--- a/drivers/mtd/mtdsuper.c
+++ b/drivers/mtd/mtdsuper.c
@@ -25,16 +25,15 @@
  */
 static int mtd_test_super(struct super_block *sb, struct fs_context *fc)
 {
-       struct mtd_info *mtd = fc->sget_key;
+       dev_t dev = *(dev_t *)fc->sget_key;

-       if (sb->s_mtd == fc->sget_key) {
-               pr_debug("MTDSB: Match on device %d (\"%s\")\n",
-                        mtd->index, mtd->name);
+       if (sb->s_dev == dev) {
+               pr_debug("MTDSB: Match on device %d\n", MINOR(sb->s_dev));
                return 1;
        }

-       pr_debug("MTDSB: No match, device %d (\"%s\"), device %d (\"%s\")\n",
-                sb->s_mtd->index, sb->s_mtd->name, mtd->index, mtd->name);
+       pr_debug("MTDSB: No match, device %d, device %d\n",
+                MINOR(sb->s_dev), MINOR(dev));
        return 0;
 }

@@ -45,9 +44,7 @@ static int mtd_test_super(struct super_block *sb, struct fs_context *fc)
  */
 static int mtd_set_super(struct super_block *sb, struct fs_context *fc)
 {
-       sb->s_mtd = fc->sget_key;
        sb->s_dev = MKDEV(MTD_BLOCK_MAJOR, sb->s_mtd->index);
-       sb->s_bdi = bdi_get(mtd_bdi);
        return 0;
 }

@@ -61,8 +58,9 @@ static int mtd_get_sb(struct fs_context *fc,
 {
        struct super_block *sb;
        int ret;
+       dev_t dev = MKDEV(MTD_BLOCK_MAJOR, mtd->index);

-       fc->sget_key = mtd;
+       fc->sget_key = &dev;
        sb = sget_fc(fc, mtd_test_super, mtd_set_super);
        if (IS_ERR(sb))
                return PTR_ERR(sb);
@@ -77,6 +75,16 @@ static int mtd_get_sb(struct fs_context *fc,
                pr_debug("MTDSB: New superblock for device %d (\"%s\")\n",
                         mtd->index, mtd->name);

+               /*
+                * Would usually have been set with @sb_lock held but in
+                * contrast to sb->s_bdev that's checked in e.g.,
+                * get_active_super() with only @sb_lock held, nothing seems to
+                * check sb->s_mtd without also holding sb->s_umount and we're
+                * holding sb->s_umount here.
+                */
+               sb->s_mtd = mtd;
+               sb->s_bdi = bdi_get(mtd_bdi);
+
                ret = fill_super(sb, fc);
                if (ret < 0)
                        goto error_sb;

Re: mtd

[Christoph Hellwig]

On Tue, Aug 29, 2023 at 01:46:20PM +0200, Christian Brauner wrote:
> Something like the following might already be enough (IT'S A DRAFT, AND
> UNTESTED, AND PROBABLY BROKEN)?

It's probably the right thing conceptually, but it will also need
the SB_I_RETIRED from test_bdev_super_fc or even just reuse
test_bdev_super_fc after that's been renamed to be more generic.

In fact I've been wondering for a while why we even support the magic
keyed get_super - if it allocates a new super it should also have a
new dev_t.  So IMHO we should stop playing stupid tricks with keys and
just declare the dev_t the key after doing all the required work for it,
that is allocating the per-instance anon dev_t in the caller.

Re: mtd

[Christian Brauner]

On Tue, Aug 29, 2023 at 02:51:18PM +0200, Christoph Hellwig wrote:
> On Tue, Aug 29, 2023 at 01:46:20PM +0200, Christian Brauner wrote:
> > Something like the following might already be enough (IT'S A DRAFT, AND
> > UNTESTED, AND PROBABLY BROKEN)?
> 
> It's probably the right thing conceptually, but it will also need
> the SB_I_RETIRED from test_bdev_super_fc or even just reuse
> test_bdev_super_fc after that's been renamed to be more generic.

I'll rename it and use it. Let me send a patch.

Re: mtd

[Christian Brauner]

On Tue, Aug 29, 2023 at 02:57:02PM +0200, Christian Brauner wrote:
> On Tue, Aug 29, 2023 at 02:51:18PM +0200, Christoph Hellwig wrote:
> > On Tue, Aug 29, 2023 at 01:46:20PM +0200, Christian Brauner wrote:
> > > Something like the following might already be enough (IT'S A DRAFT, AND
> > > UNTESTED, AND PROBABLY BROKEN)?
> > 
> > It's probably the right thing conceptually, but it will also need
> > the SB_I_RETIRED from test_bdev_super_fc or even just reuse
> > test_bdev_super_fc after that's been renamed to be more generic.
> 
> I'll rename it and use it. Let me send a patch.

Hmkay, how does that look? I think this is a fairly acceptable change
and looks better than the mtd special-test/set-sauce we currently have:

From b85ee296f59b0a8e739f10ab9005b7c1fe1aad23 Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@kernel.org>
Date: Tue, 29 Aug 2023 15:05:28 +0200
Subject: [PATCH 1/2] fs: export vfs_super_s_dev_{set,test} helpers

They will be used in other places as well.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/super.c         | 8 +++++---
 include/linux/fs.h | 2 ++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index ad7ac3a24d38..a122154facbf 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1435,16 +1435,18 @@ static int set_bdev_super(struct super_block *s, void *data)
 	return 0;
 }
 
-static int set_bdev_super_fc(struct super_block *s, struct fs_context *fc)
+int vfs_super_s_dev_set(struct super_block *s, struct fs_context *fc)
 {
 	return set_bdev_super(s, fc->sget_key);
 }
+EXPORT_SYMBOL(vfs_super_s_dev_set);
 
-static int test_bdev_super_fc(struct super_block *s, struct fs_context *fc)
+int vfs_super_s_dev_test(struct super_block *s, struct fs_context *fc)
 {
 	return !(s->s_iflags & SB_I_RETIRED) &&
 		s->s_dev == *(dev_t *)fc->sget_key;
 }
+EXPORT_SYMBOL(vfs_super_s_dev_test);
 
 int setup_bdev_super(struct super_block *sb, int sb_flags,
 		struct fs_context *fc)
@@ -1524,7 +1526,7 @@ int get_tree_bdev(struct fs_context *fc,
 
 	fc->sb_flags |= SB_NOSEC;
 	fc->sget_key = &dev;
-	s = sget_fc(fc, test_bdev_super_fc, set_bdev_super_fc);
+	s = sget_fc(fc, vfs_super_s_dev_set, vfs_super_s_dev_test);
 	if (IS_ERR(s))
 		return PTR_ERR(s);
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ca8ceccde3d6..fd32ae238700 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2274,6 +2274,8 @@ struct super_block *sget(struct file_system_type *type,
 			int (*test)(struct super_block *,void *),
 			int (*set)(struct super_block *,void *),
 			int flags, void *data);
+int vfs_super_s_dev_set(struct super_block *s, struct fs_context *fc);
+int vfs_super_s_dev_test(struct super_block *s, struct fs_context *fc);
 
 /* Alas, no aliases. Too much hassle with bringing module.h everywhere */
 #define fops_get(fops) \
-- 
2.34.1

From a91589157e4582182d48a5b7451c4303add26a69 Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@kernel.org>
Date: Tue, 29 Aug 2023 14:58:33 +0200
Subject: [PATCH 2/2] mtd: key superblock by device number

The mtd driver has similar problems than the one that was fixed in
commit dc3216b14160 ("super: ensure valid info").

The kill_mtd_super() helper calls shuts the superblock down but leaves
the superblock on fs_supers as the devices are still in use but puts the
mtd device and cleans out the superblock's s_mtd field.

This means another mounter can find the superblock on the list accessing
its s_mtd field while it is curently in the process of being freed or
already freed.

Prevent that from happening by keying superblock by dev_t just as we do
in the generic code.

Link: https://lore.kernel.org/linux-fsdevel/20230829-weitab-lauwarm-49c40fc85863@brauner
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 drivers/mtd/mtdsuper.c | 47 ++++++++++++------------------------------
 1 file changed, 13 insertions(+), 34 deletions(-)

diff --git a/drivers/mtd/mtdsuper.c b/drivers/mtd/mtdsuper.c
index 5ff001140ef4..29870a375743 100644
--- a/drivers/mtd/mtdsuper.c
+++ b/drivers/mtd/mtdsuper.c
@@ -19,38 +19,6 @@
 #include <linux/fs_context.h>
 #include "mtdcore.h"
 
-/*
- * compare superblocks to see if they're equivalent
- * - they are if the underlying MTD device is the same
- */
-static int mtd_test_super(struct super_block *sb, struct fs_context *fc)
-{
-	struct mtd_info *mtd = fc->sget_key;
-
-	if (sb->s_mtd == fc->sget_key) {
-		pr_debug("MTDSB: Match on device %d (\"%s\")\n",
-			 mtd->index, mtd->name);
-		return 1;
-	}
-
-	pr_debug("MTDSB: No match, device %d (\"%s\"), device %d (\"%s\")\n",
-		 sb->s_mtd->index, sb->s_mtd->name, mtd->index, mtd->name);
-	return 0;
-}
-
-/*
- * mark the superblock by the MTD device it is using
- * - set the device number to be the correct MTD block device for pesuperstence
- *   of NFS exports
- */
-static int mtd_set_super(struct super_block *sb, struct fs_context *fc)
-{
-	sb->s_mtd = fc->sget_key;
-	sb->s_dev = MKDEV(MTD_BLOCK_MAJOR, sb->s_mtd->index);
-	sb->s_bdi = bdi_get(mtd_bdi);
-	return 0;
-}
-
 /*
  * get a superblock on an MTD-backed filesystem
  */
@@ -61,9 +29,10 @@ static int mtd_get_sb(struct fs_context *fc,
 {
 	struct super_block *sb;
 	int ret;
+	dev_t dev = MKDEV(MTD_BLOCK_MAJOR, mtd->index);
 
-	fc->sget_key = mtd;
-	sb = sget_fc(fc, mtd_test_super, mtd_set_super);
+	fc->sget_key = &dev;
+	sb = sget_fc(fc, vfs_super_s_dev_test, vfs_super_s_dev_set);
 	if (IS_ERR(sb))
 		return PTR_ERR(sb);
 
@@ -77,6 +46,16 @@ static int mtd_get_sb(struct fs_context *fc,
 		pr_debug("MTDSB: New superblock for device %d (\"%s\")\n",
 			 mtd->index, mtd->name);
 
+		/*
+		 * Would usually have been set with @sb_lock held but in
+		 * contrast to sb->s_bdev that's checked with only
+		 * @sb_lock held, nothing checks sb->s_mtd without also
+		 * holding sb->s_umount and we're holding sb->s_umount
+		 * here.
+		 */
+		sb->s_mtd = mtd;
+		sb->s_bdi = bdi_get(mtd_bdi);
+
 		ret = fill_super(sb, fc);
 		if (ret < 0)
 			goto error_sb;
-- 
2.34.1

Re: mtd

[Christoph Hellwig]

On Tue, Aug 29, 2023 at 03:41:04PM +0200, Christian Brauner wrote:
> On Tue, Aug 29, 2023 at 02:57:02PM +0200, Christian Brauner wrote:
> > On Tue, Aug 29, 2023 at 02:51:18PM +0200, Christoph Hellwig wrote:
> > > On Tue, Aug 29, 2023 at 01:46:20PM +0200, Christian Brauner wrote:
> > > > Something like the following might already be enough (IT'S A DRAFT, AND
> > > > UNTESTED, AND PROBABLY BROKEN)?
> > > 
> > > It's probably the right thing conceptually, but it will also need
> > > the SB_I_RETIRED from test_bdev_super_fc or even just reuse
> > > test_bdev_super_fc after that's been renamed to be more generic.
> > 
> > I'll rename it and use it. Let me send a patch.
> 
> Hmkay, how does that look? I think this is a fairly acceptable change
> and looks better than the mtd special-test/set-sauce we currently have:

Looks sensibe to me, but please run it past the MTD maintainers.

Re: mtd

[Christian Brauner]

On Tue, Aug 29, 2023 at 04:09:53PM +0200, Christoph Hellwig wrote:
> On Tue, Aug 29, 2023 at 03:41:04PM +0200, Christian Brauner wrote:
> > On Tue, Aug 29, 2023 at 02:57:02PM +0200, Christian Brauner wrote:
> > > On Tue, Aug 29, 2023 at 02:51:18PM +0200, Christoph Hellwig wrote:
> > > > On Tue, Aug 29, 2023 at 01:46:20PM +0200, Christian Brauner wrote:
> > > > > Something like the following might already be enough (IT'S A DRAFT, AND
> > > > > UNTESTED, AND PROBABLY BROKEN)?
> > > > 
> > > > It's probably the right thing conceptually, but it will also need
> > > > the SB_I_RETIRED from test_bdev_super_fc or even just reuse
> > > > test_bdev_super_fc after that's been renamed to be more generic.
> > > 
> > > I'll rename it and use it. Let me send a patch.
> > 
> > Hmkay, how does that look? I think this is a fairly acceptable change
> > and looks better than the mtd special-test/set-sauce we currently have:
> 
> Looks sensibe to me, but please run it past the MTD maintainers.

Done.