From fc62f3b40b3076f2398ffe081d6358ab47e044d9 Mon Sep 17 00:00:00 2001 From: Chao Yu Date: Tue, 18 Jul 2023 12:05:23 +0800 Subject: DO NOT MERGE - f2fs-tools: do sanity check on xattr entry 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") Bug: 305658663 Signed-off-by: Chao Yu Signed-off-by: Jaegeuk Kim (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:c208057ce971b24990fb6bd1fee7e39b90ee796e) Merged-In: Ib481ef79f4e0498098559f58066bcb6367d4de29 Change-Id: Ib481ef79f4e0498098559f58066bcb6367d4de29 --- fsck/dump.c | 9 +++++++++ fsck/mount.c | 18 ++++++++++++++---- fsck/xattr.c | 22 +++++++++++++++++++--- fsck/xattr.h | 6 ++++++ 4 files changed, 48 insertions(+), 7 deletions(-) diff --git a/fsck/dump.c b/fsck/dump.c index b8f6144..4275f39 100644 --- a/fsck/dump.c +++ b/fsck/dump.c @@ -353,6 +353,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; @@ -361,10 +362,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 df0314d..055c225 100644 --- a/fsck/mount.c +++ b/fsck/mount.c @@ -238,6 +238,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; @@ -334,13 +335,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 8e66873..863c0b4 100644 --- a/fsck/xattr.c +++ b/fsck/xattr.c @@ -65,11 +65,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) @@ -135,6 +143,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; @@ -169,7 +178,14 @@ 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) { + 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 22ea35c..5a40018 100644 --- a/fsck/xattr.h +++ b/fsck/xattr.h @@ -132,6 +132,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)) -- cgit v1.2.3 From f4dc23c957076f37190afa41ad6ec0bd5cd3cbf4 Mon Sep 17 00:00:00 2001 From: Daeho Jeong Date: Fri, 20 Oct 2023 17:29:06 -0700 Subject: DO NOT MERGE - f2fs-tools: fix corrupted xattr entry Detect and fix a corrupted xattr entry. Bug: 305658663 Signed-off-by: Daeho Jeong Signed-off-by: Jaegeuk Kim (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:d2f788271e6fb5f3ac1b3de4fac39ec5bad76129) Merged-In: Ib3c4d67d2fb8523306ca2502834579d7631ae513 Change-Id: Ib3c4d67d2fb8523306ca2502834579d7631ae513 --- fsck/dump.c | 2 +- fsck/fsck.c | 40 ++++++++++++++++++++++++++++++++++++++++ fsck/fsck.h | 4 +++- fsck/mount.c | 2 +- fsck/xattr.c | 9 +++++---- 5 files changed, 50 insertions(+), 7 deletions(-) diff --git a/fsck/dump.c b/fsck/dump.c index 4275f39..0138a18 100644 --- a/fsck/dump.c +++ b/fsck/dump.c @@ -358,7 +358,7 @@ static void dump_xattr(struct f2fs_sb_info *sbi, struct f2fs_node *node_blk) char xattr_name[F2FS_NAME_LEN] = {0}; int ret; - xattr = read_all_xattrs(sbi, node_blk); + xattr = read_all_xattrs(sbi, node_blk, true); if (!xattr) return; diff --git a/fsck/fsck.c b/fsck/fsck.c index 324c3d5..4a18ecb 100644 --- a/fsck/fsck.c +++ b/fsck/fsck.c @@ -685,6 +685,43 @@ void fsck_reada_all_direct_node_blocks(struct f2fs_sb_info *sbi, } } +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); + + 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; + } + break; + } + } + + 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, @@ -860,6 +897,9 @@ check_next: } } + if (chk_extended_attributes(sbi, nid, node_blk)) + need_fix = 1; + if ((node_blk->i.i_inline & F2FS_INLINE_DATA)) { unsigned int inline_size = MAX_INLINE_DATA(node_blk); if (cur_qtype != -1) diff --git a/fsck/fsck.h b/fsck/fsck.h index dabd8b9..02ccc93 100644 --- a/fsck/fsck.h +++ b/fsck/fsck.h @@ -328,6 +328,8 @@ struct hardlink_cache_entry *f2fs_search_hardlink(struct f2fs_sb_info *sbi, struct dentry *de); /* xattr.c */ -void *read_all_xattrs(struct f2fs_sb_info *, struct f2fs_node *); +void *read_all_xattrs(struct f2fs_sb_info *, struct f2fs_node *, bool); +void write_all_xattrs(struct f2fs_sb_info *sbi, + struct f2fs_node *inode, __u32 hsize, void *txattr_addr); #endif /* _FSCK_H_ */ diff --git a/fsck/mount.c b/fsck/mount.c index 055c225..5bdac76 100644 --- a/fsck/mount.c +++ b/fsck/mount.c @@ -334,7 +334,7 @@ void print_inode_info(struct f2fs_sb_info *sbi, DISP_u32(inode, i_nid[3]); /* indirect */ DISP_u32(inode, i_nid[4]); /* double indirect */ - xattr_addr = read_all_xattrs(sbi, node); + xattr_addr = read_all_xattrs(sbi, node, true); if (!xattr_addr) goto out; diff --git a/fsck/xattr.c b/fsck/xattr.c index 863c0b4..a77b2e6 100644 --- a/fsck/xattr.c +++ b/fsck/xattr.c @@ -17,14 +17,15 @@ #include "node.h" #include "xattr.h" -void *read_all_xattrs(struct f2fs_sb_info *sbi, struct f2fs_node *inode) +void *read_all_xattrs(struct f2fs_sb_info *sbi, struct f2fs_node *inode, + bool sanity_check) { struct f2fs_xattr_header *header; void *txattr_addr; u64 inline_size = inline_xattr_size(&inode->i); nid_t xnid = le32_to_cpu(inode->i.i_xattr_nid); - if (c.func == FSCK && xnid) { + if (c.func == FSCK && xnid && sanity_check) { struct f2fs_node *node_blk = NULL; struct node_info ni; int ret; @@ -88,7 +89,7 @@ static struct f2fs_xattr_entry *__find_xattr(void *base_addr, return entry; } -static void write_all_xattrs(struct f2fs_sb_info *sbi, +void write_all_xattrs(struct f2fs_sb_info *sbi, struct f2fs_node *inode, __u32 hsize, void *txattr_addr) { void *xattr_addr; @@ -175,7 +176,7 @@ int f2fs_setxattr(struct f2fs_sb_info *sbi, nid_t ino, int index, const char *na ret = dev_read_block(inode, ni.blk_addr); ASSERT(ret >= 0); - base_addr = read_all_xattrs(sbi, inode); + base_addr = read_all_xattrs(sbi, inode, true); ASSERT(base_addr); last_base_addr = (void *)base_addr + XATTR_SIZE(&inode->i); -- cgit v1.2.3 From 95ef2b747f99eddc1e14cac4db678efa817bccf5 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 25 Oct 2023 21:00:10 +0000 Subject: DO NOT MERGE - f2fs-tools: ensure that unused xattr space is zeroized Also add a missing free() to fix a memory leak. Bug: 305658663 Test: - On kernel with bug: - Created an f2fs filesystem - Created 250-byte xattr on a directory - Deleted the xattr - Ran fsck.f2fs -f - Mounted filesystem - On kernel without bug: - Created 200-byte xattr on the directory - Listed xattrs The last step shows corruption before this change, but not after it. Signed-off-by: Eric Biggers (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:f7c0ca9f8b02cbb199d0277c8aefb5098df9a867) Merged-In: I5ae77803113683887b3aaec804a0547ceb3d80c4 Change-Id: I5ae77803113683887b3aaec804a0547ceb3d80c4 --- fsck/fsck.c | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/fsck/fsck.c b/fsck/fsck.c index 4a18ecb..d38ad1e 100644 --- a/fsck/fsck.c +++ b/fsck/fsck.c @@ -685,6 +685,17 @@ void fsck_reada_all_direct_node_blocks(struct f2fs_sb_info *sbi, } } +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) { @@ -692,6 +703,7 @@ int chk_extended_attributes(struct f2fs_sb_info *sbi, u32 nid, 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; @@ -707,18 +719,24 @@ int chk_extended_attributes(struct f2fs_sb_info *sbi, u32 nid, 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; } -- cgit v1.2.3