Linux-ext4 Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] ext4: add more paranoia checking in ext4_expand_extra_isize handling
@ 2019-11-08  2:48 Theodore Ts'o
  2019-11-10 10:12 ` kbuild test robot
  0 siblings, 1 reply; 6+ messages in thread
From: Theodore Ts'o @ 2019-11-08  2:48 UTC (permalink / raw)
  To: Ext4 Developers List
  Cc: Theodore Ts'o, syzbot+f8d6f8386ceacdbfff57, stable

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.

Reported-by: syzbot+f8d6f8386ceacdbfff57@syzkaller.appspotmail.com
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Cc: stable@kernel.org
---
 fs/ext4/inode.c | 14 ++++++++++++++
 fs/ext4/super.c | 21 ++++++++++++---------
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 381813205f99..84de97bc499e 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -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);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 7796e2ffc294..71af8780d4ee 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -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");
 	}
-- 
2.23.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ext4: add more paranoia checking in ext4_expand_extra_isize handling
  2019-11-08  2:48 [PATCH] ext4: add more paranoia checking in ext4_expand_extra_isize handling Theodore Ts'o
@ 2019-11-10 10:12 ` kbuild test robot
  2019-11-10 12:15   ` [PATCH -v2] " Theodore Y. Ts'o
  0 siblings, 1 reply; 6+ messages in thread
From: kbuild test robot @ 2019-11-10 10:12 UTC (permalink / raw)
  To: Theodore Ts'o
  Cc: kbuild-all, Ext4 Developers List, Theodore Ts'o,
	syzbot+f8d6f8386ceacdbfff57, stable

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

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
Reported-by: kbuild test robot <lkp@intel.com>

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@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38678 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH -v2] ext4: add more paranoia checking in ext4_expand_extra_isize handling
  2019-11-10 10:12 ` kbuild test robot
@ 2019-11-10 12:15   ` " Theodore Y. Ts'o
  2019-11-19  2:15     ` Eric Biggers
  0 siblings, 1 reply; 6+ messages in thread
From: Theodore Y. Ts'o @ 2019-11-10 12:15 UTC (permalink / raw)
  To: kbuild test robot
  Cc: kbuild-all, Ext4 Developers List, syzbot+f8d6f8386ceacdbfff57, stable

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 a67ad537964d10f94a4b990c084365e75316cde8 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <tytso@mit.edu>
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.

Reported-by: syzbot+f8d6f8386ceacdbfff57@syzkaller.appspotmail.com
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Cc: stable@kernel.org
---
 fs/ext4/inode.c | 15 +++++++++++++++
 fs/ext4/super.c | 21 ++++++++++++---------
 2 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 381813205f99..c6e3fe287b50 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -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);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 7796e2ffc294..71af8780d4ee 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -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");
 	}
-- 
2.23.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH -v2] ext4: add more paranoia checking in ext4_expand_extra_isize handling
  2019-11-10 12:15   ` [PATCH -v2] " Theodore Y. Ts'o
@ 2019-11-19  2:15     ` Eric Biggers
  2019-11-19  4:21       ` Theodore Y. Ts'o
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Biggers @ 2019-11-19  2:15 UTC (permalink / raw)
  To: Theodore Y. Ts'o
  Cc: kbuild test robot, kbuild-all, Ext4 Developers List,
	syzbot+f8d6f8386ceacdbfff57, stable

On Sun, Nov 10, 2019 at 07:15:10AM -0500, Theodore Y. Ts'o wrote:
> 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 a67ad537964d10f94a4b990c084365e75316cde8 Mon Sep 17 00:00:00 2001
> From: Theodore Ts'o <tytso@mit.edu>
> 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.
> 
> Reported-by: syzbot+f8d6f8386ceacdbfff57@syzkaller.appspotmail.com
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> Cc: stable@kernel.org

Is this patch intended to address
https://lore.kernel.org/linux-ext4/000000000000950f21059564e4c7@google.com
as well?  If so, you can add the second Reported-by line so that both syzbot
reports get closed.

- Eric

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH -v2] ext4: add more paranoia checking in ext4_expand_extra_isize handling
  2019-11-19  2:15     ` Eric Biggers
@ 2019-11-19  4:21       ` Theodore Y. Ts'o
  2019-11-19  4:36         ` Eric Biggers
  0 siblings, 1 reply; 6+ messages in thread
From: Theodore Y. Ts'o @ 2019-11-19  4:21 UTC (permalink / raw)
  To: Eric Biggers
  Cc: kbuild test robot, kbuild-all, Ext4 Developers List,
	syzbot+f8d6f8386ceacdbfff57, stable

On Mon, Nov 18, 2019 at 06:15:26PM -0800, Eric Biggers wrote:
> Is this patch intended to address
> https://lore.kernel.org/linux-ext4/000000000000950f21059564e4c7@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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH -v2] ext4: add more paranoia checking in ext4_expand_extra_isize handling
  2019-11-19  4:21       ` Theodore Y. Ts'o
@ 2019-11-19  4:36         ` Eric Biggers
  0 siblings, 0 replies; 6+ messages in thread
From: Eric Biggers @ 2019-11-19  4:36 UTC (permalink / raw)
  To: Theodore Y. Ts'o
  Cc: kbuild test robot, kbuild-all, Ext4 Developers List,
	syzbot+f8d6f8386ceacdbfff57, stable

On Mon, Nov 18, 2019 at 11:21:20PM -0500, Theodore Y. Ts'o wrote:
> On Mon, Nov 18, 2019 at 06:15:26PM -0800, Eric Biggers wrote:
> > Is this patch intended to address
> > https://lore.kernel.org/linux-ext4/000000000000950f21059564e4c7@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@google.com/

If these are all fixed by this patch, you can use:

Reported-by: syzbot+f8d6f8386ceacdbfff57@syzkaller.appspotmail.com
Reported-by: syzbot+33d7ea72e47de3bdf4e1@syzkaller.appspotmail.com
Reported-by: syzbot+44b6763edfc17144296f@syzkaller.appspotmail.com

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-08  2:48 [PATCH] ext4: add more paranoia checking in ext4_expand_extra_isize handling Theodore Ts'o
2019-11-10 10:12 ` kbuild test robot
2019-11-10 12:15   ` [PATCH -v2] " Theodore Y. Ts'o
2019-11-19  2:15     ` Eric Biggers
2019-11-19  4:21       ` Theodore Y. Ts'o
2019-11-19  4:36         ` Eric Biggers

Linux-ext4 Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-ext4/0 linux-ext4/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-ext4 linux-ext4/ https://lore.kernel.org/linux-ext4 \
		linux-ext4@vger.kernel.org
	public-inbox-index linux-ext4

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-ext4


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git