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

[SeongJae Park <sj@kernel.org>]

On Thu, 21 May 2026 15:19:28 +0000 sashiko-bot@kernel.org wrote:

> 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.

No one is caused by this patch, so no blocker for this patch.

The last finding seems worthy to separately work for.  I will work on it.

Please read below for more details.

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

DAMON_SYSFS enforces the power of 2 using damon_commit_ctx() based parameter
validation.  DAMON_RECLAIM and DAMON_LRU_SORT also do the checks in their
..._apply_parameters() functions.  So this cannot happen.

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

As I previously answered [1], that input is unlikely in real use case.
> 
> [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)?

Again, unlikely input in real use case.  To intentionally to this, the user
need sysfs write permission.  If a malicious user got the permission, I think
they can do anything bad?

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

Yes, and the caller should do damon_destroy_ctx() of the ctx.  And DAMON_SYSFS
does that, while vaddr doesn't.  I will work on this.

[1] https://lore.kernel.org/20260521060523.80684-1-sj@kernel.org


Thanks,
SJ

[...]