linux-ext4 - Re: [PATCH] e2fsck: improve in-inode xattr checks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180702215655.GB19410@thunk.org>
Date:   Mon, 2 Jul 2018 17:56:55 -0400
From:   "Theodore Y. Ts'o" <tytso@....edu>
To:     c17828 <artem.blagodarenko@...il.com>
Cc:     linux-ext4@...r.kernel.org, adilger.kernel@...ger.ca,
        alexey.lyashkov@...il.com,
        Andreas Dilger <andreas.dilger@...el.com>
Subject: Re: [PATCH] e2fsck: improve in-inode xattr checks

On Wed, Jun 27, 2018 at 01:02:24PM +0300, c17828 wrote:
> diff --git a/.gitignore b/.gitignore
> index cceaed6d..78460691 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -123,6 +123,7 @@ lib/ext2fs/tst_iscan
>  lib/ext2fs/tst_libext2fs
>  lib/ext2fs/tst_sha256
>  lib/ext2fs/tst_sha512
> +lib/ext2fs/tst_read_ea

This gitignore change has nothing to do with the rest of the commit.

> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index 0fedb9a4..58fcdbec 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> @@ -500,8 +500,15 @@ static void check_ea_in_inode(e2fsck_t ctx, struct problem_context *pctx,
>  				goto fix;
>  			}
>  
> -			hash = ext2fs_ext_attr_hash_entry(entry,
> -							  start + entry->e_value_offs);
> +		/* Value size cannot be larger than EA space in inode */
> +		if (entry->e_value_offs > storage_size ||
> +		    entry->e_value_offs + entry->e_value_size > storage_size) {
> +			problem = PR_1_INODE_EA_BAD_VALUE;
> +			goto fix;
> +		}
> +
> +		hash = ext2fs_ext_attr_hash_entry(entry,
> +						  start + entry->e_value_offs);
>  
>  			/* e_hash may be 0 in older inode's ea */
>  			if (entry->e_hash != 0 && entry->e_hash != hash) {

This patch has bad identation, and breaks the indentation of an
existing, otherwise unchanged line.

You also didn't create a test case for this change; if you did, I
believe you would have discovered that the newly added change is
effectively dead code, since if the value exceeds the valid region
boundaries, the region_allocate() code above would have returned -1.
The resulting e2fsck description of the file system corruption
wouldn't have been misleading (since it would report it as
EA_ALLOC_COLLISION problem), so there is value in explicitly
explaining what might be wrong --- but in that case it would be better
if the displayed message gave more detail about what was wrong
(including displaying the e_value_offs and e_value_size for the xattr
in question).

So I'm going to send back this patch with a request for improvement.

Artem, it looks like you're just blasting changes from the Lustre fork
of e2fsprogs without doing any sanity checking or clean up.  Maybe
there is a somewhat lower standard of quality for the Lustre fork of
e2fsprogs, but I'd really appreciate it if you could add value by
cleaning up the patches before you submit them for upstream inclusion.

Sending lower-quality patches degrades both your open source
reputation as well as the reputation of the Lustre project.

Regards,

							- Ted