Re: [PATCH 2/7] mm/damon/core-kunit: check region count before testing in split_at()

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Kernel memory corruption in `damon_apply_min_nr_regions()` due to unhandled split failure.
- [Medium] Incomplete test fix for memory allocation failure in `damos_filter_match()` leading to test crash.
- [Medium] Missing upper bound checks in test loops leading to Out-Of-Bounds array reads.
--

--- Patch [2]: [PATCH 2/7] mm/damon/core-kunit: check region count before testing in split_at() ---
commit c03465ac93de9ac544447d782c3c2ea15bf60ab1
Author: SJ Park <[email protected]>

mm/damon/core-kunit: check region count before testing in split_at()

damon_test_split_at() tests the next region that is assumed to be created by
damon_split_region_at(). But the split might fail. In this case, the test is
updated to check if the number of regions changed as expected and exit early.

Link: https://lore.kernel.org/[email protected]

> diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> index eba643762132f..322a295ac2cec 100644
> --- a/mm/damon/tests/core-kunit.h
> +++ b/mm/damon/tests/core-kunit.h
> @@ -155,6 +155,10 @@ static void damon_test_split_at(struct kunit *test)
>  	r->age = 10;
>  	damon_add_region(r, t);
>  	damon_split_region_at(t, r, 25);
> +	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 2);
> +	if (damon_nr_regions(t) != 2)
> +		goto out;
> +

[Severity: Medium]
This isn't a bug introduced by this patch, but it looks like the exact
same pattern being fixed here is present in damos_test_filter_out().

If damos_filter_match() fails to allocate memory, it returns without
splitting the region. The test logs a failure for the region count mismatch
but execution continues unconditionally:

mm/damon/tests/core-kunit.h:damos_test_filter_out() {
        /* filter should have split the region */
        ...
        KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 2);
        r2 = damon_next_region(r);
        ...
        damon_destroy_region(r2, t);
}

Since r is the only item in the list, does damon_next_region(r) resolve to
an out-of-bounds pointer that is then passed to damon_destroy_region(),
causing list and slab corruption during test execution?

[Severity: High]
This is a pre-existing issue, but since we are handling split failures in 
the tests, does mm/damon/core.c:damon_apply_min_nr_regions() suffer from a
similar problem in production code?

mm/damon/core.c:damon_apply_min_nr_regions() {
        damon_for_each_target(t, ctx) {
                damon_for_each_region_safe(r, next, t) {
                        while (damon_sz_region(r) > max_region_sz) {
                                damon_split_region_at(t, r, max_region_sz);
                                r = damon_next_region(r);
                        }
                }
        }
}

If damon_split_region_at() fails to allocate memory, it silently returns. 
The loop then unconditionally advances r = damon_next_region(r). If r was 
the last region, this returns a bogus pointer derived from the list head. 

The loop condition damon_sz_region(r) then evaluates garbage memory. If it
happens to evaluate to greater than max_region_sz, the loop re-enters and
passes the bogus pointer to damon_split_region_at(). Could this lead to
out-of-bounds writes and list corruption under memory pressure?

[Severity: Medium]
This is also a pre-existing issue, but while reviewing the kunit tests, I 
noticed several test loops lack upper bound checks against the destination
counts. 

For example, in damos_test_commit_dests_for():

mm/damon/tests/core-kunit.h:damos_test_commit_dests_for() {
        KUNIT_EXPECT_EQ(test, dst.nr_dests, src_nr_dests);
        for (i = 0; i < dst.nr_dests; i++) {
                KUNIT_EXPECT_EQ(test, dst.node_id_arr[i], src_node_id_arr[i]);
        ...
}

If the code under test incorrectly leaves dst.nr_dests greater than
src_nr_dests (for instance, when src_nr_dests is 0), the loop iterates out 
of bounds and dereferences src_node_id_arr[i]. 

The same unbounded iteration pattern appears to exist in 
damos_test_commit_quota_goals_for() and damon_test_commit_target_regions_for(). 
Could these also be fixed to prevent out-of-bounds reads during test failures?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2