Re: [f2fs-dev] [PATCH] f2fs-tools: do sanity check on xattr entry

[Jaegeuk Kim <jaegeuk@kernel.org>]

On 07/18, Chao Yu wrote:
> If there are corrupted xattr entries in xattr space, it may cause
> traversing across end of xattr space issue, this patch adds sanity
> check during xattr traverse to avoid such issue.
> 
> This patch synchronizes kernel commits:
> 
> 2777e654371d ("f2fs: fix to avoid accessing xattr across the boundary")
> 688078e7f36c ("f2fs: fix to avoid memory leakage in f2fs_listxattr")
> 
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
>  fsck/dump.c  |  9 +++++++++
>  fsck/mount.c | 18 ++++++++++++++----
>  fsck/xattr.c | 21 ++++++++++++++++++---
>  fsck/xattr.h |  6 ++++++
>  4 files changed, 47 insertions(+), 7 deletions(-)
> 
> diff --git a/fsck/dump.c b/fsck/dump.c
> index 6deaa7e..e7a1c5c 100644
> --- a/fsck/dump.c
> +++ b/fsck/dump.c
> @@ -354,6 +354,7 @@ static void dump_node_blk(struct f2fs_sb_info *sbi, int ntype,
>  static void dump_xattr(struct f2fs_sb_info *sbi, struct f2fs_node *node_blk)
>  {
>  	void *xattr;
> +	void *last_base_addr;
>  	struct f2fs_xattr_entry *ent;
>  	char xattr_name[F2FS_NAME_LEN] = {0};
>  	int ret;
> @@ -362,10 +363,18 @@ static void dump_xattr(struct f2fs_sb_info *sbi, struct f2fs_node *node_blk)
>  	if (!xattr)
>  		return;
>  
> +	last_base_addr = (void *)xattr + XATTR_SIZE(&node_blk->i);
> +
>  	list_for_each_xattr(ent, xattr) {
>  		char *name = strndup(ent->e_name, ent->e_name_len);
>  		void *value = ent->e_name + ent->e_name_len;
>  
> +		if ((void *)(ent) + sizeof(__u32) > last_base_addr ||
> +			(void *)XATTR_NEXT_ENTRY(ent) > last_base_addr) {
> +			MSG(0, "xattr entry crosses the end of xattr space\n");
> +			break;
> +		}
> +
>  		if (!name)
>  			continue;
>  
> diff --git a/fsck/mount.c b/fsck/mount.c
> index 2691b2f..3ee003d 100644
> --- a/fsck/mount.c
> +++ b/fsck/mount.c
> @@ -274,6 +274,7 @@ void print_inode_info(struct f2fs_sb_info *sbi,
>  {
>  	struct f2fs_inode *inode = &node->i;
>  	void *xattr_addr;
> +	void *last_base_addr;
>  	struct f2fs_xattr_entry *ent;
>  	char en[F2FS_PRINT_NAMELEN];
>  	unsigned int i = 0;
> @@ -370,13 +371,22 @@ void print_inode_info(struct f2fs_sb_info *sbi,
>  	DISP_u32(inode, i_nid[4]);	/* double indirect */
>  
>  	xattr_addr = read_all_xattrs(sbi, node);
> -	if (xattr_addr) {
> -		list_for_each_xattr(ent, xattr_addr) {
> -			print_xattr_entry(ent);
> +	if (!xattr_addr)
> +		goto out;
> +
> +	last_base_addr = (void *)xattr_addr + XATTR_SIZE(&node->i);
> +
> +	list_for_each_xattr(ent, xattr_addr) {
> +		if ((void *)(ent) + sizeof(__u32) > last_base_addr ||
> +			(void *)XATTR_NEXT_ENTRY(ent) > last_base_addr) {
> +			MSG(0, "xattr entry crosses the end of xattr space\n");
> +			break;
>  		}
> -		free(xattr_addr);
> +		print_xattr_entry(ent);
>  	}
> +	free(xattr_addr);
>  
> +out:
>  	printf("\n");
>  }
>  
> diff --git a/fsck/xattr.c b/fsck/xattr.c
> index 04c2879..4593709 100644
> --- a/fsck/xattr.c
> +++ b/fsck/xattr.c
> @@ -55,11 +55,19 @@ void *read_all_xattrs(struct f2fs_sb_info *sbi, struct f2fs_node *inode)
>  	return txattr_addr;
>  }
>  
> -static struct f2fs_xattr_entry *__find_xattr(void *base_addr, int index,
> -		size_t len, const char *name)
> +static struct f2fs_xattr_entry *__find_xattr(void *base_addr,
> +				void *last_base_addr, int index,
> +				size_t len, const char *name)
>  {
>  	struct f2fs_xattr_entry *entry;
> +
>  	list_for_each_xattr(entry, base_addr) {
> +		if ((void *)(entry) + sizeof(__u32) > last_base_addr ||
> +			(void *)XATTR_NEXT_ENTRY(entry) > last_base_addr) {
> +			MSG(0, "xattr entry crosses the end of xattr space\n");
> +			return NULL;
> +		}
> +
>  		if (entry->e_name_index != index)
>  			continue;
>  		if (entry->e_name_len != len)
> @@ -125,6 +133,7 @@ int f2fs_setxattr(struct f2fs_sb_info *sbi, nid_t ino, int index, const char *na
>  {
>  	struct f2fs_node *inode;
>  	void *base_addr;
> +	void *last_base_addr;
>  	struct f2fs_xattr_entry *here, *last;
>  	struct node_info ni;
>  	int error = 0;
> @@ -159,7 +168,13 @@ int f2fs_setxattr(struct f2fs_sb_info *sbi, nid_t ino, int index, const char *na
>  	base_addr = read_all_xattrs(sbi, inode);
>  	ASSERT(base_addr);
>  
> -	here = __find_xattr(base_addr, index, len, name);
> +	last_base_addr = (void *)base_addr + XATTR_SIZE(&inode->i);
> +
> +	here = __find_xattr(base_addr, last_base_addr, index, len, name);
> +	if (!here) {
> +		error = -EUCLEAN;

Fixed build error by replacing this with:

-               error = -EUCLEAN;
+               MSG(0, "Need to run fsck due to corrupted xattr.\n");
+               error = -EINVAL;

> +		goto exit;
> +	}
>  
>  	found = IS_XATTR_LAST_ENTRY(here) ? 0 : 1;
>  
> diff --git a/fsck/xattr.h b/fsck/xattr.h
> index bf1dd7e..867349c 100644
> --- a/fsck/xattr.h
> +++ b/fsck/xattr.h
> @@ -182,6 +182,12 @@ static inline int f2fs_acl_count(int size)
>  			!IS_XATTR_LAST_ENTRY(entry); \
>  			entry = XATTR_NEXT_ENTRY(entry))
>  
> +#define VALID_XATTR_BLOCK_SIZE	(F2FS_BLKSIZE - sizeof(struct node_footer))
> +
> +#define XATTR_SIZE(i)		((le32_to_cpu((i)->i_xattr_nid) ?	\
> +					VALID_XATTR_BLOCK_SIZE : 0) +	\
> +						(inline_xattr_size(i)))
> +
>  #define MIN_OFFSET	XATTR_ALIGN(F2FS_BLKSIZE -		\
>  		sizeof(struct node_footer) - sizeof(__u32))
>  
> -- 
> 2.40.1


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel