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

On Sat, 18 Jul 2026 00:25:43 +0000 [email protected] wrote:

> 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 <[email protected]>
> 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/[email protected]
> 
> > 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?

Yes, and another patch in this series fixes it.

> 
> [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?

Yes, and another patch in this series fixes it.

> 
> >  	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/[email protected]?part=5


Thanks,
SJ