Refactor the in-inode and xattr block consistency checking, and report
more fine-grained reports of the consistency problems. Also add more
consistency checks for ea_inode number.
Signed-off-by: Theodore Ts'o <[email protected]>
---
fs/ext4/xattr.c | 126 ++++++++++++++++++++++++++++++------------------
1 file changed, 80 insertions(+), 46 deletions(-)
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 7decaaf27e82..247560cb8016 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -182,27 +182,73 @@ ext4_xattr_handler(int name_index)
}
static int
-ext4_xattr_check_entries(struct ext4_xattr_entry *entry, void *end,
- void *value_start)
+check_xattrs(struct inode *inode, struct buffer_head *bh,
+ struct ext4_xattr_entry *entry, void *end, void *value_start,
+ const char *function, unsigned int line)
{
struct ext4_xattr_entry *e = entry;
+ int err = -EFSCORRUPTED;
+ char *err_str;
+
+ if (bh) {
+ if (BHDR(bh)->h_magic != cpu_to_le32(EXT4_XATTR_MAGIC) ||
+ BHDR(bh)->h_blocks != cpu_to_le32(1)) {
+ err_str = "invalid header";
+ goto errout;
+ }
+ if (buffer_verified(bh))
+ return 0;
+ if (!ext4_xattr_block_csum_verify(inode, bh)) {
+ err = -EFSBADCRC;
+ err_str = "invalid checksum";
+ goto errout;
+ }
+ } else {
+ struct ext4_xattr_ibody_header *header = value_start;
+
+ header -= 1;
+ if (end - (void *)header < sizeof(*header) + sizeof(u32)) {
+ err_str = "in-inode xattr block too small";
+ goto errout;
+ }
+ if (header->h_magic != cpu_to_le32(EXT4_XATTR_MAGIC)) {
+ err_str = "bad magic number in in-inode xattr";
+ goto errout;
+ }
+ }
/* 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)
- return -EFSCORRUPTED;
- if (strnlen(e->e_name, e->e_name_len) != e->e_name_len)
- return -EFSCORRUPTED;
+ if ((void *)next >= end) {
+ err_str = "e_name out of bounds";
+ goto errout;
+ }
+ if (strnlen(e->e_name, e->e_name_len) != e->e_name_len) {
+ err_str = "bad e_name length";
+ goto errout;
+ }
e = next;
}
/* Check the values */
while (!IS_LAST_ENTRY(entry)) {
u32 size = le32_to_cpu(entry->e_value_size);
+ unsigned long ea_ino = le32_to_cpu(entry->e_value_inum);
- if (size > EXT4_XATTR_SIZE_MAX)
- return -EFSCORRUPTED;
+ if (!ext4_has_feature_ea_inode(inode->i_sb) && ea_ino) {
+ err_str = "ea_inode specified without ea_inode feature enabled";
+ goto errout;
+ }
+ if (ea_ino && ((ea_ino == EXT4_ROOT_INO) ||
+ !ext4_valid_inum(inode->i_sb, ea_ino))) {
+ err_str = "invalid ea_ino";
+ goto errout;
+ }
+ if (size > EXT4_XATTR_SIZE_MAX) {
+ err_str = "e_value size too large";
+ goto errout;
+ }
if (size != 0 && entry->e_value_inum == 0) {
u16 offs = le16_to_cpu(entry->e_value_offs);
@@ -214,66 +260,54 @@ ext4_xattr_check_entries(struct ext4_xattr_entry *entry, void *end,
* the padded and unpadded sizes, since the size may
* overflow to 0 when adding padding.
*/
- if (offs > end - value_start)
- return -EFSCORRUPTED;
+ if (offs > end - value_start) {
+ err_str = "e_value out of bounds";
+ goto errout;
+ }
value = value_start + offs;
if (value < (void *)e + sizeof(u32) ||
size > end - value ||
- EXT4_XATTR_SIZE(size) > end - value)
- return -EFSCORRUPTED;
+ EXT4_XATTR_SIZE(size) > end - value) {
+ err_str = "overlapping e_value ";
+ goto errout;
+ }
}
entry = EXT4_XATTR_NEXT(entry);
}
-
+ if (bh)
+ set_buffer_verified(bh);
return 0;
+
+errout:
+ if (bh)
+ __ext4_error_inode(inode, function, line, 0, -err,
+ "corrupted xattr block %llu: %s",
+ (unsigned long long) bh->b_blocknr,
+ err_str);
+ else
+ __ext4_error_inode(inode, function, line, 0, -err,
+ "corrupted in-inode xattr: %s", err_str);
+ return err;
}
static inline int
__ext4_xattr_check_block(struct inode *inode, struct buffer_head *bh,
const char *function, unsigned int line)
{
- int error = -EFSCORRUPTED;
-
- if (BHDR(bh)->h_magic != cpu_to_le32(EXT4_XATTR_MAGIC) ||
- BHDR(bh)->h_blocks != cpu_to_le32(1))
- goto errout;
- if (buffer_verified(bh))
- return 0;
-
- error = -EFSBADCRC;
- if (!ext4_xattr_block_csum_verify(inode, bh))
- goto errout;
- error = ext4_xattr_check_entries(BFIRST(bh), bh->b_data + bh->b_size,
- bh->b_data);
-errout:
- if (error)
- __ext4_error_inode(inode, function, line, 0, -error,
- "corrupted xattr block %llu",
- (unsigned long long) bh->b_blocknr);
- else
- set_buffer_verified(bh);
- return error;
+ return check_xattrs(inode, bh, BFIRST(bh), bh->b_data + bh->b_size,
+ bh->b_data, function, line);
}
#define ext4_xattr_check_block(inode, bh) \
__ext4_xattr_check_block((inode), (bh), __func__, __LINE__)
-static int
+static inline int
__xattr_check_inode(struct inode *inode, struct ext4_xattr_ibody_header *header,
void *end, const char *function, unsigned int line)
{
- int error = -EFSCORRUPTED;
-
- if (end - (void *)header < sizeof(*header) + sizeof(u32) ||
- (header->h_magic != cpu_to_le32(EXT4_XATTR_MAGIC)))
- goto errout;
- error = ext4_xattr_check_entries(IFIRST(header), end, IFIRST(header));
-errout:
- if (error)
- __ext4_error_inode(inode, function, line, 0, -error,
- "corrupted in-inode xattr");
- return error;
+ return check_xattrs(inode, NULL, IFIRST(header), end, IFIRST(header),
+ function, line);
}
#define xattr_check_inode(inode, header, end) \
--
2.31.0
On Dec 14, 2022, at 1:08 PM, Theodore Ts'o <[email protected]> wrote:
>
> Refactor the in-inode and xattr block consistency checking, and report
> more fine-grained reports of the consistency problems. Also add more
> consistency checks for ea_inode number.
>
> Signed-off-by: Theodore Ts'o <[email protected]>
Reviewed-by: Andreas Dilger <[email protected]>
A minor nit below, but no reason not to hold up the patch.
> static int
> -ext4_xattr_check_entries(struct ext4_xattr_entry *entry, void *end,
> - void *value_start)
> +check_xattrs(struct inode *inode, struct buffer_head *bh,
> + struct ext4_xattr_entry *entry, void *end, void *value_start,
> + const char *function, unsigned int line)
> {
> struct ext4_xattr_entry *e = entry;
"e" isn't a great variable name (though it predates the patch), maybe "last"
or "last_entry" is better since that is what it is used for later?
> + int err = -EFSCORRUPTED;
> + char *err_str;
> +
> + if (bh) {
> + if (BHDR(bh)->h_magic != cpu_to_le32(EXT4_XATTR_MAGIC) ||
> + BHDR(bh)->h_blocks != cpu_to_le32(1)) {
> + err_str = "invalid header";
> + goto errout;
> + }
> + if (buffer_verified(bh))
> + return 0;
> + if (!ext4_xattr_block_csum_verify(inode, bh)) {
> + err = -EFSBADCRC;
> + err_str = "invalid checksum";
> + goto errout;
> + }
> + } else {
> + struct ext4_xattr_ibody_header *header = value_start;
> +
> + header -= 1;
> + if (end - (void *)header < sizeof(*header) + sizeof(u32)) {
> + err_str = "in-inode xattr block too small";
> + goto errout;
> + }
> + if (header->h_magic != cpu_to_le32(EXT4_XATTR_MAGIC)) {
> + err_str = "bad magic number in in-inode xattr";
> + goto errout;
> + }
> + }
>
> /* 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)
> - return -EFSCORRUPTED;
> - if (strnlen(e->e_name, e->e_name_len) != e->e_name_len)
> - return -EFSCORRUPTED;
> + if ((void *)next >= end) {
> + err_str = "e_name out of bounds";
> + goto errout;
> + }
> + if (strnlen(e->e_name, e->e_name_len) != e->e_name_len) {
> + err_str = "bad e_name length";
> + goto errout;
> + }
> e = next;
> }
>
> /* Check the values */
> while (!IS_LAST_ENTRY(entry)) {
> u32 size = le32_to_cpu(entry->e_value_size);
> + unsigned long ea_ino = le32_to_cpu(entry->e_value_inum);
>
> - if (size > EXT4_XATTR_SIZE_MAX)
> - return -EFSCORRUPTED;
> + if (!ext4_has_feature_ea_inode(inode->i_sb) && ea_ino) {
> + err_str = "ea_inode specified without ea_inode feature enabled";
> + goto errout;
> + }
> + if (ea_ino && ((ea_ino == EXT4_ROOT_INO) ||
> + !ext4_valid_inum(inode->i_sb, ea_ino))) {
> + err_str = "invalid ea_ino";
> + goto errout;
> + }
> + if (size > EXT4_XATTR_SIZE_MAX) {
> + err_str = "e_value size too large";
> + goto errout;
> + }
>
> if (size != 0 && entry->e_value_inum == 0) {
> u16 offs = le16_to_cpu(entry->e_value_offs);
> @@ -214,66 +260,54 @@ ext4_xattr_check_entries(struct ext4_xattr_entry *entry, void *end,
> * the padded and unpadded sizes, since the size may
> * overflow to 0 when adding padding.
> */
> - if (offs > end - value_start)
> - return -EFSCORRUPTED;
> + if (offs > end - value_start) {
> + err_str = "e_value out of bounds";
> + goto errout;
> + }
> value = value_start + offs;
> if (value < (void *)e + sizeof(u32) ||
> size > end - value ||
> - EXT4_XATTR_SIZE(size) > end - value)
> - return -EFSCORRUPTED;
> + EXT4_XATTR_SIZE(size) > end - value) {
> + err_str = "overlapping e_value ";
> + goto errout;
> + }
> }
> entry = EXT4_XATTR_NEXT(entry);
> }
> -
> + if (bh)
> + set_buffer_verified(bh);
> return 0;
> +
> +errout:
> + if (bh)
> + __ext4_error_inode(inode, function, line, 0, -err,
> + "corrupted xattr block %llu: %s",
> + (unsigned long long) bh->b_blocknr,
> + err_str);
> + else
> + __ext4_error_inode(inode, function, line, 0, -err,
> + "corrupted in-inode xattr: %s", err_str);
> + return err;
> }
>
> static inline int
> __ext4_xattr_check_block(struct inode *inode, struct buffer_head *bh,
> const char *function, unsigned int line)
> {
> - int error = -EFSCORRUPTED;
> -
> - if (BHDR(bh)->h_magic != cpu_to_le32(EXT4_XATTR_MAGIC) ||
> - BHDR(bh)->h_blocks != cpu_to_le32(1))
> - goto errout;
> - if (buffer_verified(bh))
> - return 0;
> -
> - error = -EFSBADCRC;
> - if (!ext4_xattr_block_csum_verify(inode, bh))
> - goto errout;
> - error = ext4_xattr_check_entries(BFIRST(bh), bh->b_data + bh->b_size,
> - bh->b_data);
> -errout:
> - if (error)
> - __ext4_error_inode(inode, function, line, 0, -error,
> - "corrupted xattr block %llu",
> - (unsigned long long) bh->b_blocknr);
> - else
> - set_buffer_verified(bh);
> - return error;
> + return check_xattrs(inode, bh, BFIRST(bh), bh->b_data + bh->b_size,
> + bh->b_data, function, line);
> }
>
> #define ext4_xattr_check_block(inode, bh) \
> __ext4_xattr_check_block((inode), (bh), __func__, __LINE__)
>
>
> -static int
> +static inline int
> __xattr_check_inode(struct inode *inode, struct ext4_xattr_ibody_header *header,
> void *end, const char *function, unsigned int line)
> {
> - int error = -EFSCORRUPTED;
> -
> - if (end - (void *)header < sizeof(*header) + sizeof(u32) ||
> - (header->h_magic != cpu_to_le32(EXT4_XATTR_MAGIC)))
> - goto errout;
> - error = ext4_xattr_check_entries(IFIRST(header), end, IFIRST(header));
> -errout:
> - if (error)
> - __ext4_error_inode(inode, function, line, 0, -error,
> - "corrupted in-inode xattr");
> - return error;
> + return check_xattrs(inode, NULL, IFIRST(header), end, IFIRST(header),
> + function, line);
> }
>
> #define xattr_check_inode(inode, header, end) \
> --
> 2.31.0
>
Cheers, Andreas
On Wed, 14 Dec 2022 15:08:18 -0500, Theodore Ts'o wrote:
> Refactor the in-inode and xattr block consistency checking, and report
> more fine-grained reports of the consistency problems. Also add more
> consistency checks for ea_inode number.
>
>
Applied, thanks!
[1/1] ext4: improve xattr consistency checking and error reporting
commit: 3478c83cf26bbffd026ae6a56bcb1fe544f0834e
Best regards,
--
Theodore Ts'o <[email protected]>