Re: [PATCH v5 2/8] rtla/osnoise: Record IPI count in osnoise top

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Out-of-bounds read and potential unaligned access when parsing the dynamically sized cpumask trace field with CPU_AND.
- [Medium] Missing return value check for tep_get_field_val() leaves dst_cpu uninitialized, leading to out-of-bounds memory accesses.
--

commit ff03318823c0d1c72fb9d5969112afecabf91462
Author: Valentin Schneider [off-list ref]

rtla/osnoise: Record IPI count in osnoise top

This commit leverages the ipi_send_cpu and ipi_send_cpumask trace events to
record the count of IPIs sent to monitored CPUs. It adds these statistics to
the osnoise top output, enabled via the new --ipi command line option.

[ ... ]

[Severity: Medium]
If tep_get_field_val() fails to extract the field (for instance, due to a
trace format mismatch), the return value is ignored and dst_cpu is left
uninitialized.

Could this result in an out-of-bounds read inside the CPU_ISSET() macro, or
an out-of-bounds heap write when dst_cpu is passed to account_ipi() and used
as an index for data->cpu_data[dst_cpu]?

+
+	return 0;
+}
+
+static cpu_set_t cpumask_tmp_cpus;
+
+/*
+ * osnoise_ipi_cpumask_handler - this is the handler for broadcasted IPI events.
+ */
+static int
+osnoise_ipi_cpumask_handler(struct trace_seq *s, struct tep_record *record,
+			 struct tep_event *event, void *context)
+{
+	struct trace_instance *trace = context;
+	struct osnoise_tool *tool;
+	struct osnoise_params *params;
+	struct tep_format_field *field;
+	cpu_set_t *event_cpus;
+	int len;
+
+	tool = container_of(trace, struct osnoise_tool, trace);
+	params = to_osnoise_params(tool->params);
+
+	field = tep_find_field(event, "cpumask");
+	if (!field)
+		return 0;
+
+	event_cpus = tep_get_field_raw(s, event, "cpumask", record, &len, 1);
+	if (!event_cpus) {
+		err_msg("Failed to get cpumask field\n");
+		return 0;
+	}
+
+	CPU_AND(&cpumask_tmp_cpus, event_cpus, &params->common.monitored_cpus);

[Severity: Medium]
The event_cpus pointer returned by tep_get_field_raw() points to dynamically
sized raw bytes according to the len parameter. The glibc CPU_AND() macro
assumes both source arguments are full cpu_set_t objects and unconditionally
operates on 128 bytes.

Does this cause an out-of-bounds read and potential unaligned access when
parsing the trace field with CPU_AND()?

+
+	/*
+	 * Computing the mask weight is overkill but there is no leaner option
+	 * provided by glibc, e.g cpumask_first() or somesuch.
+	 */
+	if (CPU_COUNT(&cpumask_tmp_cpus)) {
+		for (int cpu = 0; cpu < nr_cpus; cpu++) {
+			if (CPU_ISSET(cpu, &cpumask_tmp_cpus))
+				account_ipi(tool, cpu);
+		}
+	}
+
+	return 0;
+}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902123942.695822-1-vschneid@redhat.com?part=2