Re: [PATCH 5/7] mm/damon/core-kunit: skip wrong dest walk in commit_dests_for()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] If `damos_commit_dests()` fails to allocate the `weight_arr` array, it returns `-ENOMEM` but leaves the `dst` context partially modified with a NULL `weight_arr` and an incorrect `nr_dests`, leading to a NULL pointer dereference during DAMOS scheme execution.
- [Medium] Missing out-of-bounds array bounds checks in the test functions `damos_test_commit_quota_goals_for()` and `damon_test_commit_target_regions_for()` when iterating over tested items.
--

--- Patch [5]: [PATCH 5/7] mm/damon/core-kunit: skip wrong dest walk in commit_dests_for() ---
commit 937f902e215bffeba8d97bcc0725b5bfbb643c41
Author: SJ Park <sj@kernel.org>
Subject: mm/damon/core-kunit: skip wrong dest walk in commit_dests_for()

This patch fixes a potential out-of-bounds access in the KUnit tests for
damos_commit_dests(). It exits early in damos_test_commit_dests_for()
if the expected number of destinations does not match the actual number.

Link: https://lore.kernel.org/20260713144757.39740-1-sj@kernel.org

> diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> index eddf3a0484583..9b8c20a50ad53 100644
> --- a/mm/damon/tests/core-kunit.h
> +++ b/mm/damon/tests/core-kunit.h
> @@ -1008,6 +1008,8 @@ static void damos_test_commit_dests_for(struct kunit *test,
>  	skip = false;
>  
>  	KUNIT_EXPECT_EQ(test, dst.nr_dests, src_nr_dests);
> +	if (dst.nr_dests != src_nr_dests)
> +		goto out;

[Severity: High]
This isn't a bug introduced by this patch, but looking at the function being
tested, could damos_commit_dests() leave the destination context in a
corrupted state on allocation failure?

If kmalloc_array() fails to allocate weight_arr, it returns -ENOMEM but
leaves dst->weight_arr as NULL without resetting dst->nr_dests:

mm/damon/core.c:damos_commit_dests() {
	...
		dst->weight_arr = kmalloc_array(src->nr_dests,
			sizeof(*dst->weight_arr), GFP_KERNEL);
		if (!dst->weight_arr) {
			/* ->node_id_arr will be freed by scheme destruction */
			return -ENOMEM;
		}
	}
	...
}

Would a subsequent execution in damos_va_migrate_dests_add() dereference this
NULL array when iterating over dst->nr_dests, leading to a system crash?

[Severity: Medium]
This is a pre-existing issue, but does the same out-of-bounds array access
vulnerability exist in other test functions in this file?

Similar to the fix applied here, the loops in
damos_test_commit_quota_goals_for() and
damon_test_commit_target_regions_for() iterate over items without bounds
checking, only verifying the length after the loop completes.

For example, in damos_test_commit_quota_goals_for():

mm/damon/tests/core-kunit.h:damos_test_commit_quota_goals_for() {
	...
	damos_for_each_quota_goal(goal, &dst->quota) {
		KUNIT_EXPECT_EQ(test, goal->metric, src_goals[i].metric);
		...
	}
	...
}

Could these loops exceed the bounds of arrays like src_goals[] or
expect_start_end[] if the tested function produces more elements than
expected?

>  	for (i = 0; i < dst.nr_dests; i++) {
>  		KUNIT_EXPECT_EQ(test, dst.node_id_arr[i], src_node_id_arr[i]);
>  		KUNIT_EXPECT_EQ(test, dst.weight_arr[i], src_weight_arr[i]);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718001442.87129-1-sj@kernel.org?part=5