Re: [PATCH 4/7] mm/damon/core-kunit: handle region split failure in filter_out()

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

Pre-existing issues:
- [High] The patch adds an early exit check `if (damon_nr_regions(t) != 2) goto out;` to prevent an invalid pointer dereference when the first region split test fails, but misses the exact same vulnerability in the second region split test directly below it.
- [High] In `damon_apply_min_nr_regions`, if `damon_split_region_at` fails to allocate memory, the inner loop advances to the next list entry without inserting a new region, leading to an out-of-bounds read and write when it reaches the list head.
--

--- Patch [4]: [PATCH 4/7] mm/damon/core-kunit: handle region split failure in filter_out() ---
commit 705cf12a4e8501ec816031495df35460614c42c2
Author: SJ Park <[email protected]>

mm/damon/core-kunit: handle region split failure in filter_out()
    
damos_test_filter_out() checks if damos_filter_match() of an address
filter splits the region as expected. But the test continued regardless of
whether the split succeeded, which could lead to dereferencing invalid
pointers or corrupting memory. This adds an early exit check on the first
test block to prevent continuing on failure.

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

> diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> index 322a295ac2cec..bd3bbd421392f 100644
> --- a/mm/damon/tests/core-kunit.h
> +++ b/mm/damon/tests/core-kunit.h
> @@ -1365,6 +1365,8 @@ static void damos_test_filter_out(struct kunit *test)
>  	KUNIT_EXPECT_EQ(test, r->ar.start, 1);
>  	KUNIT_EXPECT_EQ(test, r->ar.end, 2);
>  	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 2);
> +	if (damon_nr_regions(t) != 2)
> +		goto out;
>  	r2 = damon_next_region(r);
>  	KUNIT_EXPECT_EQ(test, r2->ar.start, 2);
>  	KUNIT_EXPECT_EQ(test, r2->ar.end, 4);
> @@ -1384,6 +1386,7 @@ static void damos_test_filter_out(struct kunit *test)
>  	KUNIT_EXPECT_EQ(test, r2->ar.end, 8);
>  	damon_destroy_region(r2, t);
>  

[Severity: High]
This isn't a bug introduced by this patch, but does this second region split
test have the same vulnerability as the one fixed above?

If damos_filter_match() fails to split the region in this second test block
(e.g. due to memory allocation failure), damon_nr_regions(t) would remain 1.
r2 would be assigned damon_next_region(r), which could incorrectly resolve
to the damon_target list head.

Calling damon_destroy_region(r2, t) would then call kmem_cache_free() on
this out-of-bounds pointer, potentially corrupting memory. Should another
early exit check be added here as well?

> +out:
>  	damon_free_target(t);
>  	damos_free_filter(f);
>  }

[Severity: High]
This is a pre-existing issue, but while looking at region splits, could a
similar unchecked failure cause memory corruption in mm/damon/core.c?

In damon_apply_min_nr_regions():

			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 due to memory pressure, it does not split
the region. The call to damon_next_region(r) then erroneously advances r to
the next element.

If r was the last region, it becomes the list head (&t->regions_list). The
loop condition then evaluates damon_sz_region(r) on the list head, which
reads memory 32 bytes before the damon_target struct allocation. If this
difference exceeds max_region_sz, damon_split_region_at() could be called
on the list head, which might subsequently write to r->ar.end, corrupting
SLUB metadata or preceding objects.

Should damon_split_region_at() return an error code so callers can verify
if the split actually succeeded?

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