ocfs2: fix UBSAN array-index-out-of-bounds in ocfs2_sum_rightmost_rec

[BUG]
On-disk corruption setting l_next_free_rec to 0 in an inode's inline extent list triggers a UBSAN panic on the next write to that file.

[CAUSE]
ocfs2_sum_rightmost_rec() computes
i = le16_to_cpu(el->l_next_free_rec) - 1
and accesses el->l_recs[i] without validating i. When l_next_free_rec is 0, i becomes -1; when l_next_free_rec exceeds l_count, i falls past the end of the array. Either case violates the __counted_by_le(l_count) annotation on l_recs[] and triggers UBSAN.

[FIX]
Read l_count once into a local variable to eliminate a TOCTOU between the bounds check and the __counted_by_le-generated check at the array access. Validate i against both bounds before dereferencing l_recs[]. On an out-of-bounds index call ocfs2_error() since both cases indicate filesystem corruption.

Update the signature to accept a struct ocfs2_caching_info *ci and return int, with the cluster sum returned through a u32 out-parameter. Update both callers to pass et->et_ci and propagate the error.

Fixes 2f26f58df041 ("ocfs2: annotate flexible array members with __counted_by_le()")

Reported-by [email protected]

Closes https://syzkaller.appspot.com/bug?extid=be16e33db01e6644db7a

Signed-off-by Ian Bridges <[email protected]>

---
This patch contains a proposed fix for a crash reported by syzbot in ocfs2_grow_tree().

The file names and offsets in this description are from commit 7cb1c5b32a2bfde961fff8d5204526b609bcb30a from this repo:
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git

I also have a small test harness that reproduces the original panic, which I can make available as well.

The Bug

The ocfs2_extent_list structure (fs/ocfs2/ocfs2_fs.h:458) contains two fields relevant to this bug: l_count, the total number of extent record slots in the list, and l_next_free_rec, the index of the next unused slot. The extent record array l_recs is annotated __counted_by_le(l_count) (fs/ocfs2/ocfs2_fs.h:472), which instructs UBSAN to bounds-check every access to l_recs against l_count.

ocfs2_sum_rightmost_rec() (fs/ocfs2/alloc.c:1106) is a helper used by both ocfs2_add_branch() and ocfs2_shift_tree_depth() to compute the logical cluster position just past the rightmost occupied extent record. It derives the index of that record as:

i = le16_to_cpu(el->l_next_free_rec) - 1; /* alloc.c:1110 */

and then accesses el->l_recs[i] (fs/ocfs2/alloc.c:1112) without first checking that i is a valid index. This is the root cause of the bug.

The syzbot report shows index -1 at the crash site, which means l_next_free_rec was 0 at the point of the crash. The crash occurs inside ocfs2_shift_tree_depth() (fs/ocfs2/alloc.c:1375), which is reached when ocfs2_find_branch_target() returns 1. That return value is only produced when l_next_free_rec equals l_count (fs/ocfs2/alloc.c:1530). Since l_next_free_rec is 0, l_count must also be 0 for this condition to hold.

The normal inode read path, ocfs2_validate_inode_block() (fs/ocfs2/inode.c:1423), does not validate the inline extent list fields l_count or l_next_free_rec. A filesystem image with these fields set to 0 in an inode's inline extent list therefore passes read-time validation without error.

The syzbot console log shows a separate validation error at mount time — "Invalid dinode #17058: Corrupt state (nlink = 0 or mode = 0)" — indicating that the mounted filesystem contained at least one other corrupted inode. No reproducer was available in the report, so the exact mechanism by which the corruption was introduced is not known.

Here is a breakdown of how the crash is triggered:

1. A write syscall eventually calls into ocfs2_insert_extent() to record the newly allocated cluster in the inode's extent tree.

2. ocfs2_insert_extent() determines that the extent tree has no room for a new record and calls ocfs2_grow_tree() (fs/ocfs2/alloc.c:1550).

3. ocfs2_grow_tree() calls ocfs2_find_branch_target() (fs/ocfs2/alloc.c:1561) to walk the tree and find a node with a free slot. Because l_tree_depth is 0, the traversal loop (fs/ocfs2/alloc.c:1491) does not execute. At fs/ocfs2/alloc.c:1530, the condition

el->l_next_free_rec == el->l_count /* 0 == 0 */

evaluates to true, so the function returns 1, indicating the tree must grow by shifting its depth.

4. Back in ocfs2_grow_tree(), shift is 1, so ocfs2_shift_tree_depth() is called (fs/ocfs2/alloc.c:1581).

5. ocfs2_shift_tree_depth() (fs/ocfs2/alloc.c:1375) allocates a new extent block and copies the root extent list into it. At fs/ocfs2/alloc.c:1420, it sets:

eb_el->l_next_free_rec = root_el->l_next_free_rec; /* = 0 */

The copy loop at fs/ocfs2/alloc.c:1421 runs zero iterations since l_next_free_rec is 0, so eb_el is left with an empty extent list.

