Re: [RFC PATCH v1.2 01/14] mm/damon/core: safely handle no region case in damon_set_regions()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The ALIGN() macro can overflow when processing malicious or overly large user-provided region 'end' values, creating invalid regions where start > end.
- [High] min_region_sz can be a non-power-of-2 value, which corrupts region boundary calculations when passed to ALIGN() and ALIGN_DOWN().
- [High] Userspace can maliciously trigger a kernel warning (WARN_ONCE) by providing a region where start == end via sysfs.
- [High] A memory allocation failure during region creation results in a partial state update without a rollback, potentially leaving the running DAMON context corrupted.
--

commit fa588352eaf7b151171cca8c2acb0bb710a71c75
Author: SeongJae Park <sj@kernel.org>

mm/damon/core: safely handle no region case in damon_set_regions()

This commit refactors damon_set_regions() to explicitly handle the empty
region list case without depending on the internal implementation of
list_first_entry().

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 4e223857a0f99..d0bdf6e01dff8 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -356,6 +356,19 @@ int damon_set_regions(struct damon_target *t, struct damon_addr_range *ranges,
>  			damon_destroy_region(r, t);
>  	}
>  
> +	if (!damon_nr_regions(t)) {
> +		for (i = 0; i < nr_ranges; i++) {
> +			r = damon_new_region(
> +					ALIGN_DOWN(ranges[i].start,
> +						min_region_sz),

[Severity: High]
This is a pre-existing issue, but can min_region_sz be a non-power-of-2
value here?

If sys_ctx->addr_unit is set to an arbitrary value via sysfs, min_region_sz
might not be a power of 2. The ALIGN() and ALIGN_DOWN() macros internally
perform bitwise AND operations with ~(a - 1), which assumes the alignment
is a power of 2.

Could passing a non-power-of-2 value truncate arbitrary bits and result in
misaligned boundaries where start > end?

> +					ALIGN(ranges[i].end, min_region_sz));

[Severity: Critical]
This is a pre-existing issue, but can ALIGN() overflow when processing
overly large end values?

If a user passes ULONG_MAX as the region end via sysfs, ALIGN(ULONG_MAX,
4096) will overflow and wrap around to 0.

Would this result in damon_new_region(start, 0) being called, creating an
invalid region where start > end, which could underflow the size and cause
out-of-bounds memory accesses?

[Severity: High]
This isn't a bug introduced by this patch, but what happens if userspace
provides a region where start == end?

The sysfs interface end_store accepts arbitrary values, and if start == end,
both bypass the start > end validation check. Since ALIGN_DOWN() and ALIGN()
return identical values for identical inputs, would damon_new_region() be
invoked with equal start and end arguments, allowing an attacker with sysfs
access to maliciously trigger a WARN_ONCE(start >= end)?

> +			if (!r)
> +				return -ENOMEM;

[Severity: High]
This is a pre-existing issue, but does this allocation failure leave the
DAMON context in a corrupted state?

If damon_new_region() returns NULL due to memory pressure, the function
immediately returns -ENOMEM without rolling back the regions that were
already removed or added in the previous loop.

Could this leave the running kdamond thread with a partially modified list
of regions?

> +			damon_add_region(r, t);
> +		}
> +		return 0;
> +	}
> +
>  	r = damon_first_region(t);
[ ... ]

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