| Message ID | 20230829-weitab-lauwarm-49c40fc85863@brauner (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mtd | expand |
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.
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.
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) \
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.
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.
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;
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)?