[PATCH 1/4] ext4: sanity check the block and cluster size at mount time

[PATCH 1/4] ext4: sanity check the block and cluster size at mount time

[Theodore Ts'o]

If the block size or cluster size is insane, reject the mount.  This
is important for security reasons (although we shouldn't be just
depending on this check).

Ref: http://www.securityfocus.com/archive/1/539661
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1332506
Reported-by: Borislav Petkov <bp@alien8.de>
Reported-by: Nikolay Borisov <kernel@kyup.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Cc: stable@vger.kernel.org
---
 fs/ext4/ext4.h  |  1 +
 fs/ext4/super.c | 17 ++++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 53d6d463ac4d..bdf1e5ee8642 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -235,6 +235,7 @@ struct ext4_io_submit {
 #define	EXT4_MAX_BLOCK_SIZE		65536
 #define EXT4_MIN_BLOCK_LOG_SIZE		10
 #define EXT4_MAX_BLOCK_LOG_SIZE		16
+#define EXT4_MAX_CLUSTER_LOG_SIZE	30
 #ifdef __KERNEL__
 # define EXT4_BLOCK_SIZE(s)		((s)->s_blocksize)
 #else
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 35ccbdc2d64e..0f9ae4ce33d6 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3567,7 +3567,15 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	if (blocksize < EXT4_MIN_BLOCK_SIZE ||
 	    blocksize > EXT4_MAX_BLOCK_SIZE) {
 		ext4_msg(sb, KERN_ERR,
-		       "Unsupported filesystem blocksize %d", blocksize);
+		       "Unsupported filesystem blocksize %d (%d log_block_size)",
+			 blocksize, le32_to_cpu(es->s_log_block_size));
+		goto failed_mount;
+	}
+	if (le32_to_cpu(es->s_log_block_size) >
+	    (EXT4_MAX_BLOCK_LOG_SIZE - EXT4_MIN_BLOCK_LOG_SIZE)) {
+		ext4_msg(sb, KERN_ERR,
+			 "Invalid log block size: %u",
+			 le32_to_cpu(es->s_log_block_size));
 		goto failed_mount;
 	}
 
@@ -3699,6 +3707,13 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 				 "block size (%d)", clustersize, blocksize);
 			goto failed_mount;
 		}
+		if (le32_to_cpu(es->s_log_cluster_size) >
+		    (EXT4_MAX_CLUSTER_LOG_SIZE - EXT4_MIN_BLOCK_LOG_SIZE)) {
+			ext4_msg(sb, KERN_ERR,
+				 "Invalid log cluster size: %u",
+				 le32_to_cpu(es->s_log_cluster_size));
+			goto failed_mount;
+		}
 		sbi->s_cluster_bits = le32_to_cpu(es->s_log_cluster_size) -
 			le32_to_cpu(es->s_log_block_size);
 		sbi->s_clusters_per_group =
-- 
2.11.0.rc0.7.gbe5a750

[PATCH 2/4] ext4: fix in-superblock mount options processing

[Theodore Ts'o]

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@mit.edu>
Cc: stable@vger.kernel.org
---
 fs/ext4/super.c | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 0f9ae4ce33d6..404e6f3c1bed 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;
@@ -3322,16 +3322,14 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
 	ext4_group_t first_not_zeroed;
 
-	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
-	if (!sbi)
-		goto out_free_orig;
+	if ((data && !orig_data) || !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 +3475,19 @@ 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]) {
+		char *s_mount_opts = kstrndup(sbi->s_es->s_mount_opts,
+					      sizeof(sbi->s_es->s_mount_opts),
+					      GFP_KERNEL);
+		if (!s_mount_opts)
+			goto failed_mount;
+		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);
+		}
+		kfree(s_mount_opts);
 	}
 	sbi->s_def_mount_opt = sbi->s_mount_opt;
 	if (!parse_options((char *) data, sb, &journal_devnum,
@@ -4162,7 +4168,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,
+			 (int) sizeof(sbi->s_es->s_mount_opts),
+			 sbi->s_es->s_mount_opts,
 			 *sbi->s_es->s_mount_opts ? "; " : "", orig_data);
 
 	if (es->s_error_count)
@@ -4241,8 +4249,8 @@ 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);
 	return err ? err : ret;
 }
-- 
2.11.0.rc0.7.gbe5a750

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

[Eric Biggers]

On Fri, Nov 18, 2016 at 01:38:40PM -0500, Theodore Ts'o 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@mit.edu>
> Cc: stable@vger.kernel.org

Reviewed-by: Eric Biggers <ebiggers@google.com>

[PATCH 3/4] ext4: use more strict checks for inodes_per_block on mount

[Theodore Ts'o]

Centralize the checks for inodes_per_block and be more strict to make
sure the inodes_per_block_group can't end up being zero.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Reviewed-by: Andreas Dilger <adilger@dilger.ca>
Cc: stable@vger.kernel.org
---
 fs/ext4/super.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 404e6f3c1bed..689c02df1af4 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3668,12 +3668,16 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 
 	sbi->s_blocks_per_group = le32_to_cpu(es->s_blocks_per_group);
 	sbi->s_inodes_per_group = le32_to_cpu(es->s_inodes_per_group);
-	if (EXT4_INODE_SIZE(sb) == 0 || EXT4_INODES_PER_GROUP(sb) == 0)
-		goto cantfind_ext4;
 
 	sbi->s_inodes_per_block = blocksize / EXT4_INODE_SIZE(sb);
 	if (sbi->s_inodes_per_block == 0)
 		goto cantfind_ext4;
+	if (sbi->s_inodes_per_group < sbi->s_inodes_per_block ||
+	    sbi->s_inodes_per_group > blocksize * 8) {
+		ext4_msg(sb, KERN_ERR, "invalid inodes per group: %lu\n",
+			 sbi->s_blocks_per_group);
+		goto failed_mount;
+	}
 	sbi->s_itb_per_group = sbi->s_inodes_per_group /
 					sbi->s_inodes_per_block;
 	sbi->s_desc_per_block = blocksize / EXT4_DESC_SIZE(sb);
@@ -3756,13 +3760,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	}
 	sbi->s_cluster_ratio = clustersize / blocksize;
 
-	if (sbi->s_inodes_per_group > blocksize * 8) {
-		ext4_msg(sb, KERN_ERR,
-		       "#inodes per group too big: %lu",
-		       sbi->s_inodes_per_group);
-		goto failed_mount;
-	}
-
 	/* Do we have standard group size of clustersize * 8 blocks ? */
 	if (sbi->s_blocks_per_group == clustersize << 3)
 		set_opt2(sb, STD_GROUP_SIZE);
-- 
2.11.0.rc0.7.gbe5a750

Re: [PATCH 3/4] ext4: use more strict checks for inodes_per_block on mount

[Eric Biggers]

On Fri, Nov 18, 2016 at 01:38:41PM -0500, Theodore Ts'o wrote:
> Centralize the checks for inodes_per_block and be more strict to make
> sure the inodes_per_block_group can't end up being zero.

Nit: this should say 's_inodes_per_group', not 'inodes_per_block_group'.
> 
> +	    sbi->s_inodes_per_group > blocksize * 8) {
> +		ext4_msg(sb, KERN_ERR, "invalid inodes per group: %lu\n",
> +			 sbi->s_blocks_per_group);
> +		goto failed_mount;
> +	}

Should print out s_inodes_per_group, not s_blocks_per_group.

Eric

Re: [PATCH 3/4] ext4: use more strict checks for inodes_per_block on mount

[Theodore Ts'o]

On Fri, Nov 18, 2016 at 12:30:46PM -0800, Eric Biggers wrote:
> On Fri, Nov 18, 2016 at 01:38:41PM -0500, Theodore Ts'o wrote:
> > Centralize the checks for inodes_per_block and be more strict to make
> > sure the inodes_per_block_group can't end up being zero.
> 
> Nit: this should say 's_inodes_per_group', not 'inodes_per_block_group'.
> > 
> > +	    sbi->s_inodes_per_group > blocksize * 8) {
> > +		ext4_msg(sb, KERN_ERR, "invalid inodes per group: %lu\n",
> > +			 sbi->s_blocks_per_group);
> > +		goto failed_mount;
> > +	}
> 
> Should print out s_inodes_per_group, not s_blocks_per_group.

Thanks, good catch.  I'll fix both of these.

					- Ted

Re: [PATCH 1/4] ext4: sanity check the block and cluster size at mount time

[Eric Biggers]

On Fri, Nov 18, 2016 at 01:38:39PM -0500, Theodore Ts'o wrote:
> @@ -3567,7 +3567,15 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>  	if (blocksize < EXT4_MIN_BLOCK_SIZE ||
>  	    blocksize > EXT4_MAX_BLOCK_SIZE) {
>  		ext4_msg(sb, KERN_ERR,
> -		       "Unsupported filesystem blocksize %d", blocksize);
> +		       "Unsupported filesystem blocksize %d (%d log_block_size)",
> +			 blocksize, le32_to_cpu(es->s_log_block_size));
> +		goto failed_mount;
> +	}
> +	if (le32_to_cpu(es->s_log_block_size) >
> +	    (EXT4_MAX_BLOCK_LOG_SIZE - EXT4_MIN_BLOCK_LOG_SIZE)) {
> +		ext4_msg(sb, KERN_ERR,
> +			 "Invalid log block size: %u",
> +			 le32_to_cpu(es->s_log_block_size));
>  		goto failed_mount;
>  	}
>  

This isn't validating s_log_block_size until after it's already been used in a
shift, which means the code can have undefined behavior (shift by a value too
large).  Would it make sense to do something like the following instead?
Similarly for the cluster size case.

        blocksize =                                                              
                BLOCK_SIZE << min_t(u32, le32_to_cpu(es->s_log_block_size), 20); 
        if (blocksize < EXT4_MIN_BLOCK_SIZE ||                                   
            blocksize > EXT4_MAX_BLOCK_SIZE) {                                   
                ext4_msg(sb, KERN_ERR,                                           
                        "Unsupported filesystem blocksize %d (%u bits)",         
                         blocksize, le32_to_cpu(es->s_log_block_size));          
                goto failed_mount;                                               
        }                                                                        

Re: [PATCH 1/4] ext4: sanity check the block and cluster size at mount time

[Theodore Ts'o]

On Fri, Nov 18, 2016 at 12:02:46PM -0800, Eric Biggers wrote:
> 
> This isn't validating s_log_block_size until after it's already been used in a
> shift, which means the code can have undefined behavior (shift by a value too
> large).  Would it make sense to do something like the following instead?
> Similarly for the cluster size case.

Well, technically GCC is allowed to do *anything* with undefined
behavior, including forking and exec'ing a process to play larn or
rogue --- but that seems fairly unlikely.  The main reason why I left
things the way it was is beause most of the time we want to print a
more user-friendly message about the blocksize, as opposed to
s_log_block_size.

>         blocksize =                                                              
>                 BLOCK_SIZE << min_t(u32, le32_to_cpu(es->s_log_block_size), 20);

If I was going to do anything at all, it would probably be something like

         blocksize =                                                              
                 BLOCK_SIZE << (le32_to_cpu(es->s_log_block_size) & 0x1F);

...on the theory that a boolean AND operation is going to be faster
and cheaper than a min_t.

						- Ted