e2fsck: improve in-inode xattr checks

From: Andreas Dilger <andreas.dilger@intel.com>

Add check for in-inode xattr to make sure that it is not referencing
an offset that is beyond the end of the inode.

Change-Id: I5d7c0cac9aebfdaba4e48b5144d51b764f42e1ad
Signed-off-by: Andreas Dilger <andreas.dilger@intel.com>
Signed-off-by: Artem Blagodarenko <artem.blagodarenko@gmail.com>
---
 .gitignore       |  1 +
 e2fsck/pass1.c   | 11 +++++++++--
 e2fsck/problem.c |  4 ++++
 e2fsck/problem.h |  2 ++
 4 files changed, 16 insertions(+), 2 deletions(-)

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

On Jul 2, 2018, at 3:56 PM, Theodore Y. Ts'o <tytso@mit.edu> wrote:
> 
> 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.

The original versions of these patches were created on much older
versions of e2fsprogs (1.41 or earlier), so it is entirely possible
that some of them are now outdated, which is why I haven't been
sending them upstream myself.  They definitely need to be reviewed
more closely to see if they are still relevant to newer e2fsprogs.

Cheers, Andreas