[PATCH 1/3] ext4: forbid i_extra_isize not divisible by 4

[PATCH 1/3] ext4: forbid i_extra_isize not divisible by 4

[Eric Biggers]

i_extra_isize not divisible by 4 is problematic for several reasons:

- It causes the in-inode xattr space to be misaligned, but the xattr
  header and entries are not declared __packed to express this
  possibility.  This may cause poor performance or incorrect code
  generation on some platforms.
- When validating the xattr entries we can read past the end of the
  inode if the size available for xattrs is not a multiple of 4.
- It allows the nonsensical i_extra_isize=1, which doesn't even leave
  enough room for i_extra_isize itself.

Therefore, update ext4_iget() to consider i_extra_isize not divisible by
4 to be an error, like the case where i_extra_isize is too large.

This also matches the rule recently added to e2fsck for determining
whether an inode has valid i_extra_isize.

This patch shouldn't have any noticeable effect on
non-corrupted/non-malicious filesystems, since the size of ext4_inode
has always been a multiple of 4.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/ext4/inode.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 861f848..bc99ebe 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4564,10 +4564,12 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
 	if (EXT4_INODE_SIZE(inode->i_sb) > EXT4_GOOD_OLD_INODE_SIZE) {
 		ei->i_extra_isize = le16_to_cpu(raw_inode->i_extra_isize);
 		if (EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize >
-		    EXT4_INODE_SIZE(inode->i_sb)) {
-			EXT4_ERROR_INODE(inode, "bad extra_isize (%u != %u)",
-				EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize,
-				EXT4_INODE_SIZE(inode->i_sb));
+			EXT4_INODE_SIZE(inode->i_sb) ||
+		    (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));
 			ret = -EFSCORRUPTED;
 			goto bad_inode;
 		}
