Rework the matchBinaries selector implementation

bpf/lib/generic.h

@@ -69,17 +69,4 @@ struct sched_execve_args {
 	int old_pid;
 };
 
-#ifndef ALIGNCHECKER
-struct names_map_key {
-	char path[256];
-};
-
-struct {
-	__uint(type, BPF_MAP_TYPE_HASH);
-	__uint(max_entries, 256); /* maximum number of binary names for all matchBinary selectors */
-	__type(key, struct names_map_key);
-	__type(value, __u32);
-} names_map SEC(".maps");
-
-#endif // ALIGNCHECKER
 #endif // _GENERIC__

bpf/lib/process.h

@@ -289,9 +289,25 @@ struct msg_execve_event {
 	/* below fields are not part of the event, serve just as
 	 * heap for execve programs
 	 */
-	__u32 binary;
 }; // All fields aligned so no 'packed' attribute.
 
+#define BINARY_PATH_MAX_LEN 256
+
+// This structure stores the binary path that was recorded on execve.
+// Technically PATH_MAX is 4096 but we limit the length we store since we have
+// limits on the length of the string to compare:
+// - Artificial limits for full string comparison.
+// - Technical limits for prefix and postfix, using LPM_TRIE that have a 256
+//   bytes size limit.
+struct binary {
+	// length of the path stored in path, this should be < BINARY_PATH_MAX_LEN
+	// but can contain negative value in case of copy error.
+	// While s16 would be sufficient, 64 bits are handy for alignment.
+	__s64 path_length;
+	// BINARY_PATH_MAX_LEN first bytes of the path
+	char path[BINARY_PATH_MAX_LEN];
+}; // All fields aligned so no 'packed' attribute
+
 // The execve_map_value is tracked by the TGID of the thread group
 // the msg_execve_key.pid. The thread IDs are recorded on the
 // fly and sent with every corresponding event.
@@ -300,10 +316,9 @@ struct execve_map_value {
 	struct msg_execve_key pkey;
 	__u32 flags;
 	__u32 nspid;
-	__u32 binary;
-	__u32 pad;
 	struct msg_ns ns;
 	struct msg_capabilities caps;
+	struct binary bin;
 } __attribute__((packed)) __attribute__((aligned(8)));
 
 struct {

bpf/process/bpf_execve_event.c

@@ -130,28 +130,6 @@ read_cwd(void *ctx, struct msg_process *p)
 	return getcwd(p, p->size, p->pid);
 }
 
-static inline __attribute__((always_inline)) __u32
-binary_filter(void *ctx, struct msg_execve_event *event, void *filename)
-{
-	struct msg_process *p = &event->process;
-	struct execve_heap *heap;
-	uint32_t *value;
-	__u32 zero = 0;
-
-	// skip binaries check for long (> 255) filenames for now
-	if (p->flags & EVENT_DATA_FILENAME)
-		return 0;
-
-	heap = map_lookup_elem(&execve_heap, &zero);
-	if (!heap)
-		return 0;
-
-	memset(heap->pathname, 0, PATHNAME_SIZE);
-	probe_read_str(heap->pathname, PATHNAME_SIZE, filename);
-	value = map_lookup_elem(&names_map, heap->pathname);
-	return value ? *value : 0;
-}
-
 static inline __attribute__((always_inline)) __u32
 read_execve_shared_info(void *ctx, __u64 pid)
 {
@@ -214,8 +192,6 @@ event_execve(struct sched_execve_args *ctx)
 	event->common.ktime = p->ktime;
 	event->common.size = offsetof(struct msg_execve_event, process) + p->size;
 
-	event->binary = binary_filter(ctx, event, filename);
-
 	BPF_CORE_READ_INTO(&event->kube.net_ns, task, nsproxy, net_ns, ns.inum);
 
 	// At this time objective and subjective creds are same
@@ -274,7 +250,6 @@ execve_send(struct sched_execve_args *ctx)
 			event_set_clone(p);
 		}
 		curr->flags = 0;
-		curr->binary = event->binary;
 #ifdef __NS_CHANGES_FILTER
 		if (init_curr)
 			memcpy(&(curr->ns), &(event->ns),
@@ -287,6 +262,17 @@ execve_send(struct sched_execve_args *ctx)
 			curr->caps.inheritable = event->caps.inheritable;
 		}
 #endif
