[PATCH] ocfs2: allow xattr bucket entries to span multiple blocks

[PATCH] ocfs2: allow xattr bucket entries to span multiple blocks

[Joseph Qi]

ocfs2_validate_xattr_bucket() limits the entry array to the first
bucket block, but the write path stores entries across the whole
OCFS2_XATTR_BUCKET_SIZE region.  With 512-byte blocks a bucket spans
eight blocks, and a bucket filled with small xattrs places its last
entries past offset 512.  Reading such a bucket back errors out:

  OCFS2: ERROR (device loop0): ocfs2_validate_xattr_bucket: Invalid xattr bucket 86072: entry count 32 exceeds maximum 31
  On-disk corruption discovered. Please run fsck.ocfs2 once the filesystem is unmounted.
  OCFS2: File system is now read-only.

This is reproducible by setting ~33 xattrs with 100-byte values on a
file on a blocksize-512 volume; fsck.ocfs2 reports the resulting
image clean.

Check the entry count against the full bucket region instead.  The
per-block bounds checks for names and values stay as they are, since
ocfs2_bucket_align_free_start() keeps each name+value pair within a
single block.

Fixes: 2cf82b46d5e4 ("ocfs2: validate external xattr entries when reading metadata")
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
---
 fs/ocfs2/xattr.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 34f102db2a0e..c73628484b50 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -1153,7 +1153,13 @@ static int ocfs2_validate_xattr_bucket(struct ocfs2_xattr_bucket *bucket,
 	struct ocfs2_xattr_header *xh = bucket_xh(bucket);
 	u16 xattr_count = le16_to_cpu(xh->xh_count);
 	size_t region_size = (size_t)sb->s_blocksize * bucket->bu_blocks;
-	size_t entries_limit = sb->s_blocksize;
+	/*
+	 * The entry array grows up from the header across the whole
+	 * bucket region, so it may extend beyond the first bucket block
+	 * when the blocksize is smaller than OCFS2_XATTR_BUCKET_SIZE.
+	 * Name/value pairs, however, always live within a single block.
+	 */
+	size_t entries_limit = region_size;
 	size_t nv_limit = sb->s_blocksize;
 	size_t max_entries;
 	int i, ret;
-- 
2.39.3

[PATCH v2 0/2] ocfs2: xattr bucket validation fixes

[Joseph Qi]

This series fixes two problems around xattr bucket validation.

Patch 1 fixes a false-corruption failure on blocksize-512 volumes:
the bucket validator limited the entry array to the first bucket
block while the write path stores entries across the whole 4096-byte
bucket region, so a legitimately written, fsck-clean bucket could be
rejected and force the filesystem read-only.  It also adds an
alignment check on the bucket block number, since the entry array is
accessed as one contiguous region and a corrupted xattr tree could
otherwise point a bucket at blocks straddling a page boundary.

Patch 2 converts two mlog_bug_on_msg() checks in the bucket defrag
path to ocfs2_error() returns, so that a corrupt bucket holding
overlapping entries or an inflated xh_free_start marks the filesystem
read-only and fails the setxattr instead of panicking the kernel.

Both patches have been tested in QEMU: the blocksize-512 reproducer
(40 xattrs with 100-byte values, previously failing with "entry count
32 exceeds maximum 31") now passes with a clean fsck.ocfs2 result,
and the ocfs2 testsuite xattr tests pass 48/48 across blocksize
combinations.

Changes since v1:
- v1 was a single patch ("ocfs2: allow xattr bucket entries to span
  multiple blocks").  The series is now split into two fixes.
- Patch 1: reject buckets whose first block number is not aligned to
  the bucket size before validating the entry array; the flat entry
  access could otherwise read out of bounds for a bucket from a
  corrupted xattr tree that straddles a page boundary.
- Patch 2 is new: return an error instead of BUG() when defrag finds
  inconsistent bucket contents.

Joseph Qi (2):
  ocfs2: allow xattr bucket entries to span multiple blocks
  ocfs2: reject inconsistent xattr bucket during defrag

 fs/ocfs2/xattr.c | 37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

-- 
2.39.3

[PATCH v2 1/2] ocfs2: allow xattr bucket entries to span multiple blocks

[Joseph Qi]

ocfs2_validate_xattr_bucket() limits the entry array to the first
bucket block, but the write path stores entries across the whole
OCFS2_XATTR_BUCKET_SIZE region.  With 512-byte blocks a bucket spans
eight blocks, and a bucket filled with small xattrs places its last
entries past offset 512.  Reading such a bucket back errors out:

  OCFS2: ERROR (device loop0): ocfs2_validate_xattr_bucket: Invalid xattr bucket 86072: entry count 32 exceeds maximum 31
  On-disk corruption discovered. Please run fsck.ocfs2 once the filesystem is unmounted.
  OCFS2: File system is now read-only.

This is reproducible by setting ~33 xattrs with 100-byte values on a
file on a blocksize-512 volume; fsck.ocfs2 reports the resulting
image clean.

Check the entry count against the full bucket region instead.  The
per-block bounds checks for names and values stay as they are, since
ocfs2_bucket_align_free_start() keeps each name+value pair within a
single block.

The entry array is one contiguous region, so a bucket from a corrupted
xattr tree whose first block is not aligned to OCFS2_XATTR_BUCKET_SIZE
could straddle a page and make the validation loop read out of bounds.
Buckets allocated within clusters are always aligned, so reject any
other block number while validating.

Fixes: 2cf82b46d5e4 ("ocfs2: validate external xattr entries when reading metadata")
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
---
 fs/ocfs2/xattr.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 34f102db2a0e..c71fa7983b73 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -1153,11 +1153,30 @@ static int ocfs2_validate_xattr_bucket(struct ocfs2_xattr_bucket *bucket,
 	struct ocfs2_xattr_header *xh = bucket_xh(bucket);
 	u16 xattr_count = le16_to_cpu(xh->xh_count);
 	size_t region_size = (size_t)sb->s_blocksize * bucket->bu_blocks;
-	size_t entries_limit = sb->s_blocksize;
+	/*
+	 * The entry array grows up from the header across the whole
+	 * bucket region, so it may extend beyond the first bucket block
+	 * when the blocksize is smaller than OCFS2_XATTR_BUCKET_SIZE.
+	 * Name/value pairs, however, always live within a single block.
+	 */
+	size_t entries_limit = region_size;
 	size_t nv_limit = sb->s_blocksize;
 	size_t max_entries;
 	int i, ret;
 
+	/*
+	 * The entry array is one contiguous region that may span the
+	 * bucket's buffer_heads.  Buckets are allocated within clusters,
+	 * so their first block is always aligned to
+	 * OCFS2_XATTR_BUCKET_SIZE and the whole bucket fits in one page.
+	 * A corrupted xattr tree can point a bucket at blocks straddling
+	 * a page, so reject it before touching the entry array.
+	 */
+	if (blkno & (bucket->bu_blocks - 1))
+		return ocfs2_error(sb,
+				   "Invalid xattr bucket %llu: unaligned block number\n",
+				   (unsigned long long)blkno);
+
 	if (region_size < sizeof(*xh))
 		return ocfs2_error(sb,
 				   "Invalid xattr bucket %llu: region size %zu is too small\n",
-- 
2.39.3

[PATCH v2 2/2] ocfs2: reject inconsistent xattr bucket during defrag

[Joseph Qi]

ocfs2_defrag_xattr_bucket() has two mlog_bug_on_msg() checks that
assume the name/value pairs in a bucket are disjoint and that
xh_free_start is not below the compacted region.

ocfs2_validate_xattr_bucket() only checks each entry in isolation,
so a corrupt bucket holding overlapping entries, or one with an
inflated xh_free_start, passes validation and then hits BUG() in
defrag when a setxattr triggers it.

Defrag works on a linear copy of the bucket and does not touch the
real blocks before the copy back, so the checks can return an error
instead of calling BUG().

Fixes: 012255961c9e ("ocfs2: Enable xattr set in index btree")
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
---
 fs/ocfs2/xattr.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index c71fa7983b73..e6c49adaac8a 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -4804,16 +4804,22 @@ static int ocfs2_defrag_xattr_bucket(struct inode *inode,
 			memmove(bucket_buf + end - len,
 				bucket_buf + offset, len);
 			xe->xe_name_offset = cpu_to_le16(end - len);
+		} else if (end < offset + len) {
+			ret = ocfs2_error(inode->i_sb,
+					  "Defrag check failed for bucket %llu\n",
+					  (unsigned long long)blkno);
+			goto out;
 		}
 
-		mlog_bug_on_msg(end < offset + len, "Defrag check failed for "
-				"bucket %llu\n", (unsigned long long)blkno);
-
 		end -= len;
 	}
 
-	mlog_bug_on_msg(xh_free_start > end, "Defrag check failed for "
-			"bucket %llu\n", (unsigned long long)blkno);
+	if (xh_free_start > end) {
+		ret = ocfs2_error(inode->i_sb,
+				  "Defrag check failed for bucket %llu\n",
+				  (unsigned long long)blkno);
+		goto out;
+	}
 
 	if (xh_free_start == end)
 		goto out;
-- 
2.39.3