linux-ext4 - Re: [PATCH 2/3] ext4: fix in-superblock mount options processing

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 18 Nov 2016 02:38:41 -0700
From:   Andreas Dilger <adilger@...ger.ca>
To:     Theodore Ts'o <tytso@....edu>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>, kernel@...p.com,
        bp@...en8.de, stable@...r.kernel.org
Subject: Re: [PATCH 2/3] ext4: fix in-superblock mount options processing

On Nov 17, 2016, at 9:26 PM, Theodore Ts'o <tytso@....edu> wrote:
> 
> Fix a large number of problems with how we handle mount options in the
> superblock.  For one, if the string in the superblock is long enough
> that it is not null terminated, we could run off the end of the string
> and try to interpret superblocks fields as characters.  It's unlikely
> this will cause a security problem, but it could result in an invalid
> parse.  Also, parse_options is destructive to the string, so in some
> cases if there is a comma-separated string, it would be modified in
> the superblock.  (Fortunately it only happens on file systems with a
> 1k block size.)
> 
> Signed-off-by: Theodore Ts'o <tytso@....edu>
> Cc: stable@...r.kernel.org
> ---
> fs/ext4/super.c | 37 ++++++++++++++++++++++---------------
> 1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 12f50ef56fe1..3009c03220d6 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3303,7 +3303,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> 	char *orig_data = kstrdup(data, GFP_KERNEL);
> 	struct buffer_head *bh;
> 	struct ext4_super_block *es = NULL;
> -	struct ext4_sb_info *sbi;
> +	struct ext4_sb_info *sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> 	ext4_fsblk_t block;
> 	ext4_fsblk_t sb_block = get_sb_block(&data);
> 	ext4_fsblk_t logical_sb_block;
> @@ -3321,17 +3321,17 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> 	int err = 0;
> 	unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
> 	ext4_group_t first_not_zeroed;
> +	char *s_mount_opts = kzalloc(sizeof(sbi->s_es->s_mount_opts)+1,
> +				   GFP_KERNEL);

This doesn't check if "sbi" is non-NULL before dereferencing it.  While
there isn't really any likelihood for this allocation to fail, I don't
think there is any benefit to moving these allocations to the declaration.

> -	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> -	if (!sbi)
> -		goto out_free_orig;
> +	if ((data && !orig_data) || !s_mount_opts || !sbi)
> +		goto out_free_base;
> 
> 	sbi->s_blockgroup_lock =
> 		kzalloc(sizeof(struct blockgroup_lock), GFP_KERNEL);
> -	if (!sbi->s_blockgroup_lock) {
> -		kfree(sbi);
> -		goto out_free_orig;
> -	}
> +	if (!sbi->s_blockgroup_lock)
> +		goto out_free_base;
> +
> 	sb->s_fs_info = sbi;
> 	sbi->s_sb = sb;
> 	sbi->s_inode_readahead_blks = EXT4_DEF_INODE_READAHEAD_BLKS;
> @@ -3477,11 +3477,15 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> 	 */
> 	sbi->s_li_wait_mult = EXT4_DEF_LI_WAIT_MULT;
> 
> -	if (!parse_options((char *) sbi->s_es->s_mount_opts, sb,
> -			   &journal_devnum, &journal_ioprio, 0)) {
> -		ext4_msg(sb, KERN_WARNING,
> -			 "failed to parse options in superblock: %s",
> -			 sbi->s_es->s_mount_opts);
> +	if (sbi->s_es->s_mount_opts[0]) {
> +		strncpy(s_mount_opts, sbi->s_es->s_mount_opts,
> +			sizeof(sbi->s_es->s_mount_opts));

This could use kstrndup()?  It always allocates an extra byte for the NUL.

> +		if (!parse_options(s_mount_opts, sb, &journal_devnum,
> +				   &journal_ioprio, 0)) {
> +			ext4_msg(sb, KERN_WARNING,
> +				 "failed to parse options in superblock: %s",
> +				 s_mount_opts);
> +		}
> 	}
> 	sbi->s_def_mount_opt = sbi->s_mount_opt;
> 	if (!parse_options((char *) data, sb, &journal_devnum,
> @@ -4162,7 +4166,9 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> 
> 	if (___ratelimit(&ext4_mount_msg_ratelimit, "EXT4-fs mount"))
> 		ext4_msg(sb, KERN_INFO, "mounted filesystem with%s. "
> -			 "Opts: %s%s%s", descr, sbi->s_es->s_mount_opts,
> +			 "Opts: %.*s%s%s", descr,
> +			 sizeof(sbi->s_es->s_mount_opts),
> +			 sbi->s_es->s_mount_opts,

This isn't really needed, since there is always a NUL terminator added
to the string?

> 			 *sbi->s_es->s_mount_opts ? "; " : "", orig_data);
> 
> 	if (es->s_error_count)
> @@ -4241,9 +4247,10 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> out_fail:
> 	sb->s_fs_info = NULL;
> 	kfree(sbi->s_blockgroup_lock);
> +out_free_base:
> 	kfree(sbi);
> -out_free_orig:
> 	kfree(orig_data);
> +	kfree(s_mount_opts);
> 	return err ? err : ret;
> }


Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)