killer sensor: add support for fmod_ret #1953
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kkourt
added
the
release-note/minor
kkourt
force-pushed
the
pr/kkourt/killer-improvemenmts-2
branch
from
7c5bd21
to
17d9a19
Compare
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
kkourt
force-pushed
the
pr/kkourt/killer-improvemenmts-2
branch
from
be22074
to
d25bef3
Compare
olsajiri
reviewed
Jan 12, 2024
Have the killer sensor respect the spec setting to disable multi-probe. Also, rename the options from kprobeOptions to specOptions since they refer to the whole spec rather than just the kprobe section. Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
Some kernels do not include CONFIG_BPF_KPROBE_OVERRIDE so it's not possible to use bpf_override_return(). Instead, we can use fmod_ret. Previously, we were building two versions of the killer program: one with multi-kprobe and one without. With this patch, we build a third version: one that uses fmod_ret. Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
kkourt
force-pushed
the
pr/kkourt/killer-improvemenmts-2
branch
4 times, most recently
from
84cfbe9
to
b7a4417
Compare
|
Failure of go tests seems unrelated: CC: @tixxdz |
jrfastab
approved these changes
Jan 17, 2024
tixxdz
approved these changes
Jan 17, 2024
olsajiri
reviewed
Jan 17, 2024
This patch adds support for using fmod_ret in the killer sensor. This is a less preferable option than bpf_override_return() because it requires multiple programs. Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
The killer program uses bpf_send_signal(). If the helper is not supported, it cannot be loaded. Add a check in the killer sensor for this. We could have another version of the killer program that does not use bpf_send_signal() and can only be used for override, but this is beyond the scope of this patch. Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
KillerSpecBuilder is a simple builder for creating tracing policies that use the killer sensor. It is meant for testing and it will be used in subsequent patches. Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
There are two ways to override the return value of a syscall: using bpf_override_return() and fmod_ret. The former is preferable because we do not need multiple programs. The killer sensor detects what is available and acts accordingly. This PR adds a spec option that forces a specific override method. While this might be useful to end-users, it is mainly intended for testing because it allows us to test fmod_ret for kernels that do support bpf_override_return(). It will be used in a subsequent patch. Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
Modify the killer tests to use the builder introduced in a previous patch. Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
This patch uses the override option in the spec (introduced in a previous patch) to test more combinations. Specifically, it tests fmod_ret even when bpf_override_return() is available, and single kprobe when multi-kprobe is available. Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
On the latest 5.4, I'm seeing the following failure:
```
=== RUN TestKillerOverride/override_helper/kprobe_(no_multi)
...
load program: invalid argument:
; generic_tracepoint_filter(void *ctx)
0: (bf) r6 = r1
1: (b7) r1 = 0
; int ret, zero = 0;
2: (63) *(u32 *)(r10 -28) = r1
last_idx 2 first_idx 0
regs=2 stack=0 before 1: (b7) r1 = 0
3: (bf) r2 = r10
;
4: (07) r2 += -28
; msg = map_lookup_elem(&tp_heap, &zero);
5: (18) r1 = 0xffff954178190400
7: (85) call bpf_map_lookup_elem#1
8: (bf) r7 = r0
; if (!msg)
9: (15) if r7 == 0x0 goto pc+1989
R0_w=map_value(id=0,off=0,ks=4,vs=24304,imm=0) R6_w=ctx(id=0,off=0,imm=0) R7_w=map_value(id=0,off=0,ks=4,vs=24304,imm=0) R10=fp0 fp-32=mmmm????
; &msg->caps, &filter_map, msg->idx);
10: (61) r1 = *(u32 *)(r7 +24288)
R0_w=map_value(id=0,off=0,ks=4,vs=24304,imm=0) R6_w=ctx(id=0,off=0,imm=0) R7_w=map_value(id=0,off=0,ks=4,vs=24304,imm=0) R10=fp0 fp-32=mmmm????
11: (63) *(u32 *)(r10 -24) = r1
; struct task_struct *task = (struct task_struct *)get_current_task();
12: (85) call bpf_get_current_task#35
13: (bf) r9 = r0
; struct task_struct *task = (struct task_struct *)get_current_task();
14: (7b) *(u64 *)(r10 -16) = r9
; __u32 pid = get_current_pid_tgid() >> 32;
15: (85) call bpf_get_current_pid_tgid#14
; __u32 pid = get_current_pid_tgid() >> 32;
...
regs=4 stack=0 before 1895: (4f) r2 |= r6
regs=44 stack=0 before 1894: (4f) r2 |= r1
regs=46 stack=0 before 1893: (67) r2 <<= 24
regs=46 stack=0 before 1892: (79) r2 = *(u64 *)(r10 -40)
regs=42 stack=10 before 1891: (67) r1 <<= 16
regs=42 stack=10 before 1890: (79) r1 = *(u64 *)(r10 -48)
regs=40 stack=30 before 1889: (4f) r6 |= r1
regs=42 stack=30 before 1888: (79) r1 = *(u64 *)(r10 -56)
regs=40 stack=70 before 1887: (67) r6 <<= 8
regs=40 stack=70 before 1886: (4f) r9 |= r7
regs=40 stack=70 before 1885: (4f) r9 |= r8
regs=40 stack=70 before 1884: (67) r9 <<= 24
regs=40 stack=70 before 1883: (67) r8 <<= 16
regs=40 stack=70 before 1882: (4f) r7 |= r1
regs=40 stack=70 before 1881: (79) r1 = *(u64 *)(r10 -64)
regs=40 stack=70 before 1880: (67) r7 <<= 8
regs=40 stack=70 before 1879: (15) if r0 == 0x0 goto pc+95
regs=40 stack=70 before 1878: (85) call bpf_map_lookup_elem#1
regs=40 stack=70 before 1876: (18) r1 = 0xffff954177317000
regs=40 stack=70 before 1875: (07) r2 += -16
regs=40 stack=70 before 1874: (bf) r2 = r10
regs=40 stack=70 before 1873: (63) *(u32 *)(r10 -16) = r0
regs=40 stack=70 before 1872: (77) r0 >>= 32
R0_rw=inv(id=0) R6_rw=invP(id=0,umax_value=255,var_off=(0x0; 0xff)) R7_rw=inv(id=0,umax_value=255,var_off=(0x0; 0xff)) R8_rw=inv(id=0,umax_value=255,var_off=(0x0; 0xff)) R9_rw=inv(id=0,umax_value=255,var_off=(0x0; 0xff)) R10=fp0 fp-8=mmmm???? fp-16_r=mmmmmmmm fp-24=mmmmmmmm fp-32=mmmm???? fp-40_rw=mmmmmmmm fp-48_rw=mmmmmmmm fp-56_rw=mmmmmmmm fp-64_rw=mmmmmmmm fp-72_rw=mmmmmmmm fp-80_rw=mmmmmmmm fp-88_rw=mmmmmmmm fp-96_r=map_value fp-104=map_value fp-112=ctx fp-120=mmmmmmmm fp-128=mmmmmmmm fp-136_r=map_value fp-144_rw=mmmmmmmm fp-152_rw=mmmmmmmm fp-160_rw=mmmmmmmm fp-168_rw=mmmmmmmm fp-176_rw=mmmmmmmm fp-184_rw=mmmmmmmm fp-192_rw=mmmmmmmm fp-200_rw=mmmmmmmm fp-208_rw=mmmmmmmm
parent didn't have regs=40 stack=0 marks
last_idx 1871 first_idx 1821
regs=40 stack=0 before 1871: (85) call bpf_get_current_pid_tgid#14
regs=40 stack=0 before 1870: (71) r6 = *(u8 *)(r1 +1)
math between map_value pointer and register with unbounded min value is not allowed
processed 3841 insns (limit 1000000) max_states_per_insn 4 total_states 173 peak_states 170 mark_read 136
logcapture.go:25: time="2024-01-10T10:30:34Z" level=info msg="Unloading sensor __killer__"
logcapture.go:25: time="2024-01-10T10:30:34Z" level=info msg="BPF prog was unloaded" label=kprobe/killer pin=killer-sensor-1-killer_kprobe
logcapture.go:25: time="2024-01-10T10:30:34Z" level=info msg="map was unloaded" map=killer_data pin=killer_data
logcapture.go:25: time="2024-01-10T10:30:34Z" level=info msg="map was unloaded" map=killer_data pin=killer_data
logcapture.go:25: time="2024-01-10T10:30:34Z" level=info msg="Cleaning up killer"
observer_test_helper.go:443: SensorManager.AddTracingPolicy error: sensor gtp-sensor-2 from collection killer-override failed to load: failed prog /home/kkourt/src/tetragon/bpf/objs/bpf_generic_tracepoint_v53.o kern_version 328959 loadInstance: opening collection '/home/kkourt/src/tetragon/bpf/objs/bpf_generic_tracepoint_v53.o' failed: program generic_tracepoint_filter: load program: invalid argument: math between map_value pointer and register with unbounded min value is not allowed (18925 line(s) omitted)
filenames.go:47: keeping export file for TestKillerOverride-override_helper-kprobe_(no_multi) (/tmp/tetragon.gotest.TestKillerOverride-override_helper-kprobe_(no_multi).4012676103.json)
```
It's not clear to me what the issue exatly is, but reading the code I
found an opportunity to simplify it which seems to fix the issue.
Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
This is a preparation patch for subsequent patches. This is just reorginization, there are no functional changes. Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
Add a getcpu program that just performs the getcpu syscall and exits with the error value. Will be used in the next patch. Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
In one of the previous iterations of this PR, the patch that implemented the fmod_ret changes had an bug. The bug, instead of hooking the program to the syscall, hooked the program to security_task_prctl, which is the default label we use in the ELF object. As a result, the test was never caught by our testing. This patch, updates the override test to the getcpu syscall to ensure that the test works as expected. Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
Our detection code already has a function for detecting of fmod_ret is
supported named HasModifyReturn. If we want to load an fmod_ret program
in a syscall function, however, checking HasModifyReturn() is not
sufficient.
This is because in kernels where CONFIG_FUNCTION_ERROR_INJECTION is not
set, loading fmod_ret programs in syscalls is not supported.
So add a separate check, where we try to attach to a system call (we use
getcpu) instead of a security_ function and use it for feature detection
in the killer sensor.
Kernel check is:
```
static int check_attach_modify_return(unsigned long addr, const char *func_name)
{
if (within_error_injection_list(addr) ||
!strncmp(SECURITY_PREFIX, func_name, sizeof(SECURITY_PREFIX) - 1))
return 0;
return -EINVAL;
}
```
And:
```
...
static inline bool within_error_injection_list(unsigned long addr)
{
return false;
}
static inline int get_injectable_error_type(unsigned long addr)
{
return -EOPNOTSUPP;
}
```
Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
kkourt
force-pushed
the
pr/kkourt/killer-improvemenmts-2
branch
from
b7a4417
to
8706ff3
Compare
|
@olsajiri thanks! Pushed a new version, PTAL. |
olsajiri
approved these changes
Jan 17, 2024
|
@tixxdz name needs to change IMO but I think its fine to do a search and replace and move PR. I wouldn't mix it up with improvements/fixes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this PR is a followup to #1948.
It adds
fmod_retsupport to the killer sensor and further improves testing.