[PATCH] ext4: fix use-after-free race with debug_want_extra_isize

[PATCH] ext4: fix use-after-free race with debug_want_extra_isize

[Barret Rhoden]

When remounting with debug_want_extra_isize, we were not performing the
same checks that we do during a normal mount.  That allowed us to set a
value for s_want_extra_isize that reached outside the s_inode_size.

Reported-by: syzbot+f584efa0ac7213c226b7@syzkaller.appspotmail.com
Signed-off-by: Barret Rhoden <brho@google.com>
Cc: stable@vger.kernel.org
---

- In the current code, it looks like someone could mount with want_extra_isize
  with some value > 0 but less than the minimums in the s_es.  If that's a bug,
  I can submit a follow-on patch.

- Similarly, on a failed remount, sbi->s_want_extra_isize is changed to the
  remounted value.  I can fix that too if it's a problem.

- Is it OK to remount with a smaller s_want_extra_isize than the previous mount?
  I thought it was, but figured I'd ask while I'm looking at it.


 fs/ext4/super.c | 58 +++++++++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 24 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 6ed4eb81e674..184944d4d8d1 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3513,6 +3513,37 @@ int ext4_calculate_overhead(struct super_block *sb)
 	return 0;
 }
 
+static void ext4_clamp_want_extra_isize(struct super_block *sb)
+{
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	struct ext4_super_block *es = sbi->s_es;
+
+	/* determine the minimum size of new large inodes, if present */
+	if (sbi->s_inode_size > EXT4_GOOD_OLD_INODE_SIZE &&
+	    sbi->s_want_extra_isize == 0) {
+		sbi->s_want_extra_isize = sizeof(struct ext4_inode) -
+						     EXT4_GOOD_OLD_INODE_SIZE;
+		if (ext4_has_feature_extra_isize(sb)) {
+			if (sbi->s_want_extra_isize <
+			    le16_to_cpu(es->s_want_extra_isize))
+				sbi->s_want_extra_isize =
+					le16_to_cpu(es->s_want_extra_isize);
+			if (sbi->s_want_extra_isize <
+			    le16_to_cpu(es->s_min_extra_isize))
+				sbi->s_want_extra_isize =
+					le16_to_cpu(es->s_min_extra_isize);
+		}
+	}
+	/* Check if enough inode space is available */
+	if (EXT4_GOOD_OLD_INODE_SIZE + sbi->s_want_extra_isize >
+							sbi->s_inode_size) {
+		sbi->s_want_extra_isize = sizeof(struct ext4_inode) -
+						       EXT4_GOOD_OLD_INODE_SIZE;
+		ext4_msg(sb, KERN_INFO,
+			 "required extra inode space not available");
+	}
+}
+
 static void ext4_set_resv_clusters(struct super_block *sb)
 {
 	ext4_fsblk_t resv_clusters;
@@ -4387,30 +4418,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	} else if (ret)
 		goto failed_mount4a;
 
-	/* determine the minimum size of new large inodes, if present */
-	if (sbi->s_inode_size > EXT4_GOOD_OLD_INODE_SIZE &&
-	    sbi->s_want_extra_isize == 0) {
-		sbi->s_want_extra_isize = sizeof(struct ext4_inode) -
-						     EXT4_GOOD_OLD_INODE_SIZE;
-		if (ext4_has_feature_extra_isize(sb)) {
-			if (sbi->s_want_extra_isize <
-			    le16_to_cpu(es->s_want_extra_isize))
-				sbi->s_want_extra_isize =
-					le16_to_cpu(es->s_want_extra_isize);
-			if (sbi->s_want_extra_isize <
-			    le16_to_cpu(es->s_min_extra_isize))
-				sbi->s_want_extra_isize =
-					le16_to_cpu(es->s_min_extra_isize);
-		}
-	}
-	/* Check if enough inode space is available */
-	if (EXT4_GOOD_OLD_INODE_SIZE + sbi->s_want_extra_isize >
-							sbi->s_inode_size) {
-		sbi->s_want_extra_isize = sizeof(struct ext4_inode) -
-						       EXT4_GOOD_OLD_INODE_SIZE;
-		ext4_msg(sb, KERN_INFO, "required extra inode space not"
-			 "available");
-	}
+	ext4_clamp_want_extra_isize(sb);
 
 	ext4_set_resv_clusters(sb);
 