6. ocfs2_shift_tree_depth() then calls ocfs2_sum_rightmost_rec(eb_el) (fs/ocfs2/alloc.c:1433) to determine the cluster count for the new root record.

7. Inside ocfs2_sum_rightmost_rec(), i is computed as:

i = le16_to_cpu(eb_el->l_next_free_rec) - 1 = 0 - 1 = -1

The subsequent access to el->l_recs[-1] (fs/ocfs2/alloc.c:1112) violates the __counted_by_le(l_count) annotation on l_recs[] (fs/ocfs2/ocfs2_fs.h:472). __counted_by_le is a macro defined in include/linux/compiler_types.h that expands to __counted_by, which is the GCC/Clang attribute UBSAN uses for array bounds checking. The UBSAN error message reports __counted_by rather than __counted_by_le because the check is performed against the expanded attribute. This triggers a UBSAN array-index-out-of-bounds panic.

The Proposed Fix

The fix modifies ocfs2_sum_rightmost_rec() with three changes.

First, l_count is read once into a local variable before the bounds check. The __counted_by_le(l_count) annotation causes the compiler to emit a separate read of el->l_count at the array access site for the UBSAN check. Without the local variable, there are two independent reads of l_count — one for the guard and one at the array access site, where __counted_by_le causes the compiler to re-read it for the UBSAN check — creating a TOCTOU between them. Reading l_count once before the guard reduces this window to the minimum.

Second, the index is checked against both bounds before dereferencing l_recs[]. Both checks are sound: i < 0 compares against the constant 0, requiring no trust in any on-disk value; i >= count checks the invariant l_next_free_rec <= l_count, which holds on any valid filesystem independent of the actual field values. Neither check can fire on a valid extent list. Corruption that keeps l_next_free_rec within [1, l_count] will pass the check and produce an incorrect result, but this is pre-existing behaviour — not a regression — and cannot be avoided without an external ground truth for l_count, which I do not believe exists for the inode's inline extent list.

Third, on an out-of-bounds index, ocfs2_error() is called rather than silently returning a usable value.

To accommodate these changes the function signature is updated: a struct ocfs2_caching_info *ci parameter is added for superblock access, the return type changes from u32 to int, the cluster sum is returned via a new u32 out-parameter, and the inline qualifier is removed since the function is no longer a trivial helper. Both callers, ocfs2_add_branch() and ocfs2_shift_tree_depth(), already have status variables and bail labels, and are updated to pass et->et_ci, check the return value, and propagate any error up their respective call chains.

Quoted 66 lines · Ian Bridges

[BUG]
On-disk corruption setting l_next_free_rec to 0 in an inode's inline extent list triggers a UBSAN panic on the next write to that file.

[CAUSE]
ocfs2_sum_rightmost_rec() computes
i = le16_to_cpu(el->l_next_free_rec) - 1
and accesses el->l_recs[i] without validating i. When l_next_free_rec is 0, i becomes -1; when l_next_free_rec exceeds l_count, i falls past the end of the array. Either case violates the __counted_by_le(l_count) annotation on l_recs[] and triggers UBSAN.

[FIX]
Read l_count once into a local variable to eliminate a TOCTOU between the bounds check and the __counted_by_le-generated check at the array access. Validate i against both bounds before dereferencing l_recs[]. On an out-of-bounds index call ocfs2_error() since both cases indicate filesystem corruption.

Update the signature to accept a struct ocfs2_caching_info *ci and return int, with the cluster sum returned through a u32 out-parameter. Update both callers to pass et->et_ci and propagate the error.

Fixes 2f26f58df041 ("ocfs2: annotate flexible array members with __counted_by_le()")

Reported-by [email protected]

Closes https://syzkaller.appspot.com/bug?extid=be16e33db01e6644db7a

Signed-off-by Ian Bridges <[email protected]>

---
This patch contains a proposed fix for a crash reported by syzbot in ocfs2_grow_tree().

The file names and offsets in this description are from commit 7cb1c5b32a2bfde961fff8d5204526b609bcb30a from this repo:
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git

I also have a small test harness that reproduces the original panic, which I can make available as well.

The Bug

The ocfs2_extent_list structure (fs/ocfs2/ocfs2_fs.h:458) contains two fields relevant to this bug: l_count, the total number of extent record slots in the list, and l_next_free_rec, the index of the next unused slot. The extent record array l_recs is annotated __counted_by_le(l_count) (fs/ocfs2/ocfs2_fs.h:472), which instructs UBSAN to bounds-check every access to l_recs against l_count.

ocfs2_sum_rightmost_rec() (fs/ocfs2/alloc.c:1106) is a helper used by both ocfs2_add_branch() and ocfs2_shift_tree_depth() to compute the logical cluster position just past the rightmost occupied extent record. It derives the index of that record as:

i = le16_to_cpu(el->l_next_free_rec) - 1; /* alloc.c:1110 */

and then accesses el->l_recs[i] (fs/ocfs2/alloc.c:1112) without first checking that i is a valid index. This is the root cause of the bug.

The syzbot report shows index -1 at the crash site, which means l_next_free_rec was 0 at the point of the crash. The crash occurs inside ocfs2_shift_tree_depth() (fs/ocfs2/alloc.c:1375), which is reached when ocfs2_find_branch_target() returns 1. That return value is only produced when l_next_free_rec equals l_count (fs/ocfs2/alloc.c:1530). Since l_next_free_rec is 0, l_count must also be 0 for this condition to hold.

Quoted 6 lines

The normal inode read path, ocfs2_validate_inode_block() (fs/ocfs2/inode.c:1423), does not validate the inline extent list fields l_count or l_next_free_rec. A filesystem image with these fields set to 0 in an inode's inline extent list therefore passes read-time validation without error.

So why not move the check into ocfs2_validate_inode_block()? Seems that is the right place to do this kind of work.

Thanks,
Joseph

Quoted 93 lines

The syzbot console log shows a separate validation error at mount time — "Invalid dinode #17058: Corrupt state (nlink = 0 or mode = 0)" — indicating that the mounted filesystem contained at least one other corrupted inode. No reproducer was available in the report, so the exact mechanism by which the corruption was introduced is not known.

Here is a breakdown of how the crash is triggered:

1. A write syscall eventually calls into ocfs2_insert_extent() to record the newly allocated cluster in the inode's extent tree.

2. ocfs2_insert_extent() determines that the extent tree has no room for a new record and calls ocfs2_grow_tree() (fs/ocfs2/alloc.c:1550).

3. ocfs2_grow_tree() calls ocfs2_find_branch_target() (fs/ocfs2/alloc.c:1561) to walk the tree and find a node with a free slot. Because l_tree_depth is 0, the traversal loop (fs/ocfs2/alloc.c:1491) does not execute. At fs/ocfs2/alloc.c:1530, the condition

el->l_next_free_rec == el->l_count /* 0 == 0 */

evaluates to true, so the function returns 1, indicating the tree must grow by shifting its depth.

4. Back in ocfs2_grow_tree(), shift is 1, so ocfs2_shift_tree_depth() is called (fs/ocfs2/alloc.c:1581).

5. ocfs2_shift_tree_depth() (fs/ocfs2/alloc.c:1375) allocates a new extent block and copies the root extent list into it. At fs/ocfs2/alloc.c:1420, it sets:

eb_el->l_next_free_rec = root_el->l_next_free_rec; /* = 0 */

The copy loop at fs/ocfs2/alloc.c:1421 runs zero iterations since l_next_free_rec is 0, so eb_el is left with an empty extent list.

6. ocfs2_shift_tree_depth() then calls ocfs2_sum_rightmost_rec(eb_el) (fs/ocfs2/alloc.c:1433) to determine the cluster count for the new root record.

7. Inside ocfs2_sum_rightmost_rec(), i is computed as:

i = le16_to_cpu(eb_el->l_next_free_rec) - 1 = 0 - 1 = -1

The subsequent access to el->l_recs[-1] (fs/ocfs2/alloc.c:1112) violates the __counted_by_le(l_count) annotation on l_recs[] (fs/ocfs2/ocfs2_fs.h:472). __counted_by_le is a macro defined in include/linux/compiler_types.h that expands to __counted_by, which is the GCC/Clang attribute UBSAN uses for array bounds checking. The UBSAN error message reports __counted_by rather than __counted_by_le because the check is performed against the expanded attribute. This triggers a UBSAN array-index-out-of-bounds panic.

The Proposed Fix

The fix modifies ocfs2_sum_rightmost_rec() with three changes.

First, l_count is read once into a local variable before the bounds check. The __counted_by_le(l_count) annotation causes the compiler to emit a separate read of el->l_count at the array access site for the UBSAN check. Without the local variable, there are two independent reads of l_count — one for the guard and one at the array access site, where __counted_by_le causes the compiler to re-read it for the UBSAN check — creating a TOCTOU between them. Reading l_count once before the guard reduces this window to the minimum.

Second, the index is checked against both bounds before dereferencing l_recs[]. Both checks are sound: i < 0 compares against the constant 0, requiring no trust in any on-disk value; i >= count checks the invariant l_next_free_rec <= l_count, which holds on any valid filesystem independent of the actual field values. Neither check can fire on a valid extent list. Corruption that keeps l_next_free_rec within [1, l_count] will pass the check and produce an incorrect result, but this is pre-existing behaviour — not a regression — and cannot be avoided without an external ground truth for l_count, which I do not believe exists for the inode's inline extent list.

Third, on an out-of-bounds index, ocfs2_error() is called rather than silently returning a usable value.

To accommodate these changes the function signature is updated: a struct ocfs2_caching_info *ci parameter is added for superblock access, the return type changes from u32 to int, the cluster sum is returned via a new u32 out-parameter, and the inline qualifier is removed since the function is no longer a trivial helper. Both callers, ocfs2_add_branch() and ocfs2_shift_tree_depth(), already have status variables and bail labels, and are updated to pass et->et_ci, check the return value, and propagate any error up their respective call chains.