Re: [PATCH 1/9] perf mmap: Guard cpu__get_node() return in aio_bind()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Medium] NULL pointer dereference in `perf_mmap__aio_free()` when memory allocation fails in `perf_mmap__aio_mmap()`.
- [Medium] Sign-extension out-of-bounds write vulnerability in `perf_mmap__setup_affinity_mask`.
--

commit 2a2adfec0327c3db4323154825d9029d39b512eb
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf mmap: Guard cpu__get_node() return in aio_bind()

This commit fixes a sign-extension out-of-bounds write in perf_mmap__aio_bind()
by assigning the return of cpu__get_node() to a signed integer first.
If the return value is less than zero, the function safely returns early
instead of performing out-of-bounds operations.

> diff --git a/tools/perf/util/mmap.c b/tools/perf/util/mmap.c
> index b69f926d314b1..4404a99eee45f 100644
> --- a/tools/perf/util/mmap.c
> +++ b/tools/perf/util/mmap.c
> @@ -104,9 +104,15 @@ static int perf_mmap__aio_bind(struct mmap *map, int idx, struct perf_cpu cpu, i
>  	int err = 0;
>  
>  	if (affinity != PERF_AFFINITY_SYS && cpu__max_node() > 1) {
> +		int node;
> +
>  		data = map->aio.data[idx];
>  		mmap_len = mmap__mmap_len(map);
> -		node_index = cpu__get_node(cpu);
> +		node = cpu__get_node(cpu);
> +		/* -1 sign-extends to ULONG_MAX, wrapping bitmap_zalloc(0) and OOB __set_bit */
> +		if (node < 0)
> +			return 0;
> +		node_index = node;
>  		node_mask = bitmap_zalloc(node_index + 1);
>  		if (!node_mask) {
>  			pr_err("Failed to allocate node mask for mbind: error %m\n");

[Severity: Medium]
This is a pre-existing issue, but I noticed a similar sign-extension issue in
perf_mmap__setup_affinity_mask() that could cause an out-of-bounds write.

If map->core.cpu.cpu is -1 (for example, for a dummy CPU map created by perf
record -p <pid> --affinity cpu), wouldn't it be sign-extended to ULONG_MAX
when passed to __set_bit()?

tools/perf/util/mmap.c:perf_mmap__setup_affinity_mask() {
    ...
	if (mp->affinity == PERF_AFFINITY_NODE && cpu__max_node() > 1)
		build_node_mask(cpu__get_node(map->core.cpu), &map->affinity_mask);
	else if (mp->affinity == PERF_AFFINITY_CPU)
		__set_bit(map->core.cpu.cpu, map->affinity_mask.bits);
    ...
}


[Severity: Medium]
This is also a pre-existing issue, but while looking at the mmap code, I
noticed a potential NULL pointer dereference during error cleanup.

If memory allocation for map->aio.data fails in perf_mmap__aio_mmap(), the
function returns -1 while map->aio.nr_cblocks is already set.

During the cleanup sequence, perf_mmap__aio_munmap() unconditionally loops up
to map->aio.nr_cblocks:

tools/perf/util/mmap.c:perf_mmap__aio_munmap() {
    ...
	for (i = 0; i < map->aio.nr_cblocks; ++i)
		perf_mmap__aio_free(map, i);
    ...
}

Inside perf_mmap__aio_free(), the pointer offset is calculated and passed to
zfree():

tools/perf/util/mmap.c:perf_mmap__aio_free() {
	zfree(&(map->aio.data[idx]));
}

If map->aio.data is NULL, won't passing its evaluated offset to zfree()
cause a segmentation fault when __zfree() tries to free the invalid pointer?

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