@@ -5194,6 +5202,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
 		goto restore_opts;
 	}
 
+	ext4_clamp_want_extra_isize(sb);
+
 	if ((old_opts.s_mount_opt & EXT4_MOUNT_JOURNAL_CHECKSUM) ^
 	    test_opt(sb, JOURNAL_CHECKSUM)) {
 		ext4_msg(sb, KERN_ERR, "changing journal_checksum "
-- 
2.21.0.392.gf8f6787159e-goog

Re: [PATCH] ext4: fix use-after-free race with debug_want_extra_isize

[Jan Kara]

On Mon 15-04-19 17:19:45, Barret Rhoden wrote:
> When remounting with debug_want_extra_isize, we were not performing the
> same checks that we do during a normal mount.  That allowed us to set a
> value for s_want_extra_isize that reached outside the s_inode_size.
> 
> Reported-by: syzbot+f584efa0ac7213c226b7@syzkaller.appspotmail.com
> Signed-off-by: Barret Rhoden <brho@google.com>
> Cc: stable@vger.kernel.org

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
> 
> - In the current code, it looks like someone could mount with want_extra_isize
>   with some value > 0 but less than the minimums in the s_es.  If that's a bug,
>   I can submit a follow-on patch.
> 
> - Similarly, on a failed remount, sbi->s_want_extra_isize is changed to the
>   remounted value.  I can fix that too if it's a problem.
> 
> - Is it OK to remount with a smaller s_want_extra_isize than the previous mount?
>   I thought it was, but figured I'd ask while I'm looking at it.
> 
> 
>  fs/ext4/super.c | 58 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 34 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 6ed4eb81e674..184944d4d8d1 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3513,6 +3513,37 @@ int ext4_calculate_overhead(struct super_block *sb)
>  	return 0;
>  }
>  
> +static void ext4_clamp_want_extra_isize(struct super_block *sb)
> +{
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	struct ext4_super_block *es = sbi->s_es;
> +
> +	/* determine the minimum size of new large inodes, if present */
> +	if (sbi->s_inode_size > EXT4_GOOD_OLD_INODE_SIZE &&
> +	    sbi->s_want_extra_isize == 0) {
> +		sbi->s_want_extra_isize = sizeof(struct ext4_inode) -
> +						     EXT4_GOOD_OLD_INODE_SIZE;
> +		if (ext4_has_feature_extra_isize(sb)) {
> +			if (sbi->s_want_extra_isize <
> +			    le16_to_cpu(es->s_want_extra_isize))
> +				sbi->s_want_extra_isize =
> +					le16_to_cpu(es->s_want_extra_isize);
> +			if (sbi->s_want_extra_isize <
> +			    le16_to_cpu(es->s_min_extra_isize))
> +				sbi->s_want_extra_isize =
> +					le16_to_cpu(es->s_min_extra_isize);
> +		}
> +	}
> +	/* Check if enough inode space is available */
> +	if (EXT4_GOOD_OLD_INODE_SIZE + sbi->s_want_extra_isize >
> +							sbi->s_inode_size) {
> +		sbi->s_want_extra_isize = sizeof(struct ext4_inode) -
> +						       EXT4_GOOD_OLD_INODE_SIZE;
> +		ext4_msg(sb, KERN_INFO,
> +			 "required extra inode space not available");
> +	}
> +}
> +
>  static void ext4_set_resv_clusters(struct super_block *sb)
>  {
>  	ext4_fsblk_t resv_clusters;
> @@ -4387,30 +4418,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>  	} else if (ret)
>  		goto failed_mount4a;
>  
> -	/* determine the minimum size of new large inodes, if present */
> -	if (sbi->s_inode_size > EXT4_GOOD_OLD_INODE_SIZE &&
> -	    sbi->s_want_extra_isize == 0) {
> -		sbi->s_want_extra_isize = sizeof(struct ext4_inode) -
> -						     EXT4_GOOD_OLD_INODE_SIZE;
> -		if (ext4_has_feature_extra_isize(sb)) {
> -			if (sbi->s_want_extra_isize <
> -			    le16_to_cpu(es->s_want_extra_isize))
> -				sbi->s_want_extra_isize =
> -					le16_to_cpu(es->s_want_extra_isize);
> -			if (sbi->s_want_extra_isize <
> -			    le16_to_cpu(es->s_min_extra_isize))
> -				sbi->s_want_extra_isize =
> -					le16_to_cpu(es->s_min_extra_isize);
> -		}
> -	}
> -	/* Check if enough inode space is available */
> -	if (EXT4_GOOD_OLD_INODE_SIZE + sbi->s_want_extra_isize >
> -							sbi->s_inode_size) {
> -		sbi->s_want_extra_isize = sizeof(struct ext4_inode) -
> -						       EXT4_GOOD_OLD_INODE_SIZE;
> -		ext4_msg(sb, KERN_INFO, "required extra inode space not"
> -			 "available");
> -	}
> +	ext4_clamp_want_extra_isize(sb);
>  
>  	ext4_set_resv_clusters(sb);
>  
> @@ -5194,6 +5202,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
>  		goto restore_opts;
>  	}
>  
> +	ext4_clamp_want_extra_isize(sb);
> +
>  	if ((old_opts.s_mount_opt & EXT4_MOUNT_JOURNAL_CHECKSUM) ^
>  	    test_opt(sb, JOURNAL_CHECKSUM)) {
>  		ext4_msg(sb, KERN_ERR, "changing journal_checksum "
> -- 
> 2.21.0.392.gf8f6787159e-goog
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v2] ext4: fix use-after-free race with debug_want_extra_isize

[Barret Rhoden]

When remounting with debug_want_extra_isize, we were not performing the
same checks that we do during a normal mount.  That allowed us to set a
value for s_want_extra_isize that reached outside the s_inode_size.

Reported-by: syzbot+f584efa0ac7213c226b7@syzkaller.appspotmail.com
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Barret Rhoden <brho@google.com>
Cc: stable@vger.kernel.org # 4.14.111
---

- Updated tags

Thanks for the review!

 fs/ext4/super.c | 58 +++++++++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 24 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 6ed4eb81e674..184944d4d8d1 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3513,6 +3513,37 @@ int ext4_calculate_overhead(struct super_block *sb)
 	return 0;
 }
 
