On Tue, Jun 07, 2022 at 12:24:41AM -0400, Theodore Ts'o wrote:
> When checking an extended attrbiute block for correctness, we check if
> the starting offset plus the value size exceeds the end of the block.
> However, we weren't checking if the size was too large, and if it is
> so large that it triggers a wraparound when we added the starting
> offset, we won't notice the problem. Add the missing check.
Looks good.
Reviewed-by: Lukas Czerner <lczerner@xxxxxxxxxx>
>
> Reported-by: Nils Bars <nils.bars@xxxxxx>
> Reported-by: Moritz Schlögel <moritz.schloegel@xxxxxx>
> Reported-by: Nico Schiller <nico.schiller@xxxxxx>
> Signed-off-by: Theodore Ts'o <tytso@xxxxxxx>
> ---
> e2fsck/pass1.c | 5 +++--
> lib/ext2fs/ext2_ext_attr.h | 11 +++++++++++
> 2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index 2a17bb8a..11d7ce93 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> @@ -2556,8 +2556,9 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
> break;
> }
> if (entry->e_value_inum == 0) {
> - if (entry->e_value_offs + entry->e_value_size >
> - fs->blocksize) {
> + if (entry->e_value_size > EXT2_XATTR_SIZE_MAX ||
> + (entry->e_value_offs + entry->e_value_size >
> + fs->blocksize)) {
> if (fix_problem(ctx, PR_1_EA_BAD_VALUE, pctx))
> goto clear_extattr;
> break;
> diff --git a/lib/ext2fs/ext2_ext_attr.h b/lib/ext2fs/ext2_ext_attr.h
> index f2042ed5..c6068c48 100644
> --- a/lib/ext2fs/ext2_ext_attr.h
> +++ b/lib/ext2fs/ext2_ext_attr.h
> @@ -57,6 +57,17 @@ struct ext2_ext_attr_entry {
> #define EXT2_XATTR_SIZE(size) \
> (((size) + EXT2_EXT_ATTR_ROUND) & ~EXT2_EXT_ATTR_ROUND)
>
> +/*
> + * XATTR_SIZE_MAX is currently 64k, but for the purposes of checking
> + * for file system consistency errors, we use a somewhat bigger value.
> + * This allows XATTR_SIZE_MAX to grow in the future, but by using this
> + * instead of INT_MAX for certain consistency checks, we don't need to
> + * worry about arithmetic overflows. (Actually XATTR_SIZE_MAX is
> + * defined in include/uapi/linux/limits.h, so changing it is going
> + * not going to be trivial....)
> + */
> +#define EXT2_XATTR_SIZE_MAX (1 << 24)
> +
> #ifdef __KERNEL__
> # ifdef CONFIG_EXT2_FS_EXT_ATTR
> extern int ext2_get_ext_attr(struct inode *, const char *, char *, size_t, int);
> --
> 2.31.0
>
