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]
Date:   Wed, 7 Dec 2022 12:14:37 +0100
From:   Jan Kara <jack@...e.cz>
To:     Ye Bin <yebin@...weicloud.com>
Cc:     tytso@....edu, adilger.kernel@...ger.ca,
        linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org,
        jack@...e.cz, Ye Bin <yebin10@...wei.com>
Subject: Re: [PATCH v2 2/6] ext4: add primary check extended attribute inode
 in ext4_xattr_check_entries()

On Wed 07-12-22 15:40:39, Ye Bin wrote:
> From: Ye Bin <yebin10@...wei.com>
> 
> Add primary check for extended attribute inode, only do hash check when read
> ea_inode's data in ext4_xattr_inode_get().
> 
> Signed-off-by: Ye Bin <yebin10@...wei.com>

...

> +static inline int ext4_xattr_check_extra_inode(struct inode *inode,
> +					       struct ext4_xattr_entry *entry)
> +{
> +	int err;
> +	struct inode *ea_inode;
> +
> +	err = ext4_xattr_inode_iget(inode, le32_to_cpu(entry->e_value_inum),
> +				    le32_to_cpu(entry->e_hash), &ea_inode);
> +	if (err)
> +		return err;
> +
> +	if (i_size_read(ea_inode) != le32_to_cpu(entry->e_value_size)) {
> +		ext4_warning_inode(ea_inode,
> +                           "ea_inode file size=%llu entry size=%u",
> +                           i_size_read(ea_inode),
> +			   le32_to_cpu(entry->e_value_size));
> +		err = -EFSCORRUPTED;
> +	}
> +	iput(ea_inode);
> +
> +	return err;
> +}
> +
>  static int
> -ext4_xattr_check_entries(struct ext4_xattr_entry *entry, void *end,
> -			 void *value_start)
> +ext4_xattr_check_entries(struct inode *inode, struct ext4_xattr_entry *entry,
> +			 void *end, void *value_start)
>  {
>  	struct ext4_xattr_entry *e = entry;
>  
> @@ -221,6 +247,10 @@ ext4_xattr_check_entries(struct ext4_xattr_entry *entry, void *end,
>  			    size > end - value ||
>  			    EXT4_XATTR_SIZE(size) > end - value)
>  				return -EFSCORRUPTED;
> +		} else if (entry->e_value_inum) {
> +			int err = ext4_xattr_check_extra_inode(inode, entry);
> +			if (err)
> +				return err;
>  		}
>  		entry = EXT4_XATTR_NEXT(entry);
>  	}

So I was thinking about this. It is nice to have the inode references
checked but OTOH this is rather expensive for a filesystem with EA inodes -
we have to lookup and possibly load EA inodes from the disk although they
won't be needed for anything else than the check. Also as you have noticed
we do check whether i_size and xattr size as recorded in xattr entry match
in ext4_xattr_inode_iget() which gets called once we need to do anything
with the EA inode.

Also I've checked and we do call ext4_xattr_check_block() and
xattr_check_inode() in ext4_expand_extra_isize_ea() so Ted's suspicion that
the problem comes from not checking the xattr entries before moving them
from the inode was not correct.

So to summarize, I don't think this and the following patch is actually
needed and brings benefit that would outweight the performance cost.

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists