Re: [RFC PATCH 13/27] mm/damon/core: introduce damon_report_exist()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] mm/damon/core: out-of-bounds and uninitialized read in
  damon_report_exist()
--

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

mm/damon/core: introduce damon_report_exist()

This commit adds damon_report_exist() to search damon_report_queue for a
report matching a given type, address range, and minimum age in jiffies.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index c02c7fbc1d864..c60a6504d0a15 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -2386,6 +2386,52 @@ int damon_submit_report(struct damon_report *report)
>  	return 0;
>  }
>  
> +/**
> + * damon_report_exist() - Find if a report of a given condition exists.
> + *
> + * @report_type:	Type of the report.
> + * @addr_range:		Address range of the report.
> + * @jiffies_old:	Oldest jiffies to find the report for.
> + *
> + * Returns true if the report is found, false otherwise.
> + */
> +bool damon_report_exist(enum damon_report_type report_type,
> +		struct damon_addr_range *addr_range, unsigned long jiffies_old)
> +{
> +	struct damon_report_queue *queue;
> +	int lock_old_val;
> +	unsigned int idx, nr_read;
> +	struct damon_report *report;
> +	bool found = false;
> +
> +	queue = &damon_report_queue;
> +	lock_old_val = atomic_cmpxchg(&queue->lock, 0, 1);
> +
> +	if (lock_old_val)
> +		return false;
> +	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;

[Severity: High]
Can indexing queue->reports[idx] before decrementing and wrapping idx read
uninitialized or out-of-bounds memory?

In damon_submit_report(), queue->tail is post-incremented after writing
queue->reports[tail]:

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

Because queue->tail points one slot past the newest report, the first
iteration of damon_report_exist() reads the unwritten slot at
queue->reports[queue->tail] (which is allocated with kmalloc_objs() in
damon_init() without zeroing) and never visits queue->reports[0].  Once
queue->size (1000) reports have been submitted, queue->tail reaches 1000
and the first iteration reads queue->reports[1000], one element past the
end of the array.  Also, when damon_submit_report() wraps local tail to 0,
queue->tail itself is not wrapped and increments past queue->size on
subsequent submissions.

Should idx be decremented (wrapping from 0 to queue->size - 1) before
indexing queue->reports[idx], with damon_submit_report() updating
queue->tail = tail + 1?

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