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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Nov 26, 2016, at 11:39 PM, Eric Biggers <[email protected]> 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 <[email protected]>
Makes sense.
Reviewed-by: Andreas Dilger <[email protected]>
> ---
> 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
On Nov 26, 2016, at 11:39 PM, Eric Biggers <[email protected]> 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 <[email protected]>
Except for minor style issues (below), looks fine.
Reviewed-by: Andreas Dilger <[email protected]>
> ---
> 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
On Nov 26, 2016, at 11:39 PM, Eric Biggers <[email protected]> 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 <[email protected]>
> ---
> 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
On Mon, Nov 28, 2016 at 12:50:02PM -0700, Andreas Dilger wrote:
> On Nov 26, 2016, at 11:39 PM, Eric Biggers <[email protected]> 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
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 <[email protected]>
Thanks, applied.
- Ted
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 <[email protected]>
Thanks, applied with the style nits that Andreas pointed out.
- Ted
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 <[email protected]>
Thanks, applied.
- Ted