Re: [PATCH 04/11] mm/damon/sysfs-schemes: implement fail_charge_{num,denom} files

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [Medium] The sysfs store functions for `fail_charge_num` and `fail_charge_denom` do not validate or bound the ratio, leading to potential integer overflow in the core quota tracking logic.
--

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

mm/damon/sysfs-schemes: implement fail_charge_{num,denom} files

This commit implements the user-space ABI for the DAMOS action failed region
quota-charge ratio setup. It adds sysfs files for fail_charge_num and
fail_charge_denom to allow reading and setting the numerator and
denominator of the failed regions charge ratio.

> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index 04746cbb3327..dee1f5788bea 100644
> --- a/mm/damon/sysfs-schemes.c
> +++ b/mm/damon/sysfs-schemes.c
[ ... ]
> +static ssize_t fail_charge_num_store(struct kobject *kobj,
> +		struct kobj_attribute *attr, const char *buf, size_t count)
> +{
> +	struct damon_sysfs_quotas *quotas = container_of(kobj,
> +			struct damon_sysfs_quotas, kobj);
> +	int err = kstrtouint(buf, 0, &quotas->fail_charge_num);
> +
> +	if (err)
> +		return -EINVAL;
> +	return count;
> +}

Can this unbounded input lead to an integer overflow in the core quota
tracking logic?

The values read via kstrtouint() are unconstrained up to UINT_MAX. Later, in
damos_charge_quota(), these values are passed to:

mult_frac(sz_region - sz_applied, quota->fail_charge_num,
          quota->fail_charge_denom)

If the region size in bytes is large (for example, a single DAMON region of
10 GB) and the numerator is set near UINT_MAX, their product could exceed
ULONG_MAX. For instance, 10GB * UINT_MAX is roughly 40 * 10^18, which is
larger than ULONG_MAX's 18.4 * 10^18.

Could this cause an overflow that wraps around to an anomalously small value,
effectively bypassing the administrator's configured quota limits?

> +
> +static ssize_t fail_charge_denom_show(struct kobject *kobj,
> +		struct kobj_attribute *attr, char *buf)
> +{
> +	struct damon_sysfs_quotas *quotas = container_of(kobj,
> +			struct damon_sysfs_quotas, kobj);
> +
> +	return sysfs_emit(buf, "%u\n", quotas->fail_charge_denom);
> +}
> +
> +static ssize_t fail_charge_denom_store(struct kobject *kobj,
> +		struct kobj_attribute *attr, const char *buf, size_t count)
> +{
> +	struct damon_sysfs_quotas *quotas = container_of(kobj,
> +			struct damon_sysfs_quotas, kobj);
> +	int err = kstrtouint(buf, 0, &quotas->fail_charge_denom);
> +
> +	if (err)
> +		return -EINVAL;
> +	return count;
> +}

Does the fail_charge_denom input also require bounding to prevent the same
overflow issues when passed into the quota tracking logic?

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