From: Christian Brauner <brauner@kernel.org>
To: Jan Kara <jack@suse.cz>, Christoph Hellwig <hch@lst.de>
Cc: linux-fsdevel@vger.kernel.org
Subject: mtd
Date: Tue, 29 Aug 2023 13:46:20 +0200 [thread overview]
Message-ID: <20230829-weitab-lauwarm-49c40fc85863@brauner> (raw)
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;
next reply other threads:[~2023-08-29 11:47 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-29 11:46 Christian Brauner [this message]
2023-08-29 12:51 ` mtd Christoph Hellwig
2023-08-29 12:56 ` mtd Christian Brauner
2023-08-29 13:41 ` mtd Christian Brauner
2023-08-29 14:09 ` mtd Christoph Hellwig
2023-08-29 16:29 ` mtd Christian Brauner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230829-weitab-lauwarm-49c40fc85863@brauner \
--to=brauner@kernel.org \
--cc=hch@lst.de \
--cc=jack@suse.cz \
--cc=linux-fsdevel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).