[f2fs-dev] f2fs-tools: ensure that unused xattr space is zeroized

Commit Message

Eric Biggers Oct. 26, 2023, 2:57 a.m. UTC

From: Eric Biggers <[e-mail]>

Make fsck.f2fs zeroize the unused xattr space, i.e. the space after the
end of the zero-terminated xattr list, if it isn't already zeroized.

This is important because the kernel currently does not explicitly
zero-terminate the list when writing xattrs.  So, the kernel relies on
the unused space containing zeroes.

Also, add a missing free() to fix a memory leak.

Signed-off-by: Eric Biggers <[e-mail]>
---
 fsck/fsck.c | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)


base-commit: 104b6b83206a9919d4b10f310981cc99fbbc8ed1

Comments

Chao Yu Nov. 15, 2023, 8:37 a.m. UTC | #1

On 2023/10/26 10:57, Eric Biggers wrote:
> From: Eric Biggers <[e-mail]>
> 
> Make fsck.f2fs zeroize the unused xattr space, i.e. the space after the
> end of the zero-terminated xattr list, if it isn't already zeroized.
> 
> This is important because the kernel currently does not explicitly
> zero-terminate the list when writing xattrs.  So, the kernel relies on
> the unused space containing zeroes.
> 
> Also, add a missing free() to fix a memory leak.
> 
> Signed-off-by: Eric Biggers <[e-mail]>

Reviewed-by: Chao Yu <[e-mail]>

Thanks,

Patch

diff --git a/fsck/fsck.c b/fsck/fsck.c
index 72daa71..51e46e4 100644
--- a/fsck/fsck.c
+++ b/fsck/fsck.c
@@ -827,54 +827,72 @@  void fsck_reada_all_direct_node_blocks(struct f2fs_sb_info *sbi,
 {
 	int i;
 
 	for (i = 0; i < NIDS_PER_BLOCK; i++) {
 		u32 nid = le32_to_cpu(node_blk->in.nid[i]);
 
 		fsck_reada_node_block(sbi, nid);
 	}
 }
 
+static bool is_zeroed(const u8 *p, size_t size)
+{
+	size_t i;
+
+	for (i = 0; i < size; i++) {
+		if (p[i])
+			return false;
+	}
+	return true;
+}
+
 int chk_extended_attributes(struct f2fs_sb_info *sbi, u32 nid,
 		struct f2fs_node *inode)
 {
 	void *xattr;
 	void *last_base_addr;
 	struct f2fs_xattr_entry *ent;
 	__u32 xattr_size = XATTR_SIZE(&inode->i);
+	bool need_fix = false;
 
 	if (xattr_size == 0)
 		return 0;
 
 	xattr = read_all_xattrs(sbi, inode, false);
 	ASSERT(xattr);
 
 	last_base_addr = (void *)xattr + xattr_size;
 
 	list_for_each_xattr(ent, xattr) {
 		if ((void *)(ent) + sizeof(__u32) > last_base_addr ||
 			(void *)XATTR_NEXT_ENTRY(ent) > last_base_addr) {
 			ASSERT_MSG("[0x%x] last xattr entry (offset: %lx) "
 					"crosses the boundary",
 					nid, (long int)((void *)ent - xattr));
-			if (c.fix_on) {
-				memset(ent, 0,
-					(char *)last_base_addr - (char *)ent);
-				write_all_xattrs(sbi, inode, xattr_size, xattr);
-				FIX_MSG("[0x%x] nullify wrong xattr entries",
-						nid);
-				return 1;
-			}
+			need_fix = true;
 			break;
 		}
 	}
-
+	if (!need_fix &&
+	    !is_zeroed((u8 *)ent, (u8 *)last_base_addr - (u8 *)ent)) {
+		ASSERT_MSG("[0x%x] nonzero bytes in xattr space after "
+				"end of list", nid);
+		need_fix = true;
+	}
+	if (need_fix && c.fix_on) {
+		memset(ent, 0, (u8 *)last_base_addr - (u8 *)ent);
+		write_all_xattrs(sbi, inode, xattr_size, xattr);
+		FIX_MSG("[0x%x] nullify wrong xattr entries", nid);
+		free(xattr);
+		return 1;
+	}
+	free(xattr);
 	return 0;
 }
 
 /* start with valid nid and blkaddr */
 void fsck_chk_inode_blk(struct f2fs_sb_info *sbi, u32 nid,
 		enum FILE_TYPE ftype, struct f2fs_node *node_blk,
 		u32 *blk_cnt, struct f2fs_compr_blk_cnt *cbc,
 		struct node_info *ni, struct child_info *child_d)
 {
 	struct f2fs_fsck *fsck = F2FS_FSCK(sbi);