[v2,2/6] ext4: add primary check extended attribute inode in ext4_xattr_check_entries()
Commit Message
From: Ye Bin <yebin10@huawei.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@huawei.com>
---
fs/ext4/xattr.c | 41 ++++++++++++++++++++++++++++++++++++-----
1 file changed, 36 insertions(+), 5 deletions(-)
Comments
On Wed, Dec 07, 2022 at 03:40:39PM +0800, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
>
> Add primary check for extended attribute inode, only do hash check when read
> ea_inode's data in ext4_xattr_inode_get().
"..., which is only perform hash checking when reading ..."
On Wed 07-12-22 15:40:39, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.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@huawei.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
On 2022/12/7 19:14, Jan Kara wrote:
> On Wed 07-12-22 15:40:39, Ye Bin wrote:
>> From: Ye Bin <yebin10@huawei.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@huawei.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
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?
On Wed 07-12-22 19:39:54, yebin (H) wrote:
>
>
> On 2022/12/7 19:14, Jan Kara wrote:
> > On Wed 07-12-22 15:40:39, Ye Bin wrote:
> > > From: Ye Bin <yebin10@huawei.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@huawei.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
>
> 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?
My suggestion would be to take patches 1,4,5,6 from your series. So reduce
EXT4_XATTR_SIZE_MAX (if Ted agrees), use kvmalloc() instead of kmalloc(),
do the cleanup of funtion names, and fix the inode refcount leak.
Honza
@@ -83,6 +83,9 @@ static __le32 ext4_xattr_hash_entry(char *name, size_t name_len, __le32 *value,
size_t value_count);
static void ext4_xattr_rehash(struct ext4_xattr_header *);
+static int ext4_xattr_inode_iget(struct inode *parent, unsigned long ea_ino,
+ u32 ea_inode_hash, struct inode **ea_inode);
+
static const struct xattr_handler * const ext4_xattr_handler_map[] = {
[EXT4_XATTR_INDEX_USER] = &ext4_xattr_user_handler,
#ifdef CONFIG_EXT4_FS_POSIX_ACL
@@ -181,9 +184,32 @@ ext4_xattr_handler(int name_index)
return handler;
}
+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);
}
@@ -243,8 +273,8 @@ __ext4_xattr_check_block(struct inode *inode, struct buffer_head *bh,
error = -EFSBADCRC;
if (!ext4_xattr_block_csum_verify(inode, bh))
goto errout;
- error = ext4_xattr_check_entries(BFIRST(bh), bh->b_data + bh->b_size,
- bh->b_data);
+ error = ext4_xattr_check_entries(inode, BFIRST(bh),
+ bh->b_data + bh->b_size, bh->b_data);
errout:
if (error)
__ext4_error_inode(inode, function, line, 0, -error,
@@ -268,7 +298,8 @@ __xattr_check_inode(struct inode *inode, struct ext4_xattr_ibody_header *header,
if (end - (void *)header < sizeof(*header) + sizeof(u32) ||
(header->h_magic != cpu_to_le32(EXT4_XATTR_MAGIC)))
goto errout;
- error = ext4_xattr_check_entries(IFIRST(header), end, IFIRST(header));
+ error = ext4_xattr_check_entries(inode, IFIRST(header), end,
+ IFIRST(header));
errout:
if (error)
__ext4_error_inode(inode, function, line, 0, -error,