ext4: add more paranoia checking in ext4_expand_extra_isize handling
Theodore Ts'o
maintainerPATCH 1/1 ext4: add more paranoia checking in ext4_expand_extra_isize handling
It's possible to specify a non-zero s_want_extra_isize via debugging option, and this can cause bad things(tm) to happen when using a file system with an inode size of 128 bytes.
Add better checking when the file system is mounted, as well as when we are actually doing the trying to do the inode expansion.
--- fs/ext4/inode.c | 14 ++++++++++++++ fs/ext4/super.c | 21 ++++++++++++--------- 2 files changed, 26 insertions(+), 9 deletions(-)
fs/ext4/inode.c +14 -0
@@ -5569,8 +5569,22 @@ static int __ext4_expand_extra_isize(struct inode *inode, {struct ext4_inode *raw_inode;struct ext4_xattr_ibody_header *header; +unsigned int inode_size =EXT4_INODE_SIZE (inode->i_sb);int error; +/* this was checked at iget time, but double check for good measure */ +if ((EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize > inode_size) || + (ei->i_extra_isize &3 )) { +EXT4_ERROR_INODE (inode,"bad extra_isize %u (inode size %u)" , + ei->i_extra_isize, +EXT4_INODE_SIZE (inode->i_sb)); +return -EFSCORRUPTED ; + } +if ((new_extra_isize < ei->i_extra_size) || + (new_extra_isize <4 ) || + (new_extra_size > inode_size -EXT4_GOOD_OLD_INODE_SIZE )) + retun -EINVAL ;/* Should never happen */ + raw_inode =ext4_raw_inode (iloc); header =IHDR (inode, raw_inode);
fs/ext4/super.c +12 -9
@@ -3545,12 +3545,15 @@ 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; +unsigned def_extra_isize =sizeof (struct ext4_inode ) - +EXT4_GOOD_OLD_INODE_SIZE ; -/* 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 (sbi->s_inode_size ==EXT4_GOOD_OLD_INODE_SIZE ) { + sbi->s_want_extra_isize =0 ; +return ; + } +if (sbi->s_want_extra_isize <4 ) { + sbi->s_want_extra_isize = def_extra_isize;if (ext4_has_feature_extra_isize (sb)) {if (sbi->s_want_extra_isize <le16_to_cpu (es->s_want_extra_isize))
@@ -3563,10 +3566,10 @@ static void ext4_clamp_want_extra_isize(struct super_block *sb) } }/* 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 ; +if ((sbi->s_want_extra_isize > sbi->s_inode_size) || + (EXT4_GOOD_OLD_INODE_SIZE + sbi->s_want_extra_isize > + sbi->s_inode_size)) { + sbi->s_want_extra_isize = def_extra_isize;ext4_msg (sb,KERN_INFO ,"required extra inode space not available" ); }
Signature · 1 line
2.23.0
kbuild test robot
build botI love your patch! Perhaps something to improve:
Hi Theodore,
I love your patch! Perhaps something to improve:
[auto build test WARNING on ext4/dev]
[also build test WARNING on v5.4-rc6 next-20191108]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Theodore-Ts-o/ext4-add-more-paranoia-checking-in-ext4_expand_extra_isize-handling/20191110-005324 base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev config: i386-randconfig-d003-201945 (attached as .config) compiler: gcc-7 (Debian 7.4.0-14) 7.4.0 reproduce: # save the attached .config to linux build tree make ARCH=i386
If you fix the issue, kindly add following tag
All warnings (new ones prefixed by >>):
In file included from include/linux/export.h:44:0,
from include/linux/linkage.h:7,
from include/linux/fs.h:5,
from fs//ext4/inode.c:22:
fs//ext4/inode.c: In function '__ext4_expand_extra_isize':
fs//ext4/inode.c:5576:34: error: 'ei' undeclared (first use in this function)
if ((EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize > inode_size) ||
^
include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
>> fs//ext4/inode.c:5576:2: note: in expansion of macro 'if'
if ((EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize > inode_size) ||
^~
fs//ext4/inode.c:5576:34: note: each undeclared identifier is reported only once for each function it appears in
if ((EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize > inode_size) ||
^
include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
>> fs//ext4/inode.c:5576:2: note: in expansion of macro 'if'
if ((EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize > inode_size) ||
^~
fs//ext4/inode.c:5585:7: error: 'new_extra_size' undeclared (first use in this function); did you mean 'new_extra_isize'?
(new_extra_size > inode_size - EXT4_GOOD_OLD_INODE_SIZE))
^
include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
fs//ext4/inode.c:5583:2: note: in expansion of macro 'if'
if ((new_extra_isize < ei->i_extra_size) ||
^~ fs//ext4/inode.c:5586:3: error: 'retun' undeclared (first use in this function); did you mean 'semun'?
retun -EINVAL; /* Should never happen */
^~~~~
semun
vim +/if +5576 fs//ext4/inode.c
5564
5565 static int __ext4_expand_extra_isize(struct inode *inode,
5566 unsigned int new_extra_isize,
5567 struct ext4_iloc *iloc,
5568 handle_t *handle, int *no_expand)
5569 {
5570 struct ext4_inode *raw_inode;
5571 struct ext4_xattr_ibody_header *header;
5572 unsigned int inode_size = EXT4_INODE_SIZE(inode->i_sb);
5573 int error;
5574
5575 /* this was checked at iget time, but double check for good measure */
> 5576 if ((EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize > inode_size) || 5577 (ei->i_extra_isize & 3)) {
5578 EXT4_ERROR_INODE(inode, "bad extra_isize %u (inode size %u)",
5579 ei->i_extra_isize,
5580 EXT4_INODE_SIZE(inode->i_sb));
5581 return -EFSCORRUPTED;
5582 }
5583 if ((new_extra_isize < ei->i_extra_size) ||
5584 (new_extra_isize < 4) ||
5585 (new_extra_size > inode_size - EXT4_GOOD_OLD_INODE_SIZE))
5586 retun -EINVAL; /* Should never happen */
5587
5588 raw_inode = ext4_raw_inode(iloc);
5589
5590 header = IHDR(inode, raw_inode);
5591
5592 /* No extended attributes present */
5593 if (!ext4_test_inode_state(inode, EXT4_STATE_XATTR) ||
5594 header->h_magic != cpu_to_le32(EXT4_XATTR_MAGIC)) {
5595 memset((void *)raw_inode + EXT4_GOOD_OLD_INODE_SIZE +
5596 EXT4_I(inode)->i_extra_isize, 0,
5597 new_extra_isize - EXT4_I(inode)->i_extra_isize);
5598 EXT4_I(inode)->i_extra_isize = new_extra_isize;
5599 return 0;
5600 }
5601
5602 /* try to expand with EAs present */
5603 error = ext4_expand_extra_isize_ea(inode, new_extra_isize,
5604 raw_inode, handle);
5605 if (error) {
5606 /*
5607 * Inode size expansion failed; don't try again
5608 */
5609 *no_expand = 1;
5610 }
5611
5612 return error;
5613 }
5614
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation
Theodore Y. Ts'o
maintainerPATCH 1/1 ext4: add more paranoia checking in ext4_expand_extra_isize handling
I hadn't gotten around to resending the patch. The original version had a number of last-minute typos that had crept in...
- Ted
Quoted 1 line
From
a67ad537964d10f94a4b990c084365e75316cde8Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <[email protected]>
Date: Thu, 7 Nov 2019 21:43:41 -0500
Subject: [PATCH] ext4: add more paranoia checking in ext4_expand_extra_isize
handling
It's possible to specify a non-zero s_want_extra_isize via debugging option, and this can cause bad things(tm) to happen when using a file system with an inode size of 128 bytes.
Add better checking when the file system is mounted, as well as when we are actually doing the trying to do the inode expansion.
--- fs/ext4/inode.c | 15 +++++++++++++++ fs/ext4/super.c | 21 ++++++++++++--------- 2 files changed, 27 insertions(+), 9 deletions(-)
fs/ext4/inode.c +15 -0
@@ -5569,8 +5569,23 @@ static int __ext4_expand_extra_isize(struct inode *inode, {struct ext4_inode *raw_inode;struct ext4_xattr_ibody_header *header; +unsigned int inode_size =EXT4_INODE_SIZE (inode->i_sb); +struct ext4_inode_info *ei =EXT4_I (inode);int error; +/* this was checked at iget time, but double check for good measure */ +if ((EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize > inode_size) || + (ei->i_extra_isize &3 )) { +EXT4_ERROR_INODE (inode,"bad extra_isize %u (inode size %u)" , + ei->i_extra_isize, +EXT4_INODE_SIZE (inode->i_sb)); +return -EFSCORRUPTED ; + } +if ((new_extra_isize < ei->i_extra_isize) || + (new_extra_isize <4 ) || + (new_extra_isize > inode_size -EXT4_GOOD_OLD_INODE_SIZE )) +return -EINVAL ;/* Should never happen */ + raw_inode =ext4_raw_inode (iloc); header =IHDR (inode, raw_inode);
fs/ext4/super.c +12 -9
@@ -3545,12 +3545,15 @@ 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; +unsigned def_extra_isize =sizeof (struct ext4_inode ) - +EXT4_GOOD_OLD_INODE_SIZE ; -/* 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 (sbi->s_inode_size ==EXT4_GOOD_OLD_INODE_SIZE ) { + sbi->s_want_extra_isize =0 ; +return ; + } +if (sbi->s_want_extra_isize <4 ) { + sbi->s_want_extra_isize = def_extra_isize;if (ext4_has_feature_extra_isize (sb)) {if (sbi->s_want_extra_isize <le16_to_cpu (es->s_want_extra_isize))
@@ -3563,10 +3566,10 @@ static void ext4_clamp_want_extra_isize(struct super_block *sb) } }/* 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 ; +if ((sbi->s_want_extra_isize > sbi->s_inode_size) || + (EXT4_GOOD_OLD_INODE_SIZE + sbi->s_want_extra_isize > + sbi->s_inode_size)) { + sbi->s_want_extra_isize = def_extra_isize;ext4_msg (sb,KERN_INFO ,"required extra inode space not available" ); }
Signature · 1 line
2.23.0
Quoted 16 lines · Theodore Y. Ts'o
I hadn't gotten around to resending the patch. The original version had a number of last-minute typos that had crept in...
- Ted
From
a67ad537964d10f94a4b990c084365e75316cde8Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <[email protected]>
Date: Thu, 7 Nov 2019 21:43:41 -0500
Subject: [PATCH] ext4: add more paranoia checking inext4_expand_extra_isize
handlingIt's possible to specify a non-zero
s_want_extra_isizevia debugging option, and this can cause bad things(tm) to happen when using a file system with an inode size of 128 bytes.
Quoted 6 lines
Add better checking when the file system is mounted, as well as when we are actually doing the trying to do the inode expansion.
Reported-by syzbot+f8d6f8386ceacdbfff57(a)syzkaller.appspotmail.comSigned-off-by Theodore Ts'o <[email protected]>Cc stable(a)kernel.org
Is this patch intended to address
https://lore.kernel.org/linux-ext4/000000000000950f21059564e4c7(a)google.com
as well? If so, you can add the second Reported-by line so that both syzbot reports get closed.
- Eric
Theodore Y. Ts'o
maintainerQuoted 4 lines · Eric Biggers
Is this patch intended to address
https://lore.kernel.org/linux-ext4/000000000000950f21059564e4c7(a)google.com
as well? If so, you can add the second Reported-by line so that both syzbot reports get closed.
Yes, it appears to be the same issue. Thanks for pointing this out!
- Ted
Quoted 9 lines · Theodore Y. Ts'o
Quoted 4 lines · Eric Biggers
Is this patch intended to address
https://lore.kernel.org/linux-ext4/000000000000950f21059564e4c7(a)google.com
as well? If so, you can add the second Reported-by line so that both syzbot reports get closed.Yes, it appears to be the same issue. Thanks for pointing this out!
- Ted
There's actually a third that looks very similar too:
"KASAN: use-after-free Write in __ext4_expand_extra_isize (2)"
https://lkml.kernel.org/linux-ext4/0000000000000d74c7059564e17f(a)google.com/
If these are all fixed by this patch, you can use: