Re: [RFC PATCH 23/27] mm/damon/sysfs: implement submit command for report producer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] mm/damon: OOB write in damon_submit_report() and read in
  damon_report_exist()
--

commit 2f4ed587e778c0d51407454ba3a19b21c1bc10c0
Author: SJ Park <sj@kernel.org>

mm/damon/sysfs: implement submit command for report producer

This commit adds a "submit" command to producer_state_store() so userspace
can submit manual reports via damon_submit_report().

> diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
> index 5f9bb1955dfd5..0049aac6ab974 100644
> --- a/mm/damon/sysfs.c
> +++ b/mm/damon/sysfs.c
> @@ -3107,13 +3107,26 @@ static ssize_t producer_state_store(struct kobject *kobj,
>  {
>  	struct damon_sysfs_producer *producer = container_of(kobj,
>  			struct damon_sysfs_producer, kobj);
> +	struct damon_report report;
> +	int err;
>  
> -	if (sysfs_streq(buf, "on"))
> +	if (sysfs_streq(buf, "on")) {
>  		producer->enabled = true;
> -	else if (sysfs_streq(buf, "off"))
> +	} else if (sysfs_streq(buf, "off")) {
>  		producer->enabled = false;
> -	else
> +	} else if (sysfs_streq(buf, "submit")) {
> +		if (!producer->enabled)
> +			return -EINVAL;
> +		if (producer->report_type != DAMON_REPORT_TYPE_USER_REPORT)
> +			return -EINVAL;
> +		report.type = DAMON_REPORT_TYPE_USER_REPORT;
> +		report.addr_range = producer->addr_range;
> +		err = damon_submit_report(&report);

[Severity: High]
Can repeated "submit" writes overflow damon_report_queue.reports[] in
damon_submit_report() and read out of bounds in damon_report_exist()?

mm/damon/core.c:damon_submit_report() {
	...
	tail = queue->tail;
	if (tail == queue->size)
		tail = 0;
	queue->reports[tail] = *report;
	queue->tail++;
	...
}

When queue->tail reaches queue->size (1000), damon_submit_report() wraps
only the local variable tail to 0 and still increments queue->tail to
1001 instead of setting queue->tail = tail + 1.  On the next submission,
tail is 1001, the equality check against queue->size is false, and
queue->reports[tail] writes past the end of the 1000-element heap array.

In addition, damon_report_exist() starts its backward scan at
idx = queue->tail (the next free slot) and dereferences
queue->reports[idx] before decrementing idx:

mm/damon/core.c:damon_report_exist() {
	...
	idx = queue->tail;
	for (nr_read = 0; nr_read < queue->nr_reports; nr_read++) {
		report = &queue->reports[idx];
		if (idx)
			idx--;
		else
			idx = queue->size - 1;
	...
}

Before the ring wraps, the first iteration reads uninitialized memory at
queue->reports[queue->tail] while the loop stops before reaching
queue->reports[0], missing a single submitted report entirely; once
1000 reports have been submitted (queue->tail == queue->size), the first
iteration reads out of bounds at queue->reports[1000].

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