diff mbox series

[1/8] e2fsck: fix in-inode extended attribute checking

Message ID 20260924222944.3683556-2-sandeen@redhat.com
State New
Headers show
Series e2fsprogs: fix extended attribute iteration loop bounds checking | expand

Commit Message

Eric Sandeen Sept. 24, 2026, 10:23 p.m. UTC 
From: Andreas Gruenbacher <agruenba@redhat.com>

Extended attribute areas in inodes and on separate blocks are stored in
the following format: (1) first comes the list of name records, (2)
followed by an end marker, followed by any (3) free space that remains,
followed by (4) any attribute values that are not stored in separate
inodes.  The name records and values are all variable-length and 4-byte
aligned.  The end marker is mandatory.

A lot of the loops iterating over those extended attribute areas get this
wrong.  This patch fixes the loop in check_ea_in_inode:

- First, we can safely assume that the total size is at least 4 bytes.
  Take 4 bytes (the size of the end marker) off of the remaining space
  to make sure that enough space remains available for the end marker.
  (We can safely assume that the total size is at least 4 bytes.)

- Second, at the top of the loop, we can assume that the area contains
  at least an end marker, but we cannot assume that the list still
  contains an entire ext2_ext_attr_entry header.

- Third, since the loop condition now no longer checks if we have a
  complete ext2_ext_attr_entry header, check if we still have a complete
  name record of size EXT2_EXT_ATTR_LEN(entry->e_name_len).  The length
  of the name is stored in the first byte of the entry, so we can safely
  access it.

- Fourth, value are 4-byte aligned, so take that into consideration when
  calculating the remaining free space.

With these changes to check_ea_in_inode(), an 'Extended attribute in
inode has Aa namelen which is invalid' error is detected before
allocation is checked, which masks the previous 'Inode extended attribute
is corrupt (allocation collision)' error in test f_inode_ea_collision.
Adjust the expected test result.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 e2fsck/pass1.c                      | 32 ++++++++++-------------------
 tests/f_inode_ea_collision/expect.1 |  3 ++-
 2 files changed, 13 insertions(+), 22 deletions(-)
diff mbox series

Patch

diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
index c9711446..555429b6 100644
--- a/e2fsck/pass1.c
+++ b/e2fsck/pass1.c
@@ -501,36 +501,31 @@  static void check_ea_in_inode(e2fsck_t ctx, struct problem_context *pctx,
 		goto fix;
 	}
 
-	while (remain >= sizeof(struct ext2_ext_attr_entry) &&
-	       !EXT2_EXT_IS_LAST_ENTRY(entry)) {
+	while (!EXT2_EXT_IS_LAST_ENTRY(entry)) {
 		__u32 hash;
 
-		if (region_allocate(region, (char *)entry - (char *)header,
-				    EXT2_EXT_ATTR_LEN(entry->e_name_len))) {
-			problem = PR_1_INODE_EA_ALLOC_COLLISION;
-			goto fix;
-		}
-
-		/* header eats this space */
-		remain -= sizeof(struct ext2_ext_attr_entry);
-
-		/* is attribute name valid? */
-		if (EXT2_EXT_ATTR_SIZE(entry->e_name_len) > remain) {
+		/* entry->e_name_len is within the first four bytes */
+		if (EXT2_EXT_ATTR_LEN(entry->e_name_len) > remain) {
 			pctx->num = entry->e_name_len;
 			problem = PR_1_ATTR_NAME_LEN;
 			goto fix;
 		}
+		remain -= EXT2_EXT_ATTR_LEN(entry->e_name_len);
 
-		/* attribute len eats this space */
-		remain -= EXT2_EXT_ATTR_SIZE(entry->e_name_len);
+		if (region_allocate(region, (char *)entry - (char *)header,
+				    EXT2_EXT_ATTR_LEN(entry->e_name_len))) {
+			problem = PR_1_INODE_EA_ALLOC_COLLISION;
+			goto fix;
+		}
 
 		if (entry->e_value_inum == 0) {
 			/* check value size */
-			if (entry->e_value_size > remain) {
+			if (EXT2_EXT_ATTR_SIZE(entry->e_value_size) > remain) {
 				pctx->num = entry->e_value_size;
 				problem = PR_1_ATTR_VALUE_SIZE;
 				goto fix;
 			}
+			remain -= EXT2_EXT_ATTR_SIZE(entry->e_value_size);
 
 			if (entry->e_value_size &&
 			    region_allocate(region,
@@ -571,11 +566,6 @@  static void check_ea_in_inode(e2fsck_t ctx, struct problem_context *pctx,
 			ea_ibody_quota->inodes++;
 		}
 
-		/* If EA value is stored in external inode then it does not
-		 * consume space here */
-		if (entry->e_value_inum == 0)
-			remain -= entry->e_value_size;
-
 		entry = EXT2_EXT_ATTR_NEXT(entry);
 	}
 
diff --git a/tests/f_inode_ea_collision/expect.1 b/tests/f_inode_ea_collision/expect.1
index a67a5f19..dfe49a9f 100644
--- a/tests/f_inode_ea_collision/expect.1
+++ b/tests/f_inode_ea_collision/expect.1
@@ -1,7 +1,8 @@ 
 Pass 1: Checking inodes, blocks, and sizes
 Inode 12 extended attribute is corrupt (allocation collision).  Clear? yes
 
-Inode 13 extended attribute is corrupt (allocation collision).  Clear? yes
+Extended attribute in inode 13 has a namelen (98) which is invalid
+Clear? yes
 
 Inode 14 extended attribute is corrupt (allocation collision).  Clear? yes