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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Medium] A failure during AIO initialization causes a NULL pointer dereference during cleanup in `perf_mmap__aio_munmap`.
--

commit 288781c48aca8cf278f6a1c8e4af510fc3cb0e4c
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

    perf mmap: Guard cpu__get_node() return in aio_bind()
    
    perf_mmap__aio_bind() passes the cpu__get_node() return value directly
    to an unsigned long variable (node_index).  When cpu__get_node() returns
    -1 for an unknown CPU, the implicit int-to-unsigned-long conversion
    sign-extends it to ULONG_MAX. Check the return value in a signed
    temporary before assigning to node_index, and skip the NUMA binding
    when the node is unknown.

> 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 isn't a bug introduced by this patch, but I noticed a potential NULL
pointer dereference during error cleanup if AIO initialization fails.

If memory allocation fails early in perf_mmap__aio_mmap():

tools/perf/util/mmap.c:perf_mmap__aio_mmap() {
    ...
	map->aio.nr_cblocks = mp->nr_cblocks;
	if (map->aio.nr_cblocks) {
		map->aio.aiocb = calloc(map->aio.nr_cblocks, sizeof(struct aiocb *));
		if (!map->aio.aiocb) {
			pr_debug2("failed to allocate aiocb for data buffer, error %m\n");
			return -1;
		}
    ...
}

The function returns an error, but leaves map->aio.nr_cblocks set to the
requested number of blocks while map->aio.data remains NULL.

This error bubbles up and triggers error recovery, which eventually calls
perf_mmap__aio_munmap():

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

Which loops based on nr_cblocks and passes the map to perf_mmap__aio_free():

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

Is it possible for map->aio.data to be NULL here? It seems evaluating this
array index (or calling zfree(&(map->aio.data[idx])) when libnuma is not
supported) could cause an immediate NULL pointer dereference.

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