Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tetragon: char_buf matchArgs works not as expected #576

Closed
wants to merge 1 commit into from

Conversation

Y-dc
Copy link

@Y-dc Y-dc commented Dec 2, 2022

fix #575 . preferably based on #568 and #564 .
I found the source of the bug in filter_char_buf(source code). the comparison of strings and their lengths is wrong.
form __copy_char_buf(source code) , the target arg in args should be offset by 8.

image

Signed-off-by: dechengyuan [email protected]

@Y-dc Y-dc requested a review from a team as a code owner December 2, 2022 12:20
@Y-dc Y-dc requested a review from tpapagian December 2, 2022 12:20
@Y-dc Y-dc force-pushed the pr/Y-dc/char_buf_matchargs branch from d596d5f to a2c6535 Compare December 2, 2022 12:52
@kkourt kkourt self-requested a review December 8, 2022 06:53
@Y-dc Y-dc force-pushed the pr/Y-dc/char_buf_matchargs branch from d8dfd6d to b6933fc Compare December 9, 2022 04:03
@kkourt kkourt added the needs-rebase This PR needs to be rebased because it has merge conflicts. label Jan 11, 2023
@jrfastab
Copy link
Contributor

@Y-dc could you rebase and add a test for us? This seems like it would be good to fix.

@Y-dc Y-dc force-pushed the pr/Y-dc/char_buf_matchargs branch from b6933fc to f6f967f Compare March 2, 2023 08:26
@Y-dc Y-dc requested a review from willfindlay as a code owner March 2, 2023 08:26
@Y-dc Y-dc force-pushed the pr/Y-dc/char_buf_matchargs branch from f6f967f to 89b058a Compare March 2, 2023 08:36
@olsajiri
Copy link
Contributor

olsajiri commented Mar 2, 2023

might be unrelated but there was recent fix from @kevsecurity in that area 5ebb0e3

@Y-dc
Copy link
Author

Y-dc commented Mar 2, 2023

might be unrelated but there was recent fix from @kevsecurity in that area 5ebb0e3

nice fix! I haven't used file buf matching yet, but it's really not relevant. filter_char_buf compares char buf errors due to offset errors