+static void ext4_clamp_want_extra_isize(struct super_block *sb)
+{
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	struct ext4_super_block *es = sbi->s_es;
+
+	/* determine the minimum size of new large inodes, if present */
+	if (sbi->s_inode_size > EXT4_GOOD_OLD_INODE_SIZE &&
+	    sbi->s_want_extra_isize == 0) {
+		sbi->s_want_extra_isize = sizeof(struct ext4_inode) -
+						     EXT4_GOOD_OLD_INODE_SIZE;
+		if (ext4_has_feature_extra_isize(sb)) {
+			if (sbi->s_want_extra_isize <
+			    le16_to_cpu(es->s_want_extra_isize))
+				sbi->s_want_extra_isize =
+					le16_to_cpu(es->s_want_extra_isize);
+			if (sbi->s_want_extra_isize <
+			    le16_to_cpu(es->s_min_extra_isize))
+				sbi->s_want_extra_isize =
+					le16_to_cpu(es->s_min_extra_isize);
+		}
+	}
+	/* Check if enough inode space is available */
+	if (EXT4_GOOD_OLD_INODE_SIZE + sbi->s_want_extra_isize >
+							sbi->s_inode_size) {
+		sbi->s_want_extra_isize = sizeof(struct ext4_inode) -
+						       EXT4_GOOD_OLD_INODE_SIZE;
+		ext4_msg(sb, KERN_INFO,
+			 "required extra inode space not available");
+	}
+}
+
 static void ext4_set_resv_clusters(struct super_block *sb)
 {
 	ext4_fsblk_t resv_clusters;
@@ -4387,30 +4418,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	} else if (ret)
 		goto failed_mount4a;
 
