From ed66eccba5d530df82f94768f6c36cdf019d3948 Mon Sep 17 00:00:00 2001 From: Anastasios Papagiannis Date: Wed, 8 Mar 2023 11:24:07 +0000 Subject: [PATCH] bugfix: matchBinaries in multiple selectors Using the following tracing policy: kprobes: - call: "fd_install" syscall: false args: - index: 0 type: int - index: 1 type: "file" selectors: - matchBinaries: - operator: "In" values: - "/usr/bin/cat" matchArgs: - index: 1 operator: "Equal" values: - "/etc/passwd" - matchBinaries: - operator: "In" values: - "/usr/bin/tail" matchArgs: - index: 1 operator: "Equal" values: - "/etc/shadow" we expect to get events if: [ binary == /usr/bin/cat AND arg1 == /etc/passwd ] OR [ binary == /usr/bin/tail AND arg1 == /etc/shadow ] Using the previous tracing policy and running: /usr/bin/cat /etc/passwd && /usr/bin/cat /etc/shadow && /usr/bin/tail /etc/passwd && /usr/bin/tail /etc/shadow We get events for all of these. The issue is that in https://github.com/cilium/tetragon/pull/686 we use a single map for all matchBinaries selectors. That makes the previous tracing policy to behave as: [ (binary == /usr/bin/cat OR binary == /usr/bin/tail) AND arg1 == /etc/passwd ] OR [ (binary == /usr/bin/cat OR binary == /usr/bin/tail) AND arg1 == /etc/shadow ] This patch fixes that issues. We move from a BPF_MAP_TYPE_HASH to a BPF_MAP_TYPE_HASH_OF_MAPS. We index the outter map based on selector ID and the inner map is exactly the same as the one we used in https://github.com/cilium/tetragon/pull/686. Based on the previous tracing policy, the user side will generate the names_map and sel_names_map. names_map is shared among all selectors that have matchBinaries. Each distinct binary name will appear in the names_map with a unique value per entry (> 0). The contents of the names_map for the previous tracing policy will be: names_map["/usr/bin/cat"] = 1 names_map["/usr/bin/tail"] = 2 Whenever we have an execve event, we check the execve binary name and if is that inside names_map, we set execve_map_value->binary to the value of that entry. If we cannot find that entry inside names_map, we set execve_map_value->binary equals to 0 which means that this binary name is nowhere in all matchBinaries. Based in that we also generate the sel_names_map. This is a BPF_MAP_TYPE_HASH_OF_MAPS map where we index in the outter map using the selector ID and we index in the inner map using the execve_map_value->binary. If that exists, then matchBinary will be matched. The contents of the sel_names_map based on the previous tracing policy will be: sel_names_map[0 /* sel_index */ ] = { hash_map[1 /* value of cat in names_map */] = 1 /* always 1 for now */ } sel_names_map[1 /* sel_index */ ] = { hash_map[2 /* value of tail in names_map */] = 1 /* always 1 for now */ } Finally, if sel_names_map[sel_idx] is NULL this means that the specific selector does not container a matchBinaries action. If sel_names_map[sel_idx].hash_map[idx] is NULL this means that we don't care about the specific binary. Signed-off-by: Anastasios Papagiannis --- bpf/process/types/basic.h | 92 ++++++++++++++++------------ pkg/selectors/kernel.go | 17 ++--- pkg/selectors/selectors.go | 54 ++++++++++------ pkg/sensors/tracing/genericuprobe.go | 5 +- pkg/sensors/tracing/selectors.go | 64 +++++++++++++------ 5 files changed, 144 insertions(+), 88 deletions(-) diff --git a/bpf/process/types/basic.h b/bpf/process/types/basic.h index 83e2941f141..ee8813b4e65 100644 --- a/bpf/process/types/basic.h +++ b/bpf/process/types/basic.h @@ -155,6 +155,12 @@ static inline __attribute__((always_inline)) __u32 get_index(void *ctx) #define MAX_ENTRIES_CONFIG 1 #endif +// Filter tailcalls are {kprobe,tracepoint}/{6,7,8,9,10} +// We do one tail-call per selector, so we can have up to 5 selectors. +#define MIN_FILTER_TAILCALL 6 +#define MAX_FILTER_TAILCALL 10 +#define MAX_SELECTORS (MAX_FILTER_TAILCALL - MIN_FILTER_TAILCALL + 1) + static inline __attribute__((always_inline)) bool ty_is_nop(int ty) { switch (ty) { @@ -938,7 +944,10 @@ static inline __attribute__((always_inline)) size_t type_to_min_size(int type, /* * For matchBinaries we use two maps: * 1. names_map: global (for all sensors) keeps a mapping from names -> ids - * 2. sel_names_map: per-sensor: keeps a mapping from id -> selector val + * 2. sel_names_map: per-sensor: keeps a mapping from selector_id -> id -> selector val + * + * For each selector we have a separate inner map. We choose the appropriate + * inner map based on the selector ID. * * At exec time, we check names_map and set ->binary in execve_map equal to * the id stored in names_map. Assuming the binary name exists in the map, @@ -948,10 +957,16 @@ static inline __attribute__((always_inline)) size_t type_to_min_size(int type, * whether the selector matches or not. */ struct { - __uint(type, BPF_MAP_TYPE_HASH); - __uint(max_entries, 256); - __type(key, __u32); - __type(value, __u32); + __uint(type, BPF_MAP_TYPE_HASH_OF_MAPS); + __uint(max_entries, MAX_SELECTORS); + __uint(key_size, sizeof(u32)); /* selector id */ + __array( + values, struct { + __uint(type, BPF_MAP_TYPE_HASH); + __uint(max_entries, 256); + __type(key, __u32); + __type(value, __u32); + }); } sel_names_map SEC(".maps"); static inline __attribute__((always_inline)) int @@ -959,9 +974,8 @@ selector_arg_offset(__u8 *f, struct msg_generic_kprobe *e, __u32 selidx) { struct selector_arg_filter *filter; long seloff, argoff, pass; - __u32 index, *op; - char *args; - __u32 max = 0xffffffff; // UINT32_MAX + __u32 index; + char *args, *binaries_map; seloff = 4; /* start of the relative offsets */ seloff += (selidx * 4); /* relative offset for this selector */ @@ -982,32 +996,40 @@ selector_arg_offset(__u8 *f, struct msg_generic_kprobe *e, __u32 selidx) /* skip the matchCapabilityChanges by reading its length */ seloff += *(__u32 *)((__u64)f + (seloff & INDEX_MASK)); - op = map_lookup_elem(&sel_names_map, &max); - if (op) { - struct execve_map_value *execve; - bool walker = 0; - __u32 ppid, bin_key, *bin_val; - - execve = event_find_curr(&ppid, &walker); - if (!execve) - return 0; + // if binaries_map is NULL for the specific selidx, this + // means that the specific selector does not contain any + // matchBinaries actions. So we just proceed. + binaries_map = map_lookup_elem(&sel_names_map, &selidx); + if (binaries_map) { + __u32 *op, max = 0xffffffff; // UINT32_MAX + + op = map_lookup_elem(binaries_map, &max); + if (op) { + struct execve_map_value *execve; + bool walker = 0; + __u32 ppid, bin_key, *bin_val; + + execve = event_find_curr(&ppid, &walker); + if (!execve) + return 0; - bin_key = execve->binary; - bin_val = map_lookup_elem(&sel_names_map, &bin_key); + bin_key = execve->binary; + bin_val = map_lookup_elem(binaries_map, &bin_key); - /* - * The following things may happen: - * binary is not part of names_map, execve_map->binary will be `0` and `bin_val` will always be `0` - * binary is part of `names_map`: - * if binary is not part of this selector, bin_val will be`0` - * if binary is part of this selector: `bin_val will be `!0` - */ - if (*op == op_filter_in) { - if (!bin_val) - return 0; - } else if (*op == op_filter_notin) { - if (bin_val) - return 0; + /* + * The following things may happen: + * binary is not part of names_map, execve_map->binary will be `0` and `bin_val` will always be `0` + * binary is part of `names_map`: + * if binary is not part of this selector, bin_val will be`0` + * if binary is part of this selector: `bin_val will be `!0` + */ + if (*op == op_filter_in) { + if (!bin_val) + return 0; + } else if (*op == op_filter_notin) { + if (bin_val) + return 0; + } } } @@ -1302,12 +1324,6 @@ do_actions(struct msg_generic_kprobe *e, struct selector_action *actions, return i > 0 ? true : 0; } -// Filter tailcalls are {kprobe,tracepoint}/{6,7,8,9,10} -// We do one tail-call per selector, so we can have up to 5 selectors. -#define MIN_FILTER_TAILCALL 6 -#define MAX_FILTER_TAILCALL 10 -#define MAX_SELECTORS (MAX_FILTER_TAILCALL - MIN_FILTER_TAILCALL + 1) - static inline __attribute__((always_inline)) long filter_read_arg(void *ctx, int index, struct bpf_map_def *heap, struct bpf_map_def *filter, struct bpf_map_def *tailcalls, diff --git a/pkg/selectors/kernel.go b/pkg/selectors/kernel.go index 63f64ad7847..e655a457512 100644 --- a/pkg/selectors/kernel.go +++ b/pkg/selectors/kernel.go @@ -632,7 +632,7 @@ func ParseMatchCapabilityChanges(k *KernelSelectorState, actions []v1alpha1.Capa return nil } -func ParseMatchBinary(k *KernelSelectorState, b *v1alpha1.BinarySelector) error { +func ParseMatchBinary(k *KernelSelectorState, b *v1alpha1.BinarySelector, selIdx int) error { op, err := SelectorOp(b.Operator) if err != nil { return fmt.Errorf("matchBinary error: %w", err) @@ -640,22 +640,22 @@ func ParseMatchBinary(k *KernelSelectorState, b *v1alpha1.BinarySelector) error if op != SelectorOpIn && op != SelectorOpNotIn { return fmt.Errorf("matchBinary error: Only In and NotIn operators are supported") } - k.SetBinaryOp(op) + k.SetBinaryOp(selIdx, op) for _, s := range b.Values { if len(s) > 255 { return fmt.Errorf("matchBinary error: Binary names > 255 chars do not supported") } - k.AddBinaryName(s) + k.AddBinaryName(selIdx, s) } return nil } -func ParseMatchBinaries(k *KernelSelectorState, binarys []v1alpha1.BinarySelector) error { +func ParseMatchBinaries(k *KernelSelectorState, binarys []v1alpha1.BinarySelector, selIdx int) error { if len(binarys) > 1 { return fmt.Errorf("Only support single binary selector") } for _, s := range binarys { - if err := ParseMatchBinary(k, &s); err != nil { + if err := ParseMatchBinary(k, &s, selIdx); err != nil { return err } } @@ -665,6 +665,7 @@ func ParseMatchBinaries(k *KernelSelectorState, binarys []v1alpha1.BinarySelecto func parseSelector( k *KernelSelectorState, selectors *v1alpha1.KProbeSelector, + selIdx int, args []v1alpha1.KProbeArg, actionArgTable *idtable.Table) error { if err := ParseMatchPids(k, selectors.MatchPIDs); err != nil { @@ -682,7 +683,7 @@ func parseSelector( if err := ParseMatchCapabilityChanges(k, selectors.MatchCapabilityChanges); err != nil { return fmt.Errorf("parseMatchCapabilityChanges error: %w", err) } - if err := ParseMatchBinaries(k, selectors.MatchBinaries); err != nil { + if err := ParseMatchBinaries(k, selectors.MatchBinaries, selIdx); err != nil { return fmt.Errorf("parseMatchBinaries error: %w", err) } if err := ParseMatchArgs(k, selectors.MatchArgs, args); err != nil { @@ -739,7 +740,7 @@ func InitKernelSelectors(selectors []v1alpha1.KProbeSelector, args []v1alpha1.KP func InitKernelSelectorState(selectors []v1alpha1.KProbeSelector, args []v1alpha1.KProbeArg, actionArgTable *idtable.Table) (*KernelSelectorState, error) { - kernelSelectors := &KernelSelectorState{} + kernelSelectors := newKernelSelectorState() WriteSelectorUint32(kernelSelectors, uint32(len(selectors))) soff := make([]uint32, len(selectors)) @@ -749,7 +750,7 @@ func InitKernelSelectorState(selectors []v1alpha1.KProbeSelector, args []v1alpha for i, s := range selectors { WriteSelectorLength(kernelSelectors, soff[i]) loff := AdvanceSelectorLength(kernelSelectors) - if err := parseSelector(kernelSelectors, &s, args, actionArgTable); err != nil { + if err := parseSelector(kernelSelectors, &s, i, args, actionArgTable); err != nil { return nil, err } WriteSelectorLength(kernelSelectors, loff) diff --git a/pkg/selectors/selectors.go b/pkg/selectors/selectors.go index a9b296e052c..3785c57f661 100644 --- a/pkg/selectors/selectors.go +++ b/pkg/selectors/selectors.go @@ -17,6 +17,15 @@ var ( binVals = make(map[string]uint32) ) +type MatchBinariesMappings struct { + op uint32 + selNamesMap map[uint32]uint32 // these will be used for the sel_names_map +} + +func (k *MatchBinariesMappings) GetBinSelNamesMap() map[uint32]uint32 { + return k.selNamesMap +} + type KernelSelectorState struct { off uint32 // offset into encoding e [4096]byte // kernel encoding of selectors @@ -24,48 +33,53 @@ type KernelSelectorState struct { // valueMaps are used to populate value maps for InMap and NotInMap operators valueMaps []map[[8]byte]struct{} - // matchBinaries mappings - op uint32 - newBinVals map[uint32]string // these should be added in the names_map - selNamesMap map[uint32]uint32 // these will be used for the sel_names_map - initOnce sync.Once + matchBinaries map[int]*MatchBinariesMappings // matchBinaries mappings (one per selector) + newBinVals map[uint32]string // these should be added in the names_map } -func (k *KernelSelectorState) SetBinaryOp(op uint32) { - k.op = op +func newKernelSelectorState() *KernelSelectorState { + return &KernelSelectorState{ + matchBinaries: make(map[int]*MatchBinariesMappings), + newBinVals: make(map[uint32]string), + } } -func (k *KernelSelectorState) GetBinaryOp() uint32 { - return k.op +func (k *KernelSelectorState) SetBinaryOp(selIdx int, op uint32) { + // init a new entry (if needed) + if _, ok := k.matchBinaries[selIdx]; !ok { + k.matchBinaries[selIdx] = &MatchBinariesMappings{ + selNamesMap: make(map[uint32]uint32), + } + } + k.matchBinaries[selIdx].op = op } -func (k *KernelSelectorState) AddBinaryName(binary string) { - k.initOnce.Do(func() { - k.newBinVals = make(map[uint32]string) - k.selNamesMap = make(map[uint32]uint32) - }) +func (k *KernelSelectorState) GetBinaryOp(selIdx int) uint32 { + return k.matchBinaries[selIdx].op +} +func (k *KernelSelectorState) AddBinaryName(selIdx int, binary string) { binMu.Lock() defer binMu.Unlock() idx, ok := binVals[binary] if ok { - k.selNamesMap[idx] = 1 + k.matchBinaries[selIdx].selNamesMap[idx] = 1 return } idx = binIdx binIdx++ - binVals[binary] = idx // global map of all names_map entries - k.newBinVals[idx] = binary // new names_map entries that we should add - k.selNamesMap[idx] = 1 // value in the per-selector names_map (we ignore the value) + binVals[binary] = idx // global map of all names_map entries + k.newBinVals[idx] = binary // new names_map entries that we should add + k.matchBinaries[selIdx].selNamesMap[idx] = 1 // value in the per-selector names_map (we ignore the value) } func (k *KernelSelectorState) GetNewBinaryMappings() map[uint32]string { return k.newBinVals } -func (k *KernelSelectorState) GetBinSelNamesMap() map[uint32]uint32 { - return k.selNamesMap +func (k *KernelSelectorState) GetBinSelNamesMap() map[int]*MatchBinariesMappings { + return k.matchBinaries } func (k *KernelSelectorState) Buffer() [4096]byte { diff --git a/pkg/sensors/tracing/genericuprobe.go b/pkg/sensors/tracing/genericuprobe.go index a64d45baad0..b6b6c303942 100644 --- a/pkg/sensors/tracing/genericuprobe.go +++ b/pkg/sensors/tracing/genericuprobe.go @@ -125,9 +125,8 @@ func (k *observerUprobeSensor) LoadProbe(args sensors.LoadProbeArgs) error { Index: 0, Name: "sel_names_map", Load: func(m *ebpf.Map, index uint32) error { - // add a special entry (key == UINT32_MAX) that has as a value the number of matchBinaries entry - // if this is zero we don't have any matchBinaries selectors - if err := m.Update(uint32(0xffffffff), uprobeEntry.selectors.GetBinaryOp(), ebpf.UpdateAny); err != nil { + // add a special entry (key == UINT32_MAX) that has as a value the operation (In or NotIn) + if err := m.Update(uint32(0xffffffff), uprobeEntry.selectors.GetBinaryOp(0), ebpf.UpdateAny); err != nil { return err } for idx, val := range uprobeEntry.selectors.GetBinSelNamesMap() { diff --git a/pkg/sensors/tracing/selectors.go b/pkg/sensors/tracing/selectors.go index 655b4d982ac..b979fdd0bb9 100644 --- a/pkg/sensors/tracing/selectors.go +++ b/pkg/sensors/tracing/selectors.go @@ -63,13 +63,9 @@ func updateSelectors( return fmt.Errorf("failed to open sel_names_map map %s: %w", selNamesMapName, err) } defer selNamesMap.Close() - if err := selNamesMap.Update(uint32(0xffffffff), ks.GetBinaryOp(), ebpf.UpdateAny); err != nil { - return err - } - for idx, val := range ks.GetBinSelNamesMap() { - if err := selNamesMap.Update(idx, val, ebpf.UpdateAny); err != nil { - return err - } + + if err := populateBinariesMaps(ks, pinPathPrefix, selNamesMap); err != nil { + return fmt.Errorf("failed to populate sel_names_map: %w", err) } return nil @@ -93,18 +89,8 @@ func selectorsMaploads(ks *selectors.KernelSelectorState, pinPathPrefix string, }, { Index: 0, Name: "sel_names_map", - Load: func(m *ebpf.Map, index uint32) error { - // add a special entry (key == UINT32_MAX) that has as a value the number of matchBinaries entry - // if this is zero we don't have any matchBinaries selectors - if err := m.Update(uint32(0xffffffff), ks.GetBinaryOp(), ebpf.UpdateAny); err != nil { - return err - } - for idx, val := range ks.GetBinSelNamesMap() { - if err := m.Update(idx, val, ebpf.UpdateAny); err != nil { - return err - } - } - return nil + Load: func(outerMap *ebpf.Map, index uint32) error { + return populateBinariesMaps(ks, pinPathPrefix, outerMap) }, }, } @@ -160,3 +146,43 @@ func populateArgFilterMap( return nil } + +func populateBinariesMaps( + ks *selectors.KernelSelectorState, + pinPathPrefix string, + outerMap *ebpf.Map, +) error { + for innerID, sel := range ks.GetBinSelNamesMap() { + innerName := fmt.Sprintf("sel_names_map_%d", innerID) + innerSpec := &ebpf.MapSpec{ + Name: innerName, + Type: ebpf.Hash, + KeySize: 4, // uint32 + ValueSize: 4, // uint32 + MaxEntries: 256, + } + innerMap, err := ebpf.NewMapWithOptions(innerSpec, ebpf.MapOptions{ + PinPath: sensors.PathJoin(pinPathPrefix, innerName), + }) + if err != nil { + return fmt.Errorf("creating innerMap %s failed: %w", innerName, err) + } + defer innerMap.Close() + + // add a special entry (key == UINT32_MAX) that has as a value the operator (In or NotIn) + if err := innerMap.Update(uint32(0xffffffff), ks.GetBinaryOp(innerID), ebpf.UpdateAny); err != nil { + return err + } + + for idx, val := range sel.GetBinSelNamesMap() { + if err := innerMap.Update(idx, val, ebpf.UpdateAny); err != nil { + return err + } + } + + if err := outerMap.Update(uint32(innerID), uint32(innerMap.FD()), 0); err != nil { + return fmt.Errorf("failed to insert %s: %w", innerName, err) + } + } + return nil +}