| Message ID | 20221214200818.870087-1-tytso@mit.edu |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | ext4: improve xattr consistency checking and error reporting | expand |
On Dec 14, 2022, at 1:08 PM, Theodore Ts'o <tytso@mit.edu> 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 <tytso@mit.edu> Reviewed-by: Andreas Dilger <adilger@dilger.ca> 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,
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) \
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 <tytso@mit.edu> --- fs/ext4/xattr.c | 126 ++++++++++++++++++++++++++++++------------------ 1 file changed, 80 insertions(+), 46 deletions(-)