diff mbox series

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

Message ID 20230718040523.1171058-1-chao@kernel.org (mailing list archive)
State New
Headers show
Series [f2fs-dev] f2fs-tools: do sanity check on xattr entry | expand

Commit Message

Chao Yu July 18, 2023, 4:05 a.m. UTC 
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(-)

Comments

Jaegeuk Kim July 21, 2023, 7:21 p.m. UTC | #1 
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
diff mbox series

Patch

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;
+		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))