+		// buffer can be written at clone stage with parent's info, if previous
+		// path is longer than current, we can have leftovers at the end.
+		memset(&curr->bin, 0, sizeof(curr->bin));
+		// reuse p->args first string that contains the filename, this can't be
+		// above 256 in size (otherwise the complete will be send via data msg)
+		// which is okay because we need the 256 first bytes.
+		curr->bin.path_length = probe_read_str(curr->bin.path, BINARY_PATH_MAX_LEN, &p->args);
+		if (curr->bin.path_length > 1) {
+			// don't include the NULL byte in the length
+			curr->bin.path_length--;
+		}
 	}
 
 	event->common.flags = 0;

bpf/process/bpf_fork.c

@@ -58,7 +58,7 @@ BPF_KPROBE(event_wake_up_new_task, struct task_struct *task)
 	curr->key.pid = tgid;
 	curr->key.ktime = ktime_get_ns();
 	curr->nspid = get_task_pid_vnr();
-	curr->binary = parent->binary;
+	memcpy(&curr->bin, &parent->bin, sizeof(curr->bin));
 	curr->pkey = parent->key;
 
 	/* Setup the msg_clone_event and sent to the user. */

bpf/process/types/basic.h

@@ -848,7 +848,7 @@ filter_char_buf_postfix(struct selector_arg_filter *filter, char *arg_str, uint
 
 static inline __attribute__((always_inline)) bool is_not_operator(__u32 op)
 {
-	return (op == op_filter_neq || op == op_filter_str_notprefix || op == op_filter_str_notpostfix);
+	return (op == op_filter_neq || op == op_filter_str_notprefix || op == op_filter_str_notpostfix || op == op_filter_notin);
 }
 
 static inline __attribute__((always_inline)) long
@@ -1424,73 +1424,79 @@ static inline __attribute__((always_inline)) size_t type_to_min_size(int type,
 
 #define INDEX_MASK 0x3ff
 
-/*
- * 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 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,
- * otherwise binary is 0.
- *
- * When we check the selectors, use ->binary to index sel_names_map and decide
- * whether the selector matches or not.
- */
+struct match_binaries_sel_opts {
+	__u32 op;
+};
+
+// This map is used by the matchBinaries selectors to retrieve their options
 struct {
-	__uint(type, BPF_MAP_TYPE_HASH_OF_MAPS);
+	__uint(type, BPF_MAP_TYPE_ARRAY);
 	__uint(max_entries, MAX_SELECTORS);
-	__uint(key_size, sizeof(u32)); /* selector id */
+	__type(key, __u32); /* selector id */
+	__type(value, struct match_binaries_sel_opts);
+} tg_mb_sel_opts SEC(".maps");
+
+#define MATCH_BINARIES_PATH_MAX_LENGTH 256
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
+	__uint(max_entries, MAX_SELECTORS); // only one matchBinaries per selector
+	__uint(key_size, sizeof(__u32));
 	__array(
 		values, struct {
 			__uint(type, BPF_MAP_TYPE_HASH);
-			__uint(max_entries, 256);
-			__type(key, __u32);
-			__type(value, __u32);
+			__uint(max_entries, 1);
+			__type(key, __u8[MATCH_BINARIES_PATH_MAX_LENGTH]);
+			__type(value, __u8);
 		});
-} sel_names_map SEC(".maps");
-
-static inline __attribute__((always_inline)) int match_binaries(void *sel_names, __u32 selidx)
-{
-	void *binaries_map;
-	struct execve_map_value *execve;
-	__u32 *op, max = 0xffffffff; // UINT32_MAX
-	__u32 ppid, bin_key, *bin_val;
-	bool walker = 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, &selidx);
-	if (binaries_map) {
-		op = map_lookup_elem(binaries_map, &max);
-		if (op) {
-			execve = event_find_curr(&ppid, &walker);
-			if (!execve)
-				return 0;
+} tg_mb_paths SEC(".maps");
+
+static inline __attribute__((always_inline)) int match_binaries(__u32 selidx)
+{
+	struct execve_map_value *current;
+	__u32 ppid;
+	bool walker, match = 0;
+	void *path_map;
+	__u8 *found_key;
 
-			bin_key = execve->binary;
-			bin_val = map_lookup_elem(binaries_map, &bin_key);
+	struct match_binaries_sel_opts *selector_options;
 
-			/*
-			 * 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;
-			}
+	// retrieve the selector_options for the matchBinaries, if it's NULL it
+	// means there is not matchBinaries in this selector.
+	selector_options = map_lookup_elem(&tg_mb_sel_opts, &selidx);
+	if (selector_options) {
+		if (selector_options->op == op_filter_none)
+			return 1; // matchBinaries selector is empty <=> match
+
+		current = event_find_curr(&ppid, &walker);
+		if (!current) {
+			// this should not happen, it means that the process was missed when
+			// scanning /proc for process that started before and after tetragon
+			return 0;
 		}
+		if (current->bin.path_length < 0) {
+			// something wrong happened when copying the filename to execve_map
+			return 0;
+		}
+
+		switch (selector_options->op) {
+		case op_filter_in:
+		case op_filter_notin:
+			path_map = map_lookup_elem(&tg_mb_paths, &selidx);
+			if (!path_map)
+				return 0;
+			found_key = map_lookup_elem(path_map, current->bin.path);
+			match = !!found_key;
+			break;
+		default:
+			// should not happen
+			return 0;
+		}
+
+		return is_not_operator(selector_options->op) ? !match : match;
 	}
 
+	// no matchBinaries selector <=> match
 	return 1;
 }
 
@@ -1499,7 +1505,7 @@ generic_process_filter_binary(struct event_config *config)
 {
 	/* single flag bit at the moment (FLAGS_EARLY_FILTER) */
 	if (config->flags & FLAGS_EARLY_FILTER)
-		return match_binaries(&sel_names_map, 0);
+		return match_binaries(0);
 	return 1;
 }
 
@@ -1533,7 +1539,7 @@ selector_arg_offset(__u8 *f, struct msg_generic_kprobe *e, __u32 selidx,
 	seloff += *(__u32 *)((__u64)f + (seloff & INDEX_MASK));
 
 	// check for match binary actions
-	if (!early_binary_filter && !match_binaries(&sel_names_map, selidx))
+	if (!early_binary_filter && !match_binaries(selidx))
 		return 0;
 
 	/* Making binary selectors fixes size helps on some kernels */

pkg/api/processapi/processapi.go

@@ -37,6 +37,8 @@ const (
 	// flags of MsgCommon
 	MSG_COMMON_FLAG_RETURN     = 0x1
 	MSG_COMMON_FLAG_STACKTRACE = 0x2
+
+	BINARY_PATH_MAX_LEN = 256
 )
 
 type MsgExec struct {
@@ -135,6 +137,11 @@ type MsgCapabilities struct {
 	Inheritable uint64
 }
 
+type Binary struct {
+	PathLength int64
+	Path       [BINARY_PATH_MAX_LEN]byte
+}
+
 type MsgNamespaces struct {
 	UtsInum       uint32
 	IpcInum       uint32

pkg/selectors/kernel.go

@@ -11,6 +11,7 @@ import (
 	"strings"
 
 	"github.com/cilium/tetragon/api/v1/tetragon"
+	"github.com/cilium/tetragon/pkg/api/processapi"
 	"github.com/cilium/tetragon/pkg/idtable"
 	"github.com/cilium/tetragon/pkg/k8s/apis/cilium.io/v1alpha1"
 	"github.com/cilium/tetragon/pkg/kernels"
@@ -1170,22 +1171,31 @@ func ParseMatchBinary(k *KernelSelectorState, b *v1alpha1.BinarySelector, selIdx
 	if err != nil {
 		return fmt.Errorf("matchBinary error: %w", err)
 	}
-	if op != SelectorOpIn && op != SelectorOpNotIn {
-		return fmt.Errorf("matchBinary error: Only In and NotIn operators are supported")
-	}
-	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")
+
+	// prepare the selector options
+	sel := MatchBinariesSelectorOptions{}
+	sel.Op = op
+
+	switch op {
+	case SelectorOpIn, SelectorOpNotIn:
+		for _, s := range b.Values {
+			if len(s) > processapi.BINARY_PATH_MAX_LEN-1 {
+				return fmt.Errorf("matchBinary error: Binary names > %d chars do not supported", processapi.BINARY_PATH_MAX_LEN-1)
+			}
+			k.WriteMatchBinariesPath(selIdx, s)
 		}
-		k.AddBinaryName(selIdx, s)
+	default:
+		return fmt.Errorf("matchBinary error: Only \"In\" and \"NotIn\" operators are supported")
 	}
+
+	k.AddMatchBinaries(selIdx, sel)
+
 	return nil
 }
 
 func ParseMatchBinaries(k *KernelSelectorState, binarys []v1alpha1.BinarySelector, selIdx int) error {
 	if len(binarys) > 1 {
-		return fmt.Errorf("Only support single binary selector")
+		return fmt.Errorf("only support a single matchBinaries per selector")
 	}
 	for _, s := range binarys {
 		if err := ParseMatchBinary(k, &s, selIdx); err != nil {