Re: [RFC PATCH v1.2 2/2] mm/damon/ops-common: prevent >DAMON_MAX_SUBSCORE freq_subscore

[SeongJae Park <sj@kernel.org>]

On Mon, 22 Jun 2026 14:24:27 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [Critical] The patch attempts to fix an out-of-bounds array access by capping `freq_subscore`, but fails to account for integer overflow, leaving the vulnerability fully exploitable.
> - [Critical] The `hotness` weighted average calculation is vulnerable to integer overflows via unchecked user-provided weights, leading to an arbitrary out-of-bounds array write.
> --
> 
> commit 0014f07b693432d299630c4586e19c0cad5fbec9
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/ops-common: prevent >DAMON_MAX_SUBSCORE freq_subscore
> 
> When a zero sampling interval and a zero aggregation interval are
> online-committed, some regions could have nr_acceses values that are
> larger than damon_max_nr_accesses() for the remaining aggregation window.
> This commit attempts to fix out-of-bounds array accesses by applying an
> upper bound of DAMON_MAX_SUBSCORE to the freq_subscore.
> 
> Link: https://lore.kernel.org/20260621175849.91990-1-sj@kernel.org
> 
> > diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
> > index 5c93ef2bb8a97..8d516851a69e4 100644
> > --- a/mm/damon/ops-common.c
> > +++ b/mm/damon/ops-common.c
> > @@ -115,6 +115,9 @@ int damon_hot_score(struct damon_ctx *c, struct damon_region *r,
> >  
> >  	freq_subscore = r->nr_accesses * DAMON_MAX_SUBSCORE /
> >  		damon_max_nr_accesses(&c->attrs);
> > +	/* TODO: update monitoring results always to avoid this. */
> > +	if (freq_subscore > DAMON_MAX_SUBSCORE)
> > +		freq_subscore = DAMON_MAX_SUBSCORE;
> 
> [Severity: Critical]
> This is a pre-existing issue, but does the new bounds check fully prevent the
> out-of-bounds access if freq_subscore overflows and becomes negative?
> 
> Since r->nr_accesses is an unsigned int and DAMON_MAX_SUBSCORE is 100,
> r->nr_accesses * DAMON_MAX_SUBSCORE is evaluated as an unsigned int.
> 
> If a user previously configured a large aggregation interval, r->nr_accesses
> can grow up to ~22,000,000. If intervals are then online-committed to 0,
> damon_update_monitoring_results() skips scaling, leaving nr_accesses at
> its large value, while damon_max_nr_accesses() returns 1.
> 
> When r->nr_accesses * 100 exceeds 2^31 - 1, the division result assigned
> to the signed int freq_subscore overflows and becomes a large negative value.
> 
> The newly added check if (freq_subscore > DAMON_MAX_SUBSCORE) uses signed
> comparison, evaluates to false for negative values, and entirely bypasses
> the clamp.
> 
> This negative value propagates to the hotness score. Finally,
> damos_adjust_quota() implicitly casts this negative score to an unsigned int
> array index, resulting in a huge offset causing an arbitrary out-of-bounds
> memory write on c->regions_score_histogram.

Nice finding.  I will fix this in the next revision.

> 
> [Severity: Critical]
> This is a pre-existing issue, but can the hotness weighted average calculation
> overflow and bypass the DAMOS_MAX_SCORE bounds?
> 
> The user-configurable weights freq_weight (s->quota.weight_nr_accesses) and
> age_weight (s->quota.weight_age) are set via sysfs without upper bounds,
> allowing them to be arbitrarily large.
> 
> In damon_hot_score(), the hotness is computed in 32-bit unsigned arithmetic:
> 
> mm/damon/ops-common.c:damon_hot_score() {
> 	...
> 	hotness = (freq_weight * freq_subscore + age_weight * age_subscore);
> 	if (freq_weight + age_weight)
> 		hotness /= freq_weight + age_weight;
> 	...
> }
> 
> If the weights are chosen such that freq_weight + age_weight wraps around
> UINT_MAX to a small value (e.g., 2), the numerator also wraps.
> 
> When the wrapped numerator is cast to the signed int hotness and divided by
> the small unsigned sum, C's usual arithmetic conversions promote the negative
> int hotness to a huge unsigned int before division.
> 
> The final scaled result entirely evades the intended DAMOS_MAX_SCORE bound,
> returning an arbitrary out-of-bounds index that damos_adjust_quota() uses
> for an unsafe write on c->regions_score_histogram.

Nice finding, but not necessarily part of this series.  I will separately work
on this.


Thanks,
SJ

[...]