-	/* determine the minimum size of new large inodes, if present */
-	if (sbi->s_inode_size > EXT4_GOOD_OLD_INODE_SIZE &&
-	    sbi->s_want_extra_isize == 0) {
-		sbi->s_want_extra_isize = sizeof(struct ext4_inode) -
-						     EXT4_GOOD_OLD_INODE_SIZE;
-		if (ext4_has_feature_extra_isize(sb)) {
-			if (sbi->s_want_extra_isize <
-			    le16_to_cpu(es->s_want_extra_isize))
-				sbi->s_want_extra_isize =
-					le16_to_cpu(es->s_want_extra_isize);
-			if (sbi->s_want_extra_isize <
-			    le16_to_cpu(es->s_min_extra_isize))
-				sbi->s_want_extra_isize =
-					le16_to_cpu(es->s_min_extra_isize);
-		}
-	}
-	/* Check if enough inode space is available */
-	if (EXT4_GOOD_OLD_INODE_SIZE + sbi->s_want_extra_isize >
-							sbi->s_inode_size) {
-		sbi->s_want_extra_isize = sizeof(struct ext4_inode) -
-						       EXT4_GOOD_OLD_INODE_SIZE;
-		ext4_msg(sb, KERN_INFO, "required extra inode space not"
-			 "available");
-	}
+	ext4_clamp_want_extra_isize(sb);
 
 	ext4_set_resv_clusters(sb);
 
@@ -5194,6 +5202,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
 		goto restore_opts;
 	}
 
