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 <[email protected]> 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/[email protected]?part=8
