From 50ee90630098039ce79486d6836e075b57fbe241 Mon Sep 17 00:00:00 2001 From: Giles Hutton Date: Wed, 5 Aug 2026 13:08:31 +0100 Subject: [PATCH 1/4] Tolerate mismatched parameter lengths in string extraction Event parameter data can have a recorded length that doesn't match the null-terminated string length. This has been observed on some kernels for both string and integer parameters (e.g. clone3 exe param_len=5 vs strnlen=1, clone flags param_len=597 vs expected 4). Use the full parameter length instead of throwing sinsp_exception, which propagated uncaught through sinsp::next() and crashed the collector via std::unexpected() -> abort(). --- userspace/libsinsp/event.h | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/userspace/libsinsp/event.h b/userspace/libsinsp/event.h index a9edec05e..1beac62cc 100644 --- a/userspace/libsinsp/event.h +++ b/userspace/libsinsp/event.h @@ -213,11 +213,14 @@ inline std::string_view get_event_param_as(const sinsp_evt_par } size_t string_len = strnlen(param_data, param_len); - // We expect the parameter to be exactly one null-terminated string + // We expect the parameter to be exactly one null-terminated string. + // When it doesn't match, use the full parameter length instead. + // Event parameter data can have a recorded length that doesn't match + // the null-terminated string length. This has been observed on some + // kernels for both string and integer parameters (e.g. clone3 exe + // param_len=5 vs strnlen=1, clone flags param_len=597 vs expected 4). if(param_len != string_len + 1) { - // By moving this error string building operation to a separate function - // the compiler is more likely to inline this entire function. - param.throw_invalid_len_error(string_len + 1); + string_len = param_len - 1; } return {param_data, string_len}; @@ -231,14 +234,11 @@ inline std::string get_event_param_as(const sinsp_evt_param& param) } size_t string_len = strnlen(param_data, param_len); - // We expect the parameter to be exactly one null-terminated string if(param_len != string_len + 1) { - // By moving this error string building operation to a separate function - // the compiler is more likely to inline this entire function. - param.throw_invalid_len_error(string_len + 1); + string_len = param_len - 1; } - return std::string(param_data); + return std::string(param_data, string_len); } template<> From 713d313c67b9ce66690f39e31b9ec2e936eb0ccd Mon Sep 17 00:00:00 2001 From: Giles Hutton Date: Tue, 11 Aug 2026 11:07:00 +0100 Subject: [PATCH 2/4] Add enhanced diagnostics for parameter length mismatch Revert the param_len-1 workaround and restore the original throwing behaviour, but add comprehensive diagnostic logging before the throw to identify the root cause of event parameter corruption seen on Fedora CoreOS (kernel 7.1.x). When a string parameter length doesn't match strnlen+1, the error handler now dumps: - Event header fields (nparams from both header and table, event_len, event_type, header size) - The full raw parameter length array from the event - Hex dump of the event header + length array - First 128 bytes of the parameter data region This will show whether the corruption is caused by an nparams mismatch, shifted length array, or something else entirely. --- userspace/libsinsp/event.cpp | 63 ++++++++++++++++++++++++++++++++++++ userspace/libsinsp/event.h | 18 +++++------ 2 files changed, 72 insertions(+), 9 deletions(-) diff --git a/userspace/libsinsp/event.cpp b/userspace/libsinsp/event.cpp index de564d756..f5bf86715 100644 --- a/userspace/libsinsp/event.cpp +++ b/userspace/libsinsp/event.cpp @@ -1730,6 +1730,69 @@ void sinsp_evt_param::throw_invalid_len_error(size_t requested_length) const { "parameter raw data: \n" + buffer_to_multiline_hex(param_data, param_len), sinsp_logger::SEV_ERROR); + // Enhanced diagnostics: dump raw event structure to identify the + // root cause of parameter corruption (ROX-33614 investigation). + const scap_evt *raw = m_evt->get_scap_evt(); + const ppm_event_info *evtinfo = m_evt->get_info(); + if(raw && evtinfo) { + std::stringstream diag; + diag << "event diagnostics:" + << " hdr_nparams=" << raw->nparams + << " table_nparams=" << evtinfo->nparams + << " event_len=" << raw->len + << " event_type=" << raw->type + << " hdr_size=" << sizeof(struct ppm_evt_hdr); + libsinsp_logger()->log(diag.str(), sinsp_logger::SEV_ERROR); + + // Dump the length array from the raw event. + // Layout: [ppm_evt_hdr][len0][len1]...[lenN][data0][data1]... + // Each length entry is uint16_t (non-large) or uint32_t (large). + const char *evt_base = reinterpret_cast(raw); + const char *len_array = evt_base + sizeof(struct ppm_evt_hdr); + bool is_large = (evtinfo->flags & EF_LARGE_PAYLOAD) != 0; + uint32_t len_entry_size = is_large ? sizeof(uint32_t) : sizeof(uint16_t); + uint32_t len_array_bytes = raw->nparams * len_entry_size; + + // Dump all param lengths from the raw length array. + std::stringstream lens; + lens << "raw param lengths (" << (is_large ? "large" : "u16") << "):"; + for(uint32_t i = 0; i < raw->nparams && i < PPM_MAX_EVENT_PARAMS; i++) { + uint32_t plen = 0; + if(is_large) { + memcpy(&plen, len_array + i * sizeof(uint32_t), sizeof(uint32_t)); + } else { + uint16_t plen16 = 0; + memcpy(&plen16, len_array + i * sizeof(uint16_t), sizeof(uint16_t)); + plen = plen16; + } + lens << " [" << i << "]=" << plen; + } + libsinsp_logger()->log(lens.str(), sinsp_logger::SEV_ERROR); + + // Dump the raw event header + length array as hex. + size_t hdr_and_lens = sizeof(struct ppm_evt_hdr) + len_array_bytes; + size_t dump_len = std::min(hdr_and_lens, (size_t)256); + libsinsp_logger()->log( + "raw header+lengths (" + std::to_string(dump_len) + " bytes):\n" + + buffer_to_multiline_hex(evt_base, dump_len), + sinsp_logger::SEV_ERROR); + + // Dump the first 128 bytes of the param data region. + const char *data_region = len_array + len_array_bytes; + size_t data_avail = 0; + if(raw->len > hdr_and_lens) { + data_avail = raw->len - hdr_and_lens; + } + size_t data_dump = std::min(data_avail, (size_t)128); + if(data_dump > 0) { + libsinsp_logger()->log( + "param data region (first " + std::to_string(data_dump) + + " of " + std::to_string(data_avail) + " bytes):\n" + + buffer_to_multiline_hex(data_region, data_dump), + sinsp_logger::SEV_ERROR); + } + } + throw sinsp_exception(error_string); } diff --git a/userspace/libsinsp/event.h b/userspace/libsinsp/event.h index 1beac62cc..a9edec05e 100644 --- a/userspace/libsinsp/event.h +++ b/userspace/libsinsp/event.h @@ -213,14 +213,11 @@ inline std::string_view get_event_param_as(const sinsp_evt_par } size_t string_len = strnlen(param_data, param_len); - // We expect the parameter to be exactly one null-terminated string. - // When it doesn't match, use the full parameter length instead. - // Event parameter data can have a recorded length that doesn't match - // the null-terminated string length. This has been observed on some - // kernels for both string and integer parameters (e.g. clone3 exe - // param_len=5 vs strnlen=1, clone flags param_len=597 vs expected 4). + // We expect the parameter to be exactly one null-terminated string if(param_len != string_len + 1) { - string_len = param_len - 1; + // By moving this error string building operation to a separate function + // the compiler is more likely to inline this entire function. + param.throw_invalid_len_error(string_len + 1); } return {param_data, string_len}; @@ -234,11 +231,14 @@ inline std::string get_event_param_as(const sinsp_evt_param& param) } size_t string_len = strnlen(param_data, param_len); + // We expect the parameter to be exactly one null-terminated string if(param_len != string_len + 1) { - string_len = param_len - 1; + // By moving this error string building operation to a separate function + // the compiler is more likely to inline this entire function. + param.throw_invalid_len_error(string_len + 1); } - return std::string(param_data, string_len); + return std::string(param_data); } template<> From ec3747cd426d8f477e3ef538e0b3cbafc41f2fb9 Mon Sep 17 00:00:00 2001 From: Giles Hutton Date: Wed, 12 Aug 2026 10:17:53 +0100 Subject: [PATCH 3/4] Use a LRU hash for auxmaps to avoid race condition falcosecurity/libs#2719 --- driver/modern_bpf/helpers/base/maps_getters.h | 33 +++++++++++++++++-- driver/modern_bpf/maps/maps.h | 31 +++++++++++++++-- userspace/libpman/src/maps.c | 14 ++++++-- 3 files changed, 71 insertions(+), 7 deletions(-) diff --git a/driver/modern_bpf/helpers/base/maps_getters.h b/driver/modern_bpf/helpers/base/maps_getters.h index 6e4e0afc0..d37c7e6d1 100644 --- a/driver/modern_bpf/helpers/base/maps_getters.h +++ b/driver/modern_bpf/helpers/base/maps_getters.h @@ -184,8 +184,37 @@ static __always_inline uint16_t maps__get_ppm_sc(uint16_t syscall_id) { /*=============================== AUXILIARY MAPS ===========================*/ static __always_inline struct auxiliary_map *maps__get_auxiliary_map() { - uint32_t cpu_id = (uint32_t)bpf_get_smp_processor_id(); - return (struct auxiliary_map *)bpf_map_lookup_elem(&auxiliary_maps, &cpu_id); + /* Key by `pid_tgid` rather than CPU: BPF programs are preemptible on + * kernels >= 5.11, so a per-CPU scratch buffer can be clobbered by + * another program scheduled on the same CPU. A task only ever runs on + * one CPU at a time, so `pid_tgid` uniquely identifies an in-flight + * event build (stable across the tail-call chain). See + * falcosecurity/libs#2719. + */ + uint64_t pid_tgid = bpf_get_current_pid_tgid(); + struct auxiliary_map *auxmap = + (struct auxiliary_map *)bpf_map_lookup_elem(&auxiliary_maps, &pid_tgid); + if(auxmap) { + return auxmap; + } + + /* First event for this task (or the entry was evicted from the LRU): we + * need to create the entry. `bpf_map_update_elem` requires a value to + * copy from; use the single-element init template (a 128 KB value cannot + * live on the BPF stack). The auxmap does not need to be zeroed (the + * header, payload_pos and lengths_pos are set explicitly by + * auxmap__preload_event_header, and param data is written before it is + * read), so the template contents are irrelevant. */ + uint32_t zero = 0; + struct auxiliary_map *init = + (struct auxiliary_map *)bpf_map_lookup_elem(&auxiliary_map_init, &zero); + if(!init) { + return NULL; + } + if(bpf_map_update_elem(&auxiliary_maps, &pid_tgid, init, BPF_ANY)) { + return NULL; + } + return (struct auxiliary_map *)bpf_map_lookup_elem(&auxiliary_maps, &pid_tgid); } /*=============================== AUXILIARY MAPS ===========================*/ diff --git a/driver/modern_bpf/maps/maps.h b/driver/modern_bpf/maps/maps.h index 3ec27b865..ca4393ec8 100644 --- a/driver/modern_bpf/maps/maps.h +++ b/driver/modern_bpf/maps/maps.h @@ -144,15 +144,40 @@ struct { */ /** - * @brief For every CPU on the system we have an auxiliary - * map where the event is temporally saved before being + * @brief Auxiliary map where the event is temporally saved before being * pushed in the ringbuffer. + * + * This is keyed by `pid_tgid` (not CPU) and backed by an LRU hash to avoid a + * data-corruption race on preemptible kernels (Linux >= 5.11). BPF programs run + * with `migrate_disable()` but are preemptible: a per-CPU scratch buffer can be + * clobbered if a program is preempted mid-write and another program runs on the + * same CPU. Keying by `pid_tgid` gives each in-flight task its own scratch + * entry, since a task can only run on one CPU at a time. See + * falcosecurity/libs#2719. + */ +struct { + __uint(type, BPF_MAP_TYPE_LRU_HASH); + __type(key, uint64_t); + __type(value, struct auxiliary_map); +} auxiliary_maps __weak SEC(".maps"); + +/** + * @brief Single-element template used to initialise a new `auxiliary_maps` + * entry. + * + * `bpf_map_update_elem` on the LRU hash requires a source value to copy from + * when creating a new entry for a task. We cannot build a 128 KB value on the + * BPF stack (512 byte limit), so we keep one element to copy from. Its contents + * are irrelevant (the auxmap is always written before it is read), so it is + * never explicitly initialised. A regular ARRAY is used because a + * BPF_MAP_TYPE_PERCPU_ARRAY element is limited to 32 KB. */ struct { __uint(type, BPF_MAP_TYPE_ARRAY); + __uint(max_entries, 1); __type(key, uint32_t); __type(value, struct auxiliary_map); -} auxiliary_maps __weak SEC(".maps"); +} auxiliary_map_init __weak SEC(".maps"); /** * @brief For every CPU on the system we have a counter diff --git a/userspace/libpman/src/maps.c b/userspace/libpman/src/maps.c index e89186ae8..5578e65cb 100644 --- a/userspace/libpman/src/maps.c +++ b/userspace/libpman/src/maps.c @@ -377,8 +377,18 @@ int pman_mark_single_64bit_syscall(int syscall_id, bool interesting) { } static int size_auxiliary_maps() { - /* We always allocate auxiliary maps from all the CPUs, even if some of them are not online. */ - if(bpf_map__set_max_entries(g_state.skel->maps.auxiliary_maps, g_state.n_possible_cpus)) { + /* `auxiliary_maps` is an LRU hash keyed by `pid_tgid`, holding one scratch + * entry per task that is currently building an event. Only one task runs + * per CPU at a time, but a task can be preempted mid-build while another + * runs on the same CPU, so we need headroom above the CPU count. We size + * it at 4x the possible CPUs (with a sensible floor) so that LRU eviction + * of an in-flight entry is effectively impossible. If an entry were ever + * evicted mid-build the event would be dropped, never corrupted. */ + uint32_t aux_entries = g_state.n_possible_cpus * 4; + if(aux_entries < 128) { + aux_entries = 128; + } + if(bpf_map__set_max_entries(g_state.skel->maps.auxiliary_maps, aux_entries)) { pman_print_error("unable to set max entries for 'auxiliary_maps'"); return errno; } From a4c9d16f029df02ab9013e90dce103e9243667d6 Mon Sep 17 00:00:00 2001 From: Giles Hutton Date: Wed, 12 Aug 2026 15:33:21 +0100 Subject: [PATCH 4/4] Improve housekeeping for LRU map --- driver/modern_bpf/helpers/base/maps_getters.h | 14 ++++++++++++++ .../helpers/store/auxmap_store_params.h | 5 +++++ userspace/libpman/src/maps.c | 18 +++++++++--------- 3 files changed, 28 insertions(+), 9 deletions(-) diff --git a/driver/modern_bpf/helpers/base/maps_getters.h b/driver/modern_bpf/helpers/base/maps_getters.h index d37c7e6d1..d9aa534c0 100644 --- a/driver/modern_bpf/helpers/base/maps_getters.h +++ b/driver/modern_bpf/helpers/base/maps_getters.h @@ -217,6 +217,20 @@ static __always_inline struct auxiliary_map *maps__get_auxiliary_map() { return (struct auxiliary_map *)bpf_map_lookup_elem(&auxiliary_maps, &pid_tgid); } +/** + * @brief Release the current task's auxiliary map entry. + * + * Called once an event has been submitted (best effort). Freeing the entry + * immediately keeps the `auxiliary_maps` LRU hash populated only with events + * that are currently being built (bounded by the CPU count), rather than one + * entry per task that has ever produced an event. Entries left behind by + * abandoned events (e.g. a failed tail call) are reclaimed by LRU eviction. + */ +static __always_inline void maps__release_auxiliary_map() { + uint64_t pid_tgid = bpf_get_current_pid_tgid(); + bpf_map_delete_elem(&auxiliary_maps, &pid_tgid); +} + /*=============================== AUXILIARY MAPS ===========================*/ /*=============================== COUNTER MAPS ===========================*/ diff --git a/driver/modern_bpf/helpers/store/auxmap_store_params.h b/driver/modern_bpf/helpers/store/auxmap_store_params.h index a74bc89b9..daaaa2061 100644 --- a/driver/modern_bpf/helpers/store/auxmap_store_params.h +++ b/driver/modern_bpf/helpers/store/auxmap_store_params.h @@ -149,6 +149,11 @@ static __always_inline void auxmap__submit_event(struct auxiliary_map *auxmap) { counter->n_drops_buffer++; compute_event_types_stats(auxmap->event_type, counter); } + + /* The event has been handed to the ring buffer (or dropped); release this + * task's auxiliary map entry so the LRU hash is not filled with one entry + * per task that has ever produced an event. See falcosecurity/libs#2719. */ + maps__release_auxiliary_map(); return; } diff --git a/userspace/libpman/src/maps.c b/userspace/libpman/src/maps.c index 5578e65cb..c5fe69353 100644 --- a/userspace/libpman/src/maps.c +++ b/userspace/libpman/src/maps.c @@ -378,15 +378,15 @@ int pman_mark_single_64bit_syscall(int syscall_id, bool interesting) { static int size_auxiliary_maps() { /* `auxiliary_maps` is an LRU hash keyed by `pid_tgid`, holding one scratch - * entry per task that is currently building an event. Only one task runs - * per CPU at a time, but a task can be preempted mid-build while another - * runs on the same CPU, so we need headroom above the CPU count. We size - * it at 4x the possible CPUs (with a sensible floor) so that LRU eviction - * of an in-flight entry is effectively impossible. If an entry were ever - * evicted mid-build the event would be dropped, never corrupted. */ - uint32_t aux_entries = g_state.n_possible_cpus * 4; - if(aux_entries < 128) { - aux_entries = 128; + * entry per event currently being built. Entries are released as soon as an + * event is submitted, so at any instant the number of live entries is + * bounded by the number of tasks building an event concurrently (at most + * one per CPU). We size it at 2x the possible CPUs (with a small floor) to + * leave headroom; the LRU reclaims any entries left behind by abandoned + * events. */ + uint32_t aux_entries = g_state.n_possible_cpus * 2; + if(aux_entries < 16) { + aux_entries = 16; } if(bpf_map__set_max_entries(g_state.skel->maps.auxiliary_maps, aux_entries)) { pman_print_error("unable to set max entries for 'auxiliary_maps'");