mtd

[Christian Brauner <brauner@xxxxxxxxxx>]

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;