* [PATCH] ext4: fix use-after-free race with debug_want_extra_isize
@ 2019-04-15 21:19 Barret Rhoden
2019-04-18 11:55 ` Jan Kara
2019-04-18 15:59 ` [PATCH v2] " Barret Rhoden
0 siblings, 2 replies; 5+ messages in thread
From: Barret Rhoden @ 2019-04-15 21:19 UTC (permalink / raw)
To: Theodore Ts'o, Andreas Dilger
Cc: syzbot+f584efa0ac7213c226b7, stable, linux-ext4, linux-kernel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] ext4: fix use-after-free race with debug_want_extra_isize
2019-04-15 21:19 [PATCH] ext4: fix use-after-free race with debug_want_extra_isize Barret Rhoden
@ 2019-04-18 11:55 ` Jan Kara
2019-04-18 15:59 ` [PATCH v2] " Barret Rhoden
1 sibling, 0 replies; 5+ messages in thread
From: Jan Kara @ 2019-04-18 11:55 UTC (permalink / raw)
To: Barret Rhoden
Cc: Theodore Ts'o, Andreas Dilger, syzbot+f584efa0ac7213c226b7,
stable, linux-ext4, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH v2] ext4: fix use-after-free race with debug_want_extra_isize
2019-04-15 21:19 [PATCH] ext4: fix use-after-free race with debug_want_extra_isize Barret Rhoden
2019-04-18 11:55 ` Jan Kara
@ 2019-04-18 15:59 ` Barret Rhoden
2019-04-18 19:00 ` Andreas Dilger
2019-04-25 15:57 ` Theodore Ts'o
1 sibling, 2 replies; 5+ messages in thread
From: Barret Rhoden @ 2019-04-18 15:59 UTC (permalink / raw)
To: Theodore Ts'o, Andreas Dilger
Cc: syzbot+f584efa0ac7213c226b7, Jan Kara, stable, linux-ext4, linux-kernel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v2] ext4: fix use-after-free race with debug_want_extra_isize
2019-04-18 15:59 ` [PATCH v2] " Barret Rhoden
@ 2019-04-18 19:00 ` Andreas Dilger
2019-04-25 15:57 ` Theodore Ts'o
1 sibling, 0 replies; 5+ messages in thread
From: Andreas Dilger @ 2019-04-18 19:00 UTC (permalink / raw)
To: Barret Rhoden
Cc: Theodore Ts'o, syzbot+f584efa0ac7213c226b7, Jan Kara, stable,
linux-ext4, open list
[-- 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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] ext4: fix use-after-free race with debug_want_extra_isize
2019-04-18 15:59 ` [PATCH v2] " Barret Rhoden
2019-04-18 19:00 ` Andreas Dilger
@ 2019-04-25 15:57 ` Theodore Ts'o
1 sibling, 0 replies; 5+ messages in thread
From: Theodore Ts'o @ 2019-04-25 15:57 UTC (permalink / raw)
To: Barret Rhoden
Cc: Andreas Dilger, syzbot+f584efa0ac7213c226b7, Jan Kara, stable,
linux-ext4, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-04-25 15:58 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-15 21:19 [PATCH] ext4: fix use-after-free race with debug_want_extra_isize Barret Rhoden
2019-04-18 11:55 ` Jan Kara
2019-04-18 15:59 ` [PATCH v2] " Barret Rhoden
2019-04-18 19:00 ` Andreas Dilger
2019-04-25 15:57 ` Theodore Ts'o
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.