Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 7 Jun 2022 15:33:19 +0200
From:   Lukas Czerner <lczerner@redhat.com>
To:     Theodore Ts'o <tytso@mit.edu>
Cc:     Ext4 Developers List <linux-ext4@vger.kernel.org>,
        Nils Bars <nils.bars@rub.de>,
        Moritz =?utf-8?B?U2NobMO2Z2Vs?= <moritz.schloegel@rub.de>,
        Nico Schiller <nico.schiller@rub.de>
Subject: Re: [PATCH 4/7] e2fsck: check for xattr value size integer wraparound
Message-ID: <20220607133319.5s4b4l43lronawnr@fedora>
References: <20220607042444.1798015-1-tytso@mit.edu>
 <20220607042444.1798015-5-tytso@mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220607042444.1798015-5-tytso@mit.edu>
Precedence: bulk

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
>