From: Ye Bin <[email protected]>
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 <[email protected]>
---
fs/ext4/xattr.c | 41 ++++++++++++++++++++++++++++++++++++-----
1 file changed, 36 insertions(+), 5 deletions(-)
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 718ef3987f94..eed001eee3ec 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -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,
--
2.31.1
On Wed, Dec 07, 2022 at 03:40:39PM +0800, Ye Bin wrote:
> From: Ye Bin <[email protected]>
>
> 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 ..."
--
An old man doll... just what I always wanted! - Clara
On Wed 07-12-22 15:40:39, Ye Bin wrote:
> From: Ye Bin <[email protected]>
>
> 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 <[email protected]>
...
> +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 <[email protected]>
SUSE Labs, CR
On 2022/12/7 19:14, Jan Kara wrote:
> On Wed 07-12-22 15:40:39, Ye Bin wrote:
>> From: Ye Bin <[email protected]>
>>
>> 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 <[email protected]>
> ...
>
>> +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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > ...
> >
> > > +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
--
Jan Kara <[email protected]>
SUSE Labs, CR