| 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
| ||
|
Date: Tue, 7 Apr 2009 13:55:02 +0800
From: Wu Fengguang <fengguang.wu@...el.com>
To: Ingo Molnar <mingo@...e.hu>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Avan Anishchuk <matimatik@...il.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Pekka Enberg <penberg@...helsinki.fi>,
Steven Rostedt <rostedt@...dmis.org>,
Thomas Gleixner <tglx@...utronix.de>,
Eduard - Gabriel Munteanu <eduard.munteanu@...ux360.ro>
Subject: Re: [patch] ramfs: add support for "mode=" mount option, fix
On Tue, Apr 07, 2009 at 01:28:01PM +0800, Ingo Molnar wrote:
>
> * Linus Torvalds <torvalds@...ux-foundation.org> wrote:
>
> > On Mon, 6 Apr 2009, Linus Torvalds wrote:
> > >
> > > It bisected past them. I'm getting worried that it's timing-related,
> > > because nothing that remains looks even remotely interesting for that Mac
> > > mini, but right now:
> > >
> > > - bad: 56fcef75117a153f298b3fe54af31053f53997dd
> > > - good: bb233fdfc7b7cefe45bfa2e8d1b24e79c60a48e5
> > >
> > > and there's not a whole lot of commits in between.
> >
> > It's c3b1b1cbf002e65a3cabd479e68b5f35886a26db: 'ramfs: add support
> > for "mode=" mount option'.
> >
> > And I checked. Reverting it at the tip fixes it. So no random
> > timing fluctuations.
> >
> > So that commit causes some random SLAB corruption, that then
> > (depending apparently on luck) may or may not crash in some odd
> > random places later.
>
> ah - forget my previous mail then.
>
> This commit does have a couple of genuinely odd looking lines.
>
> For example:
>
> + sb->s_fs_info = fsi;
> +
> + err = ramfs_parse_options(data, &fsi->mount_opts);
> + if (err)
> + goto fail;
>
> Say we fail in ramfs_parse_options() and do the 'fail' pattern:
>
> +fail:
> + kfree(fsi);
> + iput(inode);
> + return err;
>
> so we have 'fsi' kfree()'d but dont clear sb->s_fs_info! That's
> almost always a bad practice. And indeed, in the kill_super
Sorry - yes, the double kfree() shall be the root cause!
get_sb_nodev() calls kill_sb() after a failed fill_super():
error = fill_super(s, data, flags & MS_SILENT ? 1 : 0);
if (error) {
up_write(&s->s_umount);
deactivate_super(s);
return error;
}
> callback:
>
> +static void ramfs_kill_sb(struct super_block *sb)
> +{
> + kfree(sb->s_fs_info);
>
> What ensures that this cannot be a double kfree() memory corruption?
> That pointer should have been cleared with something like the patch
> below. (totally untested)
>
> And there's also another, probably just theoretical worry about
> another failure path:
>
> + fsi = kzalloc(sizeof(struct ramfs_fs_info), GFP_KERNEL);
> + if (!fsi) {
> + err = -ENOMEM;
> + goto fail;
> + }
> + sb->s_fs_info = fsi;
>
> leaves sb->s_fs_info uninitialized in the failure case, and might
> hit this code unconditionally:
>
> +static void ramfs_kill_sb(struct super_block *sb)
> +{
> + kfree(sb->s_fs_info);
> + kill_litter_super(sb);
> +}
>
> Leaving this code at the mercy of the external call environment
> initializing sb->s_fs_info. Which if it does not do (or stops
> doing in the future), can trigger a kfree of a random pointer.
>
> (I think ->kill_super() gets called even if ->fill_super() fails,
> but i have not checked closely.)
You are right, see above.
> These kinds of assymetric failure paths are really a red flag during
> review.
>
> VFS infrastructure nit: we have 20 other similar looking but
> slightly differently implemented filesystem options parsers, in each
> filesystem. Might make sense to factor that out a bit and
> standardize it across all filesystems and make it all a bit safer.
> Duplicating code like that is never good IMHO.
>
> Ingo
>
Acked-by: Wu Fengguang <fengguang.wu@...el.com>
The patch looks pretty good and runs OK here.
Thanks,
Fengguang
> diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
> index a404fb8..3a6b193 100644
> --- a/fs/ramfs/inode.c
> +++ b/fs/ramfs/inode.c
> @@ -221,22 +221,23 @@ static int ramfs_fill_super(struct super_block * sb, void * data, int silent)
> save_mount_options(sb, data);
>
> fsi = kzalloc(sizeof(struct ramfs_fs_info), GFP_KERNEL);
> + sb->s_fs_info = fsi;
> if (!fsi) {
> err = -ENOMEM;
> goto fail;
> }
> - sb->s_fs_info = fsi;
>
> err = ramfs_parse_options(data, &fsi->mount_opts);
> if (err)
> goto fail;
>
> - sb->s_maxbytes = MAX_LFS_FILESIZE;
> - sb->s_blocksize = PAGE_CACHE_SIZE;
> - sb->s_blocksize_bits = PAGE_CACHE_SHIFT;
> - sb->s_magic = RAMFS_MAGIC;
> - sb->s_op = &ramfs_ops;
> - sb->s_time_gran = 1;
> + sb->s_maxbytes = MAX_LFS_FILESIZE;
> + sb->s_blocksize = PAGE_CACHE_SIZE;
> + sb->s_blocksize_bits = PAGE_CACHE_SHIFT;
> + sb->s_magic = RAMFS_MAGIC;
> + sb->s_op = &ramfs_ops;
> + sb->s_time_gran = 1;
> +
> inode = ramfs_get_inode(sb, S_IFDIR | fsi->mount_opts.mode, 0);
> if (!inode) {
> err = -ENOMEM;
> @@ -244,14 +245,16 @@ static int ramfs_fill_super(struct super_block * sb, void * data, int silent)
> }
>
> root = d_alloc_root(inode);
> + sb->s_root = root;
> if (!root) {
> err = -ENOMEM;
> goto fail;
> }
> - sb->s_root = root;
> +
> return 0;
> fail:
> kfree(fsi);
> + sb->s_fs_info = NULL;
> iput(inode);
> return err;
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists