Re: [PATCH v6 11/14] gtrace: Add perf driver for tracing using perf tool

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] gtrace: NULL pointer dereference of event->owner in gtrace_setup_aux
- [High] gtrace: deferred vunmap creates dangling page table entries mapping freed pages
- [High] gtrace: unbounded copyto_auxbuf overwrites unconsumed perf ring buffer data
- [Medium] gtrace: sink driver format flags overwritten in gtrace_event_stop
- [Medium] gtrace: perf_report_aux_output_id reports logical CPU instead of hardware trace ID
- [Medium] kconfig: GTRACE selects PERF_EVENTS without inheriting HAVE_PERF_EVENTS dependency
--

commit 94baa393278755b8b8c03ef3fec39f40b4499dbb
Author: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>

gtrace: Add perf driver for tracing using perf tool

Add perf driver for the gtrace similar to ARM Coresight and Hisilicon
PTT drivers. The driver adds 'gtrace' event descriptor which can be used
by the perf tool to record the trace data.

> diff --git a/drivers/hwtracing/gtrace/Kconfig b/drivers/hwtracing/gtrace/Kconfig
> index de4a3b5335911..e5b62d65e2fdb 100644
> --- a/drivers/hwtracing/gtrace/Kconfig
> +++ b/drivers/hwtracing/gtrace/Kconfig
> @@ -3,6 +3,7 @@
>  menuconfig GTRACE
>  	tristate "Generic Hardware Trace Support"
>  	depends on OF
> +	select PERF_EVENTS

[Severity: Medium]
Will this cause unmet direct dependency warnings or build failures?

The PERF_EVENTS config symbol depends on HAVE_PERF_EVENTS. By using
select PERF_EVENTS unconditionally here, it forces the configuration on
even on architectures that support OF but do not have HAVE_PERF_EVENTS.

>  	help
>  	  This framework provides an architecture-neutral kernel interface
>  	  for hardware trace drivers. It builds a topological view of the

[ ... ]

> diff --git a/drivers/hwtracing/gtrace/gtrace-perf.c b/drivers/hwtracing/gtrace/gtrace-perf.c
> new file mode 100644
> index 0000000000000..928f0405c6c17
> --- /dev/null
> +++ b/drivers/hwtracing/gtrace/gtrace-perf.c

[ ... ]

> +static void gtrace_free_aux(void *data)
> +{
> +	struct gtrace_event_data *event_data = data;
> +
> +	queue_work(gtrace_wq, &event_data->work);
> +}

[Severity: High]
Does this defer the vunmap() operation too late?

The perf core calls the pmu->free_aux callback synchronously, and immediately
after it returns, calls __free_page() on the underlying physical pages.

Because gtrace_free_aux() defers the cleanup to a workqueue, the physical
pages are freed while valid kernel vmap page table entries still point to
them.

This creates a window where memory corruption could occur if the driver or
hardware accesses the vmap address before the workqueue executes vunmap().