@Y-dc Y-dc force-pushed the pr/Y-dc/char_buf_matchargs branch from 89b058a to d818f9e Compare March 2, 2023 09:36
:);
v = (int)value[j];
a = (int)args[0];
v = ((int *)value)[j];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the original construction was broken (only taking the first byte of the length at &value[j], this replacement appears to incorrectly treat j as a (int *) index rather than a (char *) index. It probably needs to be:
v = *(int *)&value[j];
instead. Similar should be done for the following line, although it obviously matters less.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 It looks like you've removed this change. I think you were right it could do with fixing (for future proofing it nothing else). Did you disagree with "v = *(int *)&value[j];" ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you are right, I agree "v = *(int *)&value[j];". I removed the change to determine whether it was impossible to pass the test case before the change

@Y-dc Y-dc force-pushed the pr/Y-dc/char_buf_matchargs branch 4 times, most recently from d7c2860 to c552d4b Compare March 3, 2023 03:49
@Y-dc
Copy link
Author

Y-dc commented Mar 3, 2023

I have tested successfully.
TracingPolicy:

apiVersion: cilium.io/v1alpha1
kind: TracingPolicy
metadata:
  name: syscall
spec:
  tracepoints:
  - args:
    - index: 5
      returnCopy: false
      type: fd
    - index: 6
      returnCopy: false
      sizeArgIndex: 8
      type: char_buf
    - index: 7
      returnCopy: false
      type: size_t
    event: sys_enter_write
    selectors:
    - matchArgs:
      - index: 1
        operator: Equal
        values:
        - "123"
    subsystem: syscalls

go program print 123

    fmt.Printf("123")

tetragon log

time="2023-03-03T03:40:01Z" level=info msg="Loading sensor" name=gtp-sensor-4
time="2023-03-03T03:40:01Z" level=info msg="Loading kernel version 5.4.119"
time="2023-03-03T03:40:01Z" level=info msg="tetragon, map loaded." map=fdinstall_map path=/sys/fs/bpf/tetragon/gtp-sensor-4-gtp-2-fdinstall_map sensor=gtp-sensor-4
time="2023-03-03T03:40:01Z" level=info msg="tetragon, map loaded." map=tp_calls path=/sys/fs/bpf/tetragon/gtp-sensor-4-gtp-2-tp_calls sensor=gtp-sensor-4
time="2023-03-03T03:40:01Z" level=info msg="tetragon, map loaded." map=filter_map path=/sys/fs/bpf/tetragon/gtp-sensor-4-gtp-2-filter_map sensor=gtp-sensor-4
time="2023-03-03T03:40:01Z" level=info msg="tetragon, map loaded." map=argfilter_maps path=/sys/fs/bpf/tetragon/gtp-sensor-4-gtp-2-argfilter_maps sensor=gtp-sensor-4
time="2023-03-03T03:40:02Z" level=info msg="tetragon, map loaded." map=sel_names_map path=/sys/fs/bpf/tetragon/gtp-sensor-4-gtp-2-sel_names_map sensor=gtp-sensor-4
time="2023-03-03T03:40:02Z" level=info msg="tetragon, map loaded." map=fdinstall_map path=/sys/fs/bpf/tetragon/gtp-sensor-4-gtp-3-fdinstall_map sensor=gtp-sensor-4
time="2023-03-03T03:40:02Z" level=info msg="tetragon, map loaded." map=tp_calls path=/sys/fs/bpf/tetragon/gtp-sensor-4-gtp-3-tp_calls sensor=gtp-sensor-4
time="2023-03-03T03:40:02Z" level=info msg="tetragon, map loaded." map=filter_map path=/sys/fs/bpf/tetragon/gtp-sensor-4-gtp-3-filter_map sensor=gtp-sensor-4
time="2023-03-03T03:40:02Z" level=info msg="tetragon, map loaded." map=argfilter_maps path=/sys/fs/bpf/tetragon/gtp-sensor-4-gtp-3-argfilter_maps sensor=gtp-sensor-4
time="2023-03-03T03:40:02Z" level=info msg="tetragon, map loaded." map=sel_names_map path=/sys/fs/bpf/tetragon/gtp-sensor-4-gtp-3-sel_names_map sensor=gtp-sensor-4
time="2023-03-03T03:40:02Z" level=info msg="Loading registered BPF probe" Program=/var/lib/tetragon/bpf_generic_tracepoint_v53.o Type=generic_tracepoint
time="2023-03-03T03:40:07Z" level=info msg="Loaded generic tracepoint program: /var/lib/tetragon/bpf_generic_tracepoint_v53.o -> syscalls/sys_enter_write"
time="2023-03-03T03:40:07Z" level=info msg="Loading registered BPF probe" Program=/var/lib/tetragon/bpf_generic_tracepoint_v53.o Type=generic_tracepoint
time="2023-03-03T03:40:11Z" level=info msg="Loaded generic tracepoint program: /var/lib/tetragon/bpf_generic_tracepoint_v53.o -> syscalls/sys_enter_read"
time="2023-03-03T03:40:11Z" level=info msg="Loaded BPF maps and events for sensor successfully" sensor=gtp-sensor-4
---processed event:
 {"process_tracepoint":{"process":{"exec_id":"ZWtsZXQtc3VibmV0LTU3aG15ZW5lLTFhNnBmcHhkOjEyMDcwMDAwMDAwOjU2NQ==","pid":565,"uid":0,"cwd":"/data/code","binary":"/data/code/main","flags":"procFS auid","start_time":"2023-03-03T03:37:05.801376013Z","auid":4294967295,"pod":{"namespace":"dctestpro","name":"tetragon-dev-v2-8569d884b-blmvh","container":{"id":"containerd://b806b4c3fc896540a3d7078f66455fec660f51ace6d217fa819b7f0bbc7abbcc","name":"tetragon-dev-v2","image":{"id":"","name":""},"start_time":"2023-03-03T03:37:05Z","pid":7},"pod_labels":{"app":"tetragon-dev-v2","app-creator":"","app-env":"dev","app-id":"22668","devops-env":"release","env":"dev","k8s-app":"tetragon-dev-v2","node-type":"eks","paasid":"","paasname":"tetragon","pod-template-hash":"8569d884b"}},"docker":"b806b4c3fc896540a3d7078f66455fe","parent_exec_id":"ZWtsZXQtc3VibmV0LTU3aG15ZW5lLTFhNnBmcHhkOjExOTUwMDAwMDAwOjU1Mw==","refcnt":1},"parent":{"exec_id":"ZWtsZXQtc3VibmV0LTU3aG15ZW5lLTFhNnBmcHhkOjExOTUwMDAwMDAwOjU1Mw==","pid":553,"uid":0,"cwd":"/data/code","binary":"/usr/bin/bash","arguments":"-c /data/tmp/bd_dockerfile_cmdcommands.sh","flags":"procFS auid","start_time":"2023-03-03T03:37:05.681376378Z","auid":4294967295,"pod":{"namespace":"dctestpro","name":"tetragon-dev-v2-8569d884b-blmvh","container":{"id":"containerd://b806b4c3fc896540a3d7078f66455fec660f51ace6d217fa819b7f0bbc7abbcc","name":"tetragon-dev-v2","image":{"id":"","name":""},"start_time":"2023-03-03T03:37:05Z","pid":1},"pod_labels":{"app":"tetragon-dev-v2","app-creator":"dechengyuan","app-env":"dev","app-id":"22668","devops-env":"release","env":"dev","k8s-app":"tetragon-dev-v2","node-type":"eks","paasid":"","paasname":"tetragon","pod-template-hash":"8569d884b"}},"docker":"b806b4c3fc896540a3d7078f66455fe","parent_exec_id":"ZWtsZXQtc3VibmV0LTU3aG15ZW5lLTFhNnBmcHhkOjcyNDAwMDAwMDA6NDU1","refcnt":2},"subsys":"syscalls","event":"sys_enter_write","args":[{"size_arg":"1"},{"bytes_arg":"MTIz"},{"size_arg":"3"}]},"node_name":"eklet-subnet-57hmyene-1a6pfpxd","time":"2023-03-03T03:40:30.003200687Z"}

I need to check the failure of TestkprobeobjectOpen

@Y-dc
Copy link
Author

Y-dc commented Mar 3, 2023

i found the reason.
The string type and the char_buf type use the same filter processing function,
source
image

but use different copy functions, and the offsets of the string values and char_buf values ​​in the respective copy functions are different.
source
image

static inline __attribute__((always_inline)) long
copy_strings(char *args, unsigned long arg)
{
	int *s = (int *)args;
	long size;

	size = probe_read_str(&args[4], MAX_STRING, (char *)arg);
	if (size < 0) {
		return filter;
	}
	*s = size;
	// Initial 4 bytes hold string length
	return size + 4;
}

This patch fixup the char_buf args match (matchArgs) filter.

Signed-off-by: dechengyuan <[email protected]>
@Y-dc Y-dc force-pushed the pr/Y-dc/char_buf_matchargs branch from c552d4b to 925c50b Compare March 3, 2023 11:02
@kkourt kkourt mentioned this pull request Mar 7, 2023
Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Great catch and fix!

I have a few nits, but overall LGTM.
I made a PR with the suggested changes and test for the issue so that we don't break things again: #770.

We can either merge this PR and add the tests on top, or just merge #770. Up to you!

Thanks again!

bpf/process/types/basic.h Show resolved Hide resolved
bpf/process/types/basic.h Show resolved Hide resolved
bpf/process/types/basic.h Show resolved Hide resolved
@Y-dc
Copy link
Author

Y-dc commented Mar 8, 2023

Thanks! Great catch and fix!

I have a few nits, but overall LGTM. I made a PR with the suggested changes and test for the issue so that we don't break things again: #770.

We can either merge this PR and add the tests on top, or just merge #770. Up to you!

Thanks again!

This PR does not add tests, merging #770 does it!

@kkourt
Copy link
Contributor

kkourt commented Mar 9, 2023

Thanks @Y-dc! Merged #770, so I'm closing this PR.

@kkourt kkourt closed this Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase This PR needs to be rebased because it has merge conflicts.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: char_buf matchArgs works not as expected
5 participants