Re: [PATCH] ext4: avoid infinite loops caused by data conflicts

[Jan Kara]

On Sun 01-03-26 18:12:47, Edward Adam Davis wrote:
> In the execution paths of mkdir and openat, there are two different
> structures, struct ext4_xattr_header and struct ext4_dir_entry_2,
> that both reference the same buffer head.
> 
> In the mkdir path, ext4_add_entry() first sets the rec_len member of
> the struct ext4_dir_entry_2 to 2048, and then sets the file_type value
> to 2 in add_dirent_to_buf()->ext4_insert_dentry()->ext4_set_de_type().

If I understand it right, the filesystem is corrupted so that directory
block of some directory is also pointed to as xattr block of some inode.
The right question to investigate here is why the block passed validation
as both xattr block and directory block - that needs explanation. Likely
metadata checksums were disabled and with some effort we could then create
a block that will be both valid directory block and valid xattr block. If
that is indeed the case, this is game over and we can't fix this in ext4
(the kernel just doesn't have enough resources to validate against such
cases) - enable metadata checksums if you need to protect against such
corruptions.

Your attempt at a "fix" changes the on disk filesystem format. I don't
think you've put too much thought into that, did you?

								Honza

> This causes the h_refcount value in the other struct ext4_xattr_header,
> which references the same buffer head, to be too large in the openat
> path.
> 
> The above causes ext4_xattr_block_set() to enter an infinite loop about
> "inserted" and cannot release the inode lock, ultimately leading to the
> 143s blocking problem mentioned in [1].
> 
> When accessing the ext4_xattr_header structure in xattr, the accessed
> buffer head data is placed after ext4_dir_entry_2 to prevent data
> collisions caused by data overlap.
> 
> [1]
> INFO: task syz.0.17:5995 blocked for more than 143 seconds.
> Call Trace:
>  inode_lock_nested include/linux/fs.h:1073 [inline]
>  __start_dirop fs/namei.c:2923 [inline]
>  start_dirop fs/namei.c:2934 [inline]
> 
> Reported-by: syzbot+512459401510e2a9a39f@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=1659aaaaa8d9d11265d7
> Tested-by: syzbot+1659aaaaa8d9d11265d7@xxxxxxxxxxxxxxxxxxxxxxxxx
> Reported-by: syzbot+1659aaaaa8d9d11265d7@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=512459401510e2a9a39f
> Tested-by: syzbot+1659aaaaa8d9d11265d7@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Edward Adam Davis <eadavis@xxxxxx>
> ---
>  fs/ext4/ext4.h  | 2 ++
>  fs/ext4/xattr.c | 2 +-
>  fs/ext4/xattr.h | 3 ++-
>  3 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 293f698b7042..4b72da4d646f 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2425,6 +2425,8 @@ struct ext4_dir_entry_2 {
>  	char	name[EXT4_NAME_LEN];	/* File name */
>  };
>  
> +#define DIFF_AREA_DE_XH sizeof(struct ext4_dir_entry_2)
> +
>  /*
>   * Access the hashes at the end of ext4_dir_entry_2
>   */
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 7bf9ba19a89d..313c460a93c5 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -2160,7 +2160,7 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
>  				error = -EIO;
>  				goto getblk_failed;
>  			}
> -			memcpy(new_bh->b_data, s->base, new_bh->b_size);
> +			memcpy(new_bh->b_data + DIFF_AREA_DE_XH, s->base, new_bh->b_size);
>  			ext4_xattr_block_csum_set(inode, new_bh);
>  			set_buffer_uptodate(new_bh);
>  			unlock_buffer(new_bh);
> diff --git a/fs/ext4/xattr.h b/fs/ext4/xattr.h
> index 1fedf44d4fb6..4a28023c72e8 100644
> --- a/fs/ext4/xattr.h
> +++ b/fs/ext4/xattr.h
> @@ -8,6 +8,7 @@
>  */
>  
>  #include <linux/xattr.h>
> +#include "ext4.h"
>  
>  /* Magic value in attribute blocks */
>  #define EXT4_XATTR_MAGIC		0xEA020000
> @@ -90,7 +91,7 @@ struct ext4_xattr_entry {
>  #define EXT4_XATTR_MIN_LARGE_EA_SIZE(b)					\
>  	((b) - EXT4_XATTR_LEN(3) - sizeof(struct ext4_xattr_header) - 4)
>  
> -#define BHDR(bh) ((struct ext4_xattr_header *)((bh)->b_data))
> +#define BHDR(bh) ((struct ext4_xattr_header *)((bh)->b_data + DIFF_AREA_DE_XH))
>  #define ENTRY(ptr) ((struct ext4_xattr_entry *)(ptr))
>  #define BFIRST(bh) ENTRY(BHDR(bh)+1)
>  #define IS_LAST_ENTRY(entry) (*(__u32 *)(entry) == 0)
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR