Re: [PATCH v2 08/15] tools/rv: Implement BPF monitor loading and tracing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] bpf: path traversal allows loading arbitrary BPF objects
- [Medium] bpf: missing validation of required BTF fields leads to out-of-bounds read
- [Medium] bpf: missing data_sz bounds checks in handle_event leads to out-of-bounds read
- [Medium] bpf: type punning in read_field violates strict aliasing
--

commit 91a3dbdeaaa49f37b21710db6d56a6658ad6fe6b
Author: Gabriele Monaco <gmonaco@redhat.com>

tools/rv: Implement BPF monitor loading and tracing

This commit implements BPF monitor loading, execution, and optional tracing
functionality in the rv tool. It parses event and state names from BTF and
hooks into the ring buffer for output formatting.

> diff --git a/tools/verification/rv/src/bpf_monitor.c b/tools/verification/rv/src/bpf_monitor.c
> --- a/tools/verification/rv/src/bpf_monitor.c
> +++ b/tools/verification/rv/src/bpf_monitor.c

[ ... ]

> 		for (int j = 0; j < FIELD_MAX; j++) {
> 			if (strcmp(name, field_names[j]) == 0) {
> 				ctx->field_metadata[j].offset = offset;
> 				ctx->field_metadata[j].size = size;
> 				if (j == FIELD_ID)
> 					config.has_id = true;
> 				break;
> 			}
> 		}
> 	}
> 
> 	return 0;
> }

[Severity: Medium]
Is there a need to verify that all required fields were found after this
loop?

If the BTF data does not contain a required field (like comm), its offset
in ctx->field_metadata remains 0. Later, handle_event() relies on this
offset unconditionally, which would point to the start of the payload
and result in an out-of-bounds read.

[ ... ]

> static inline uint64_t read_field(uint64_t *entry, enum field_id id,
> 				  const uint8_t *raw,
> 				  const struct bpf_monitor_ctx *ctx)
> {
> 	const struct field *field = &ctx->field_metadata[id];
> 
> 	switch (field->size) {
> 	case 1:
> 		return entry[id] = *(const uint8_t *)(raw + field->offset);
> 	case 2:
> 		return entry[id] = *(const uint16_t *)(raw + field->offset);
> 	case 4:
> 		return entry[id] = *(const uint32_t *)(raw + field->offset);

[Severity: Medium]
Does casting a const uint8_t * byte pointer to larger scalar types like
uint32_t * violate strict aliasing rules?

The tools directory is compiled with -fstrict-aliasing enabled, so this type
punning can result in undefined behavior and miscompilation.

Also, is it possible for the offset + size to exceed the data_sz provided
by the ring buffer? read_field() dereferences memory based purely on
BTF offsets without validating that the access fits within the actual
payload size.

[ ... ]

> static int handle_event(void *ctx, void *data, size_t data_sz)
> {
> 	struct bpf_monitor_ctx *mon_ctx = ctx;
> 	const uint8_t *raw = data;

[Severity: Medium]
Is the data_sz parameter ignored in this function?

If a BPF program submits a truncated event to the ring buffer, the missing
bounds checks in this function and read_field() could lead to out-of-bounds
memory reads.

[ ... ]

> 	comm = (const char *)(raw + mon_ctx->field_metadata[FIELD_COMM].offset);
> 	read_field(entry, FIELD_CURR_STATE, raw, mon_ctx);
> 	read_field(entry, FIELD_EVENT, raw, mon_ctx);
> 
> 	printf("%16s-%-8"PRIu64" [%.3"PRIu64"] ", comm, entry[FIELD_PID], entry[FIELD_CPU]);

[Severity: Medium]
Could this printf() format specifier lead to unbounded memory reads?

Using %16s instead of %.16s allows printf to read indefinitely until it
encounters a null byte. If the string is not null-terminated or was absent,
this might read beyond data_sz.

[ ... ]

> static int find_bpf_file(const char *subdir, const char *name, char *path_out, size_t path_len)
> {
> 	char path[MAX_PATH];
> 
> 	for (int i = 0; bpf_base_paths[i][0]; i++) {
> 		size_t size = snprintf(path, sizeof(path), "%s/%s/%s.o",
> 				       bpf_base_paths[i], subdir, name);
> 
> 		if (size < MAX_PATH && access(path, R_OK) == 0) {

[Severity: High]
Can a malicious user supply a name containing path traversal characters like
../../../ to load arbitrary object files?

The user-controlled monitor_name is passed to find_bpf_file() and blindly
appended to the path string. If an administrator configures sudo to allow
running rv mon, an attacker could exploit this traversal to load their
own BPF program and gain arbitrary kernel code execution.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001152042.124445-1-gmonaco@redhat.com?part=8