+	ext4_clamp_want_extra_isize(sb);
+
 	if ((old_opts.s_mount_opt & EXT4_MOUNT_JOURNAL_CHECKSUM) ^
 	    test_opt(sb, JOURNAL_CHECKSUM)) {
 		ext4_msg(sb, KERN_ERR, "changing journal_checksum "
-- 
2.21.0.392.gf8f6787159e-goog

Re: [PATCH v2] ext4: fix use-after-free race with debug_want_extra_isize

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 3904 bytes --]

On Apr 18, 2019, at 9:59 AM, Barret Rhoden <brho@google.com> wrote:
> 
> When remounting with debug_want_extra_isize, we were not performing the
> same checks that we do during a normal mount.  That allowed us to set a
> value for s_want_extra_isize that reached outside the s_inode_size.
> 
> Reported-by: syzbot+f584efa0ac7213c226b7@syzkaller.appspotmail.com
> Reviewed-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Barret Rhoden <brho@google.com>
> Cc: stable@vger.kernel.org # 4.14.111

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> ---
> 
> - Updated tags
> 
> Thanks for the review!
> 
> fs/ext4/super.c | 58 +++++++++++++++++++++++++++++--------------------
> 1 file changed, 34 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 6ed4eb81e674..184944d4d8d1 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3513,6 +3513,37 @@ int ext4_calculate_overhead(struct super_block *sb)
> 	return 0;
> }
> 
> +static void ext4_clamp_want_extra_isize(struct super_block *sb)
> +{
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	struct ext4_super_block *es = sbi->s_es;
> +
> +	/* determine the minimum size of new large inodes, if present */
> +	if (sbi->s_inode_size > EXT4_GOOD_OLD_INODE_SIZE &&
> +	    sbi->s_want_extra_isize == 0) {
> +		sbi->s_want_extra_isize = sizeof(struct ext4_inode) -
> +						     EXT4_GOOD_OLD_INODE_SIZE;
> +		if (ext4_has_feature_extra_isize(sb)) {
> +			if (sbi->s_want_extra_isize <
> +			    le16_to_cpu(es->s_want_extra_isize))
> +				sbi->s_want_extra_isize =
> +					le16_to_cpu(es->s_want_extra_isize);
> +			if (sbi->s_want_extra_isize <
> +			    le16_to_cpu(es->s_min_extra_isize))
> +				sbi->s_want_extra_isize =
> +					le16_to_cpu(es->s_min_extra_isize);
> +		}
> +	}
> +	/* Check if enough inode space is available */
> +	if (EXT4_GOOD_OLD_INODE_SIZE + sbi->s_want_extra_isize >
> +							sbi->s_inode_size) {
> +		sbi->s_want_extra_isize = sizeof(struct ext4_inode) -
> +						       EXT4_GOOD_OLD_INODE_SIZE;
> +		ext4_msg(sb, KERN_INFO,
> +			 "required extra inode space not available");
> +	}
> +}
> +
> static void ext4_set_resv_clusters(struct super_block *sb)
> {
> 	ext4_fsblk_t resv_clusters;
> @@ -4387,30 +4418,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> 	} else if (ret)
> 		goto failed_mount4a;
> 
> -	/* determine the minimum size of new large inodes, if present */
> -	if (sbi->s_inode_size > EXT4_GOOD_OLD_INODE_SIZE &&
> -	    sbi->s_want_extra_isize == 0) {
> -		sbi->s_want_extra_isize = sizeof(struct ext4_inode) -
> -						     EXT4_GOOD_OLD_INODE_SIZE;
> -		if (ext4_has_feature_extra_isize(sb)) {
> -			if (sbi->s_want_extra_isize <
> -			    le16_to_cpu(es->s_want_extra_isize))
> -				sbi->s_want_extra_isize =
> -					le16_to_cpu(es->s_want_extra_isize);
> -			if (sbi->s_want_extra_isize <
> -			    le16_to_cpu(es->s_min_extra_isize))
> -				sbi->s_want_extra_isize =
> -					le16_to_cpu(es->s_min_extra_isize);
> -		}
> -	}
> -	/* Check if enough inode space is available */
> -	if (EXT4_GOOD_OLD_INODE_SIZE + sbi->s_want_extra_isize >
> -							sbi->s_inode_size) {
> -		sbi->s_want_extra_isize = sizeof(struct ext4_inode) -
> -						       EXT4_GOOD_OLD_INODE_SIZE;
> -		ext4_msg(sb, KERN_INFO, "required extra inode space not"
> -			 "available");
> -	}
> +	ext4_clamp_want_extra_isize(sb);
> 
> 	ext4_set_resv_clusters(sb);
> 
> @@ -5194,6 +5202,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
> 		goto restore_opts;
> 	}
> 
> +	ext4_clamp_want_extra_isize(sb);
> +
> 	if ((old_opts.s_mount_opt & EXT4_MOUNT_JOURNAL_CHECKSUM) ^
> 	    test_opt(sb, JOURNAL_CHECKSUM)) {
> 		ext4_msg(sb, KERN_ERR, "changing journal_checksum "
> --
> 2.21.0.392.gf8f6787159e-goog
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH v2] ext4: fix use-after-free race with debug_want_extra_isize

[Theodore Ts'o]

On Thu, Apr 18, 2019 at 11:59:37AM -0400, Barret Rhoden wrote:
> When remounting with debug_want_extra_isize, we were not performing the
> same checks that we do during a normal mount.  That allowed us to set a
> value for s_want_extra_isize that reached outside the s_inode_size.
> 
> Reported-by: syzbot+f584efa0ac7213c226b7@syzkaller.appspotmail.com
> Reviewed-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Barret Rhoden <brho@google.com>
> Cc: stable@vger.kernel.org # 4.14.111

Applied, thanks.

					- Ted