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

[SJ Park <sj@kernel.org>]

On Sun, 15 Nov 2026 20:43:17 +0000 sashiko-bot@kernel.org wrote:

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

Yes, I will fix those in the earlier patches.

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


Thanks,
SJ