> +
> +static struct gtrace_path **gtrace_event_cpu_path_ptr(struct gtrace_event_data *data,
> +							int cpu)
> +{
> +	return per_cpu_ptr(data->path, cpu);
> +}
> +
> +static void free_event_data(struct work_struct *work)
> +{
> +	struct gtrace_event_data *event_data;
> +	struct gtrace_path *path;
> +	cpumask_t *mask;
> +	int cpu;
> +
> +	event_data = container_of(work, struct gtrace_event_data, work);
> +	mask = &event_data->mask;
> +	for_each_cpu(cpu, mask) {
> +		path = *gtrace_event_cpu_path_ptr(event_data, cpu);
> +		gtrace_destroy_path(path);
> +	}
> +	if (event_data->buf.base)
> +		vunmap(event_data->buf.base);

[Severity: High]
As mentioned above, executing vunmap() asynchronously here creates a race
where the underlying pages have already been returned to the page allocator.

> +	free_percpu(event_data->path);
> +	kfree(event_data);
> +}
> +
> +static void *gtrace_setup_aux(struct perf_event *event, void **pages,
> +			       int nr_pages, bool overwrite)
> +{

[ ... ]

> +	/*
> +	 * Create the path for each CPU in the mask. In case of any failure skip the CPU
> +	 */
> +	for_each_cpu(cpu, mask) {
> +		struct gtrace_component *src;
> +		struct gtrace_path *path;
> +
> +		src = gtrace_cpu_source(cpu);
> +		if (!src) {
> +			cpumask_clear_cpu(cpu, mask);
> +			continue;
> +		}
> +
> +		path = gtrace_create_path(src, NULL, GTRACE_COMPONENT_MODE_PERF);
> +		if (IS_ERR(path)) {
> +			cpumask_clear_cpu(cpu, mask);
> +			continue;
> +		}
> +
> +		path->owner = task_pid_nr(event->owner);

[Severity: Critical]
Can event->owner be NULL or TASK_TOMBSTONE here?

If an unprivileged user creates a trace event, forks, and the parent exits,
event->owner is cleared to NULL via perf_event_exit_task(). If the child
then mmaps the inherited file descriptor, gtrace_setup_aux() unconditionally
dereferences event->owner via task_pid_nr().

Additionally, kernel counters set event->owner to TASK_TOMBSTONE
((void *)-1L), which would also cause a panic.

> +		*gtrace_event_cpu_path_ptr(event_data, cpu) = path;
> +	}

[ ... ]

> +static void gtrace_event_start(struct perf_event *event, int flags)
> +{

[ ... ]

> +	/*
> +	 * output cpu / trace ID in perf record, once for the lifetime
> +	 * of the event.
> +	 */
> +	if (!cpumask_test_cpu(cpu, &event_data->aux_hwid_done)) {
> +		cpumask_set_cpu(cpu, &event_data->aux_hwid_done);
> +		perf_report_aux_output_id(event, cpu);

[Severity: Medium]
Should this report the hardware trace ID instead of the logical CPU index?

If the hardware trace stream embeds the real hardware trace ID, passing the
logical CPU index here means the decoder in userspace won't be able to match
packets to CPUs, breaking trace decoding.

Could this pass path->trace_id instead of the local cpu variable?

> +	}
> +
> +out:
> +	/* Tell the perf core the event is alive */
> +	event->hw.state = 0;
> +	ctxt->event_data = event_data;
> +	return;
> +fail_end:
> +	perf_aux_output_end(handle, 0);
> +fail:
> +	event->hw.state = PERF_HES_STOPPED;
> +}
> +
> +static void gtrace_event_stop(struct perf_event *event, int mode)
> +{
> +	struct gtrace_ctxt *ctxt = this_cpu_ptr(&gtrace_ctxt);
> +	struct perf_output_handle *handle = &ctxt->handle;
> +	u64 format = PERF_AUX_FLAG_GTRACE_FORMAT_UNKNOWN;
> +	struct gtrace_event_data *event_data;
> +	int ret, cpu = smp_processor_id();
> +	struct gtrace_path *path;
> +	size_t size = 0;

[ ... ]

> +	event->hw.state = PERF_HES_STOPPED;
> +	if (handle->event && (mode & PERF_EF_UPDATE)) {
> +		if (WARN_ON_ONCE(handle->event != event))
> +			return;
> +		ret = gtrace_path_copyto_auxbuf(path, &event_data->buf, &size, &format);

[Severity: High]
Does this allow an unbounded copy that can corrupt unread ring buffer data?

The gtrace_path_copyto_auxbuf() function is only passed event_data->buf,
which only knows the total buffer length, ignoring the handle->size limit
indicating how much contiguous space is safely writable.

> +		WARN_ON_ONCE(ret);
> +		format = gtrace_path_aux_format(path);

[Severity: Medium]
Are the format flags returned by the sink driver being discarded?

The gtrace_path_copyto_auxbuf() call allows the sink driver to set format
flags in the format parameter (like truncation or formatting details).

Immediately following this, the format variable is unconditionally
overwritten with gtrace_path_aux_format(), discarding the driver's flags
rather than OR'ing them.

> +		if (READ_ONCE(handle->event)) {
> +			/* Tag the AUX data with the source's trace format. */
> +			perf_aux_output_flag(handle, format);
> +			if (size > handle->size) {
> +				size = handle->size;
> +				perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> +			}

[Severity: High]
The check for size > handle->size happens here, after the unbounded memory 
copy has already been performed.

> +			perf_aux_output_end(handle, size);
> +		} else
> +			WARN_ON(size);
> +	}
> +
> +	return;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001052243.3042627-1-mayuresh.chitale@oss.qualcomm.com?part=11