Re: [RFC PATCH v1.1 3/7] mm/damon/vaddr-kunit: check region count in three_regions test

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

Pre-existing issues:
- [High] Out-of-bounds array read in KUnit test helpers due to missing array bounds check and ignored return values
--

--- Patch [3]: [RFC PATCH v1.1 3/7] mm/damon/vaddr-kunit: check region count in three_regions test ---
commit b2d38d564a3004fa6df02a9f1d54af3423302437
Author: SJ Park <[email protected]>

mm/damon/vaddr-kunit: check region count in three_regions test

This commit adds a check in damon_do_test_apply_three_regions() to ensure
damon_set_regions() succeeded in setting the expected number of regions.
This prevents a potential NULL pointer dereference during test iteration.

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

> diff --git a/mm/damon/tests/vaddr-kunit.h b/mm/damon/tests/vaddr-kunit.h
> index 61f844336ffb5..6a95441d193ae 100644
> --- a/mm/damon/tests/vaddr-kunit.h
> +++ b/mm/damon/tests/vaddr-kunit.h

[ ... ]

[Severity: High]
This is a pre-existing issue, but while reviewing this test fix, I noticed
similar missing bounds checks and unhandled return values in
mm/damon/tests/core-kunit.h that can cause out-of-bounds array reads.

In damon_test_commit_target_regions_for():

	damon_commit_target_regions(dst_target, src_target, 1);
	i = 0;
	damon_for_each_region(r, dst_target) {
		KUNIT_EXPECT_EQ(test, r->ar.start, expect_start_end[i][0]);
		KUNIT_EXPECT_EQ(test, r->ar.end, expect_start_end[i][1]);
		i++;
	}

If memory allocation fails inside damon_commit_target_regions(), it returns
an error without modifying dst_target. Because the return value is ignored
here, could this iterate over the original regions of dst_target and read
past the end of the expect_start_end stack array?

A similar pattern appears in damos_test_commit_quota_goals_for():

	damos_commit_quota_goals(&dst, &src);

	i = 0;
	damos_for_each_quota_goal(goal, (&dst)) {
		KUNIT_EXPECT_EQ(test, goal->metric, src_goals[i].metric);
		KUNIT_EXPECT_EQ(test, goal->target_value,
				src_goals[i++].target_value);
	}

Does ignoring the return value of damos_commit_quota_goals() risk an
out-of-bounds read on src_goals[] if the destination goals are not updated
and retain more items than expected?

Finally, in 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]);
		KUNIT_EXPECT_EQ(test, dst.weight_arr[i], src_weight_arr[i]);
	}

Since KUNIT_EXPECT_EQ does not abort test execution on failure, could an
out-of-bounds read occur on src_node_id_arr[] and src_weight_arr[] if
dst.nr_dests ends up larger than src_nr_dests?

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