Re: [RFC PATCH 16/27] mm/damon/sysfs: support REPORT_MADE type probe filter setup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] mm/damon: out-of-bounds and uninitialized access in
  damon_report_exist()
- [Medium] mm/damon: uninitialized report_type in damon_commit_filters()
--

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

mm/damon/sysfs: support REPORT_MADE type probe filter setup

This commit adds "report_made" to the DAMON sysfs probe filter type list
and copies the filter's report_type when setting up probe filters.

> diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
> index ca971cf1dcf6d..1c171af4590a8 100644
> --- a/mm/damon/sysfs.c
> +++ b/mm/damon/sysfs.c
> @@ -1016,6 +1016,10 @@ damon_sysfs_filter_type_names[] = {
>  		.type = DAMON_FILTER_TYPE_ACTIVE,
>  		.name = "active",
>  	},
> +	{
> +		.type = DAMON_FILTER_TYPE_REPORT_MADE,
> +		.name = "report_made",
> +	},
>  };
>  
>  static ssize_t type_show(struct kobject *kobj,

[Severity: High]
Can enabling DAMON_FILTER_TYPE_REPORT_MADE filters cause uninitialized and
out-of-bounds accesses on queue->reports[]?

In damon_submit_report(), each report is stored at queue->reports[tail] and
then queue->tail is incremented, so queue->tail points to the next slot to
write rather than the most recent entry:

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

When damon_pa_filter_match() calls damon_report_exist() for a report_made
filter, the loop reads queue->reports[idx] starting at idx = queue->tail
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;
    ...
}

While queue->nr_reports < queue->size, the first iteration reads the
unwritten slot at queue->reports[queue->tail] and the loop stops before
checking queue->reports[0].  Once queue->tail reaches queue->size,
damon_report_exist() reads out of bounds at queue->reports[queue->size],
and the next damon_submit_report() wraps only the local tail variable
before incrementing queue->tail past queue->size, causing subsequent
submissions and lookups to write and read out of bounds on the heap.

> @@ -2336,6 +2340,8 @@ static int damon_sysfs_set_filters(struct damon_probe *probe,
>  		} else if (filter->type == DAMON_FILTER_TYPE_HUGEPAGE_SIZE) {
>  			filter->range_min = sys_filter->range_min;
>  			filter->range_max = sys_filter->range_max;
> +		} else if (filter->type == DAMON_FILTER_TYPE_REPORT_MADE) {
> +			filter->report_type = sys_filter->report_type;
>  		}
>  		damon_add_filter(probe, filter);
>  	}

[Severity: Medium]
Does committing a new DAMON_FILTER_TYPE_REPORT_MADE filter to a running
context leave new_filter->report_type uninitialized?

When damon_sysfs_commit_input() commits the context built here via
damon_commit_ctx() -> __damon_commit_ctx() -> damon_commit_probes() ->
damon_commit_filters(), newly added filters are allocated with
damon_new_filter(), which does not initialize the union in struct
damon_filter, and the switch in damon_commit_filters() does not handle
DAMON_FILTER_TYPE_REPORT_MADE:

mm/damon/core.c:damon_commit_filters() {
    ...
		new_filter = damon_new_filter(src_filter->type,
				src_filter->matching, src_filter->allow);
		if (!new_filter)
			return -ENOMEM;
		switch (src_filter->type) {
		case DAMON_FILTER_TYPE_MEMCG:
			new_filter->memcg_id = src_filter->memcg_id;
			break;
		case DAMON_FILTER_TYPE_HUGEPAGE_SIZE:
			...
		default:
			break;
		}
    ...
}

As a result, damon_pa_filter_match() passes uninitialized slab memory in
new_filter->report_type to damon_report_exist().

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