Re: [RFC PATCH 14/27] mm/damon/paddr: support REPORT_MADE type probe filter

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] mm/damon: out-of-bounds and uninitialized read in
  damon_report_exist()
- [Medium] mm/damon: phys_addr_t truncation and unit mismatch in
  damon_pa_filter_match()
- [Medium] mm/damon: uninitialized filter->report_type in
  damon_pa_filter_match()
--

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

mm/damon/paddr: support REPORT_MADE type probe filter

This commit adds support for the DAMON_FILTER_TYPE_REPORT_MADE probe
filter in the physical address space operation set by querying
damon_report_exist() over the last sampling interval.

> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> index 5b773012a8764..04e95047f8cea 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
> @@ -140,9 +140,10 @@ static void damon_pa_prep_probes(struct damon_ctx *ctx, bool set_samples)
>  }
>  
>  static bool damon_pa_filter_match(struct damon_filter *filter,
> -		struct folio *folio)
> +		struct folio *folio, phys_addr_t pa, struct damon_ctx *ctx)
>  {
>  	bool matched = false;
> +	struct damon_addr_range addr_range;
>  
>  	switch (filter->type) {
>  	case DAMON_FILTER_TYPE_PGIDLE_UNSET:
> @@ -157,19 +158,27 @@ static bool damon_pa_filter_match(struct damon_filter *filter,
>  		else
>  			matched = damon_folio_young(folio) == false;
>  		break;
> +	case DAMON_FILTER_TYPE_REPORT_MADE:
> +		addr_range.start = pa;
> +		addr_range.end = pa + 1;

[Severity: Medium]
Can assigning pa and pa + 1 into addr_range truncate 64-bit physical
addresses and miss reports when ctx->addr_unit > 1?

struct damon_addr_range defines start and end as unsigned long, so on
32-bit PAE/LPAE systems where phys_addr_t is 64-bit, physical addresses
at or above 4 GiB lose their upper 32 bits (and pa + 1 wraps to 0 when
the low 32 bits are 0xffffffff).  In addition, when ctx->addr_unit > 1,
pa is aligned to ctx->addr_unit, so setting addr_range.end = pa + 1 only
checks the first byte of the sampled unit instead of [pa, pa +
ctx->addr_unit).

> +		matched = damon_report_exist(filter->report_type, &addr_range,
> +				jiffies -
> +				usecs_to_jiffies(ctx->attrs.sample_interval));
> +		break;

[Severity: High]
Does calling damon_report_exist() read uninitialized or out-of-bounds
elements from damon_report_queue.reports?

In damon_submit_report(), the report is written at queue->reports[tail]
and queue->tail is then incremented, so queue->tail points to the next
slot to be written and is never wrapped when it reaches queue->size:

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

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;
    ...
}

Because damon_report_exist() indexes queue->reports[idx] at queue->tail
before decrementing idx, it reads uninitialized heap memory on the first
iteration while 1 <= nr_reports < queue->size (and never inspects index
0), and reads out of bounds at queue->reports[queue->size] and beyond once
queue->size reports have been submitted.

[Severity: Medium]
Can filter->report_type be uninitialized here when a new
DAMON_FILTER_TYPE_REPORT_MADE filter is added via damon_commit_ctx()?

damon_new_filter() allocates struct damon_filter with kmalloc_obj()
without zeroing the union, and when damon_commit_filters() creates a new
filter it only copies union members for DAMON_FILTER_TYPE_MEMCG and
DAMON_FILTER_TYPE_HUGEPAGE_SIZE:

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:
			if (src_filter->range_max < src_filter->range_min) {
				damon_destroy_filter(new_filter);
				return -EINVAL;
			}
			new_filter->range_min = src_filter->range_min;
			new_filter->range_max = src_filter->range_max;
			break;
		default:
			break;
		}
    ...
}

As a result, new_filter->report_type retains uninitialized slab bytes
when damon_pa_filter_match() passes it to damon_report_exist().

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