Re: [PATCH 4/7] e2fsck: check for xattr value size integer wraparound

[Lukas Czerner <lczerner@redhat.com>]

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@redhat.com>

> 
> Reported-by: Nils Bars <nils.bars@rub.de>
> Reported-by: Moritz Schlögel <moritz.schloegel@rub.de>
> Reported-by: Nico Schiller <nico.schiller@rub.de>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  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
>