Re: [PATCH v2 2/6] ext4: add primary check extended attribute inode in ext4_xattr_check_entries()

From: yebin (H)
Date: Wed Dec 07 2022 - 06:40:02 EST




On 2022/12/7 19:14, Jan Kara wrote:
On Wed 07-12-22 15:40:39, Ye Bin wrote:
From: Ye Bin <yebin10@xxxxxxxxxx>

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@xxxxxxxxxx>
...

+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

Yes, I agree with you.
In ext4_ xattr_ check_ Entries () simply verifies the length of the extended attribute with
ea_inode. If the previous patch is not merged, EXT4_ XATTR_ SIZE_ MAX is much larger
than the actual constraint value. Data verification can only be postponed until the ea_inode
is read.
So your suggestion is to modify EXT4_ XATTR_ SIZE_ MAX Or defer data verification until
the ea_inode is read?