@@ -4685,6 +4687,7 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
 	if (EXT4_INODE_SIZE(inode->i_sb) > EXT4_GOOD_OLD_INODE_SIZE) {
 		if (ei->i_extra_isize == 0) {
 			/* The extra space is currently unused. Use it. */
+			BUILD_BUG_ON(sizeof(struct ext4_inode) & 3);
 			ei->i_extra_isize = sizeof(struct ext4_inode) -
 					    EXT4_GOOD_OLD_INODE_SIZE;
 		} else {
-- 
2.8.0.rc3.226.g39d4020

[PATCH 2/3] ext4: don't read out of bounds when checking for in-inode xattrs

[Eric Biggers]

With i_extra_isize equal to or close to the available space, it was
possible for us to read past the end of the inode when trying to detect
or validate in-inode xattrs.  Fix this by checking for the needed extra
space first.

This patch shouldn't have any noticeable effect on
non-corrupted/non-malicious filesystems.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/ext4/inode.c | 4 +++-
 fs/ext4/xattr.c | 5 ++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index bc99ebe..e52f41a 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4519,7 +4519,9 @@ static inline void ext4_iget_extra_inode(struct inode *inode,
 {
 	__le32 *magic = (void *)raw_inode +
 			EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize;
-	if (*magic == cpu_to_le32(EXT4_XATTR_MAGIC)) {
+	if (EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize + sizeof(__le32)
+			<= EXT4_INODE_SIZE(inode->i_sb) &&
+	    *magic == cpu_to_le32(EXT4_XATTR_MAGIC)) {
 		ext4_set_inode_state(inode, EXT4_STATE_XATTR);
 		ext4_find_inline_data_nolock(inode);
 	} else
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 1846e91..59c9ec7 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -231,13 +231,12 @@ static int
 __xattr_check_inode(struct inode *inode, struct ext4_xattr_ibody_header *header,
 			 void *end, const char *function, unsigned int line)
 {
-	struct ext4_xattr_entry *entry = IFIRST(header);
 	int error = -EFSCORRUPTED;
 
-	if (((void *) header >= end) ||
+	if (end - (void *)header < sizeof(*header) + sizeof(u32) ||
 	    (header->h_magic != cpu_to_le32(EXT4_XATTR_MAGIC)))
 		goto errout;
-	error = ext4_xattr_check_names(entry, end, entry);
+	error = ext4_xattr_check_names(IFIRST(header), end, IFIRST(header));
 errout:
 	if (error)
 		__ext4_error_inode(inode, function, line, 0,
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 2/3] ext4: don't read out of bounds when checking for in-inode xattrs

[Andreas Dilger]

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

On Nov 26, 2016, at 11:39 PM, Eric Biggers <ebiggers@google.com> wrote:
> 
> With i_extra_isize equal to or close to the available space, it was
> possible for us to read past the end of the inode when trying to detect
> or validate in-inode xattrs.  Fix this by checking for the needed extra
> space first.
> 
> This patch shouldn't have any noticeable effect on
> non-corrupted/non-malicious filesystems.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Except for minor style issues (below), looks fine.

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

> ---
> fs/ext4/inode.c | 4 +++-
> fs/ext4/xattr.c | 5 ++---
> 2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index bc99ebe..e52f41a 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4519,7 +4519,9 @@ static inline void ext4_iget_extra_inode(struct inode *inode,
> {
> 	__le32 *magic = (void *)raw_inode +
> 			EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize;
> -	if (*magic == cpu_to_le32(EXT4_XATTR_MAGIC)) {
> +	if (EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize + sizeof(__le32)
> +			<= EXT4_INODE_SIZE(inode->i_sb) &&

(style) operators should go at the end of the previous continued line
(style) continued line should align after first '(' on previous line

> +	    *magic == cpu_to_le32(EXT4_XATTR_MAGIC)) {
> 		ext4_set_inode_state(inode, EXT4_STATE_XATTR);
> 		ext4_find_inline_data_nolock(inode);
> 	} else
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 1846e91..59c9ec7 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -231,13 +231,12 @@ static int
> __xattr_check_inode(struct inode *inode, struct ext4_xattr_ibody_header *header,
> 			 void *end, const char *function, unsigned int line)
> {
> -	struct ext4_xattr_entry *entry = IFIRST(header);
> 	int error = -EFSCORRUPTED;
> 
> -	if (((void *) header >= end) ||
> +	if (end - (void *)header < sizeof(*header) + sizeof(u32) ||
> 	    (header->h_magic != cpu_to_le32(EXT4_XATTR_MAGIC)))
> 		goto errout;
> -	error = ext4_xattr_check_names(entry, end, entry);
> +	error = ext4_xattr_check_names(IFIRST(header), end, IFIRST(header));
> errout:
> 	if (error)
> 		__ext4_error_inode(inode, function, line, 0,
> --
> 2.8.0.rc3.226.g39d4020
> 


Cheers, Andreas






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

Re: [PATCH 2/3] ext4: don't read out of bounds when checking for in-inode xattrs

[Theodore Ts'o]

On Sat, Nov 26, 2016 at 10:39:45PM -0800, Eric Biggers wrote:
> With i_extra_isize equal to or close to the available space, it was
> possible for us to read past the end of the inode when trying to detect
> or validate in-inode xattrs.  Fix this by checking for the needed extra
> space first.
> 
> This patch shouldn't have any noticeable effect on
> non-corrupted/non-malicious filesystems.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Thanks, applied with the style nits that Andreas pointed out.

		     	       	    	 - Ted

[PATCH 3/3] ext4: correctly detect when an xattr value has an invalid size

[Eric Biggers]

It was possible for an xattr value to have a very large size, which
would then pass validation on 32-bit architectures due to a pointer
wraparound.  Fix this by validating the size in a way which avoids
pointer wraparound.

It was also possible that a value's size would fit in the available
space but its padded size would not.  This would cause an out-of-bounds
memory write in ext4_xattr_set_entry when replacing the xattr value.
For example, if an xattr value of unpadded size 253 bytes went until the
very end of the inode or block, then using setxattr(2) to replace this
xattr's value with 256 bytes would cause a write to the 3 bytes past the
end of the inode or buffer, and the new xattr value would be incorrectly
truncated.  Fix this by requiring that the padded size fit in the
available space rather than the unpadded size.

This patch shouldn't have any noticeable effect on
non-corrupted/non-malicious filesystems.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/ext4/xattr.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 59c9ec7..5a94fa52 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -185,6 +185,7 @@ ext4_xattr_check_names(struct ext4_xattr_entry *entry, void *end,
 {
 	struct ext4_xattr_entry *e = entry;
 
+	/* Find the end of the names list */
 	while (!IS_LAST_ENTRY(e)) {
 		struct ext4_xattr_entry *next = EXT4_XATTR_NEXT(e);
 		if ((void *)next >= end)
@@ -192,15 +193,29 @@ ext4_xattr_check_names(struct ext4_xattr_entry *entry, void *end,
 		e = next;
 	}
 
+	/* Check the values */
 	while (!IS_LAST_ENTRY(entry)) {
 		if (entry->e_value_block != 0)
 			return -EFSCORRUPTED;
-		if (entry->e_value_size != 0 &&
-		    (value_start + le16_to_cpu(entry->e_value_offs) <
-		     (void *)e + sizeof(__u32) ||
-		     value_start + le16_to_cpu(entry->e_value_offs) +
-		    le32_to_cpu(entry->e_value_size) > end))
-			return -EFSCORRUPTED;
+		if (entry->e_value_size != 0) {
+			u16 offs = le16_to_cpu(entry->e_value_offs);
+			u32 size = le32_to_cpu(entry->e_value_size);
+			void *value;
+
+			/*
+			 * The value cannot overlap the names, and the value
+			 * with padding cannot extend beyond 'end'.  Check both
+			 * the padded and unpadded sizes, since the size may
+			 * overflow to 0 when adding padding.
+			 */
+			if (offs > end - value_start)
+				return -EFSCORRUPTED;
+			value = value_start + offs;
+			if (value < (void *)e + sizeof(u32) ||
+			    size > end - value ||
+			    EXT4_XATTR_SIZE(size) > end - value)
+				return -EFSCORRUPTED;
+		}
 		entry = EXT4_XATTR_NEXT(entry);
 	}
 
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 3/3] ext4: correctly detect when an xattr value has an invalid size

[Andreas Dilger]

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

On Nov 26, 2016, at 11:39 PM, Eric Biggers <ebiggers@google.com> wrote:
> 
> It was possible for an xattr value to have a very large size, which
> would then pass validation on 32-bit architectures due to a pointer
> wraparound.  Fix this by validating the size in a way which avoids
> pointer wraparound.

It isn't actually possible for a valid xattr value to be very large.
At most 65536 bytes even with large blocks, so it might be easier to
directly check that e_value_size is not too large rather than trying
to deal with values of 0xfffffffe bytes or similar?

Cheers, Andreas

> It was also possible that a value's size would fit in the available
> space but its padded size would not.  This would cause an out-of-bounds
> memory write in ext4_xattr_set_entry when replacing the xattr value.
> For example, if an xattr value of unpadded size 253 bytes went until the
> very end of the inode or block, then using setxattr(2) to replace this
> xattr's value with 256 bytes would cause a write to the 3 bytes past the
> end of the inode or buffer, and the new xattr value would be incorrectly
> truncated.  Fix this by requiring that the padded size fit in the
> available space rather than the unpadded size.
> 
> This patch shouldn't have any noticeable effect on
> non-corrupted/non-malicious filesystems.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
> fs/ext4/xattr.c | 27 +++++++++++++++++++++------
> 1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 59c9ec7..5a94fa52 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -185,6 +185,7 @@ ext4_xattr_check_names(struct ext4_xattr_entry *entry, void *end,
> {
> 	struct ext4_xattr_entry *e = entry;
> 
> +	/* Find the end of the names list */
> 	while (!IS_LAST_ENTRY(e)) {
> 		struct ext4_xattr_entry *next = EXT4_XATTR_NEXT(e);
> 		if ((void *)next >= end)
> @@ -192,15 +193,29 @@ ext4_xattr_check_names(struct ext4_xattr_entry *entry, void *end,
> 		e = next;
> 	}
> 
> +	/* Check the values */
> 	while (!IS_LAST_ENTRY(entry)) {
> 		if (entry->e_value_block != 0)
> 			return -EFSCORRUPTED;
> -		if (entry->e_value_size != 0 &&
> -		    (value_start + le16_to_cpu(entry->e_value_offs) <
> -		     (void *)e + sizeof(__u32) ||
> -		     value_start + le16_to_cpu(entry->e_value_offs) +
> -		    le32_to_cpu(entry->e_value_size) > end))
> -			return -EFSCORRUPTED;
> +		if (entry->e_value_size != 0) {
> +			u16 offs = le16_to_cpu(entry->e_value_offs);
> +			u32 size = le32_to_cpu(entry->e_value_size);
> +			void *value;
> +
> +			/*
> +			 * The value cannot overlap the names, and the value
> +			 * with padding cannot extend beyond 'end'.  Check both
> +			 * the padded and unpadded sizes, since the size may
> +			 * overflow to 0 when adding padding.
> +			 */
> +			if (offs > end - value_start)
> +				return -EFSCORRUPTED;
> +			value = value_start + offs;
> +			if (value < (void *)e + sizeof(u32) ||
> +			    size > end - value ||
> +			    EXT4_XATTR_SIZE(size) > end - value)
> +				return -EFSCORRUPTED;
> +		}
> 		entry = EXT4_XATTR_NEXT(entry);
> 	}
> 
> --
> 2.8.0.rc3.226.g39d4020
> 


Cheers, Andreas






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

Re: [PATCH 3/3] ext4: correctly detect when an xattr value has an invalid size

[Eric Biggers]

On Mon, Nov 28, 2016 at 12:50:02PM -0700, Andreas Dilger wrote:
> On Nov 26, 2016, at 11:39 PM, Eric Biggers <ebiggers@google.com> wrote:
> > 
> > It was possible for an xattr value to have a very large size, which
> > would then pass validation on 32-bit architectures due to a pointer
> > wraparound.  Fix this by validating the size in a way which avoids
> > pointer wraparound.
> 
> It isn't actually possible for a valid xattr value to be very large.
> At most 65536 bytes even with large blocks, so it might be easier to
> directly check that e_value_size is not too large rather than trying
> to deal with values of 0xfffffffe bytes or similar?
> 

I suppose we could do something like

	EXT4_XATTR_SIZE(size) > end - value || size > EXT4_MAX_BLOCK_SIZE

instead of

	size > end - value || EXT4_XATTR_SIZE(size) > end - value

But I don't think it's really any better.

Eric

Re: [PATCH 3/3] ext4: correctly detect when an xattr value has an invalid size

[Theodore Ts'o]

On Sat, Nov 26, 2016 at 10:39:46PM -0800, Eric Biggers wrote:
> It was possible for an xattr value to have a very large size, which
> would then pass validation on 32-bit architectures due to a pointer
> wraparound.  Fix this by validating the size in a way which avoids
> pointer wraparound.
> 
> It was also possible that a value's size would fit in the available
> space but its padded size would not.  This would cause an out-of-bounds
> memory write in ext4_xattr_set_entry when replacing the xattr value.
> For example, if an xattr value of unpadded size 253 bytes went until the
> very end of the inode or block, then using setxattr(2) to replace this
> xattr's value with 256 bytes would cause a write to the 3 bytes past the
> end of the inode or buffer, and the new xattr value would be incorrectly
> truncated.  Fix this by requiring that the padded size fit in the
> available space rather than the unpadded size.
> 
> This patch shouldn't have any noticeable effect on
> non-corrupted/non-malicious filesystems.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Thanks, applied.

					- Ted

Re: [PATCH 1/3] ext4: forbid i_extra_isize not divisible by 4

[Andreas Dilger]

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

On Nov 26, 2016, at 11:39 PM, Eric Biggers <ebiggers@google.com> wrote:
> 
> i_extra_isize not divisible by 4 is problematic for several reasons:
> 
> - It causes the in-inode xattr space to be misaligned, but the xattr
>  header and entries are not declared __packed to express this
>  possibility.  This may cause poor performance or incorrect code
>  generation on some platforms.
> - When validating the xattr entries we can read past the end of the
>  inode if the size available for xattrs is not a multiple of 4.
> - It allows the nonsensical i_extra_isize=1, which doesn't even leave
>  enough room for i_extra_isize itself.
> 
> Therefore, update ext4_iget() to consider i_extra_isize not divisible by
> 4 to be an error, like the case where i_extra_isize is too large.
> 
> This also matches the rule recently added to e2fsck for determining
> whether an inode has valid i_extra_isize.
> 
> This patch shouldn't have any noticeable effect on
> non-corrupted/non-malicious filesystems, since the size of ext4_inode
> has always been a multiple of 4.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Makes sense.

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

> ---
> fs/ext4/inode.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 861f848..bc99ebe 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4564,10 +4564,12 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
> 	if (EXT4_INODE_SIZE(inode->i_sb) > EXT4_GOOD_OLD_INODE_SIZE) {
> 		ei->i_extra_isize = le16_to_cpu(raw_inode->i_extra_isize);
> 		if (EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize >
> -		    EXT4_INODE_SIZE(inode->i_sb)) {
> -			EXT4_ERROR_INODE(inode, "bad extra_isize (%u != %u)",
> -				EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize,
> -				EXT4_INODE_SIZE(inode->i_sb));
> +			EXT4_INODE_SIZE(inode->i_sb) ||
> +		    (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));
> 			ret = -EFSCORRUPTED;
> 			goto bad_inode;
> 		}
> @@ -4685,6 +4687,7 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
> 	if (EXT4_INODE_SIZE(inode->i_sb) > EXT4_GOOD_OLD_INODE_SIZE) {
> 		if (ei->i_extra_isize == 0) {
> 			/* The extra space is currently unused. Use it. */
> +			BUILD_BUG_ON(sizeof(struct ext4_inode) & 3);
> 			ei->i_extra_isize = sizeof(struct ext4_inode) -
> 					    EXT4_GOOD_OLD_INODE_SIZE;
> 		} else {
> --
> 2.8.0.rc3.226.g39d4020
> 


Cheers, Andreas






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

Re: [PATCH 1/3] ext4: forbid i_extra_isize not divisible by 4

[Theodore Ts'o]

On Sat, Nov 26, 2016 at 10:39:44PM -0800, Eric Biggers wrote:
> i_extra_isize not divisible by 4 is problematic for several reasons:
> 
> - It causes the in-inode xattr space to be misaligned, but the xattr
>   header and entries are not declared __packed to express this
>   possibility.  This may cause poor performance or incorrect code
>   generation on some platforms.
> - When validating the xattr entries we can read past the end of the
>   inode if the size available for xattrs is not a multiple of 4.
> - It allows the nonsensical i_extra_isize=1, which doesn't even leave
>   enough room for i_extra_isize itself.
> 
> Therefore, update ext4_iget() to consider i_extra_isize not divisible by
> 4 to be an error, like the case where i_extra_isize is too large.
> 
> This also matches the rule recently added to e2fsck for determining
> whether an inode has valid i_extra_isize.
> 
> This patch shouldn't have any noticeable effect on
> non-corrupted/non-malicious filesystems, since the size of ext4_inode
> has always been a multiple of 4.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Thanks, applied.

					- Ted