linux-kernel - Re: [PATCH v2 2/6] ext4: add primary check extended attribute inode in ext4_xattr_check

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: <20221207111437.birh6zujw4wauvhu@quack3>
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