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 <[email protected]>
Reported-by: Nikolay Borisov <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
Cc: [email protected]
---
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
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 <[email protected]>
Cc: [email protected]
---
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
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 <[email protected]>
Reviewed-by: Andreas Dilger <[email protected]>
Cc: [email protected]
---
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
The commit "ext4: sanity check the block and cluster size at mount
time" should prevent any problems, but in case the superblock is
modified while the file system is mounted, add an extra safety check
to make sure we won't overrun the allocated buffer.
Signed-off-by: Theodore Ts'o <[email protected]>
---
fs/ext4/super.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 689c02df1af4..2d8a49d74f56 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3195,10 +3195,15 @@ static int count_overhead(struct super_block *sb, ext4_group_t grp,
ext4_set_bit(s++, buf);
count++;
}
- for (j = ext4_bg_num_gdb(sb, grp); j > 0; j--) {
- ext4_set_bit(EXT4_B2C(sbi, s++), buf);
- count++;
+ j = ext4_bg_num_gdb(sb, grp);
+ if (s + j > EXT4_BLOCKS_PER_GROUP(sb)) {
+ ext4_error(sb, "Invalid number of block group "
+ "descriptor blocks: %d", j);
+ j = EXT4_BLOCKS_PER_GROUP(sb) - s;
}
+ count += j;
+ for (; j > 0; j--)
+ ext4_set_bit(EXT4_B2C(sbi, s++), buf);
}
if (!count)
return 0;
--
2.11.0.rc0.7.gbe5a750
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;
}
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 <[email protected]>
> Cc: [email protected]
Reviewed-by: Eric Biggers <[email protected]>
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
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
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