Add dynamic extraction of a parameter attribute #3143

ScriptSathi · 2024-11-19T16:27:31Z

The discussion for this PR can be found here #3142

Take this PR in the today state as a proof of concept for dynamic parameter extraction. I will continue to work on this PR until the below checks are done.

At the current state, the PR is able to

Extract parameters from LSM programs
Extract parameters from kprobes programs
Handle exception for uprobes as it is not part of BTF
Have unit tests for all cases
- Validate CI for all kernel versions

Description

This PR introduce the dynamic extraction of a custom attribute

Test the PR

You can use the following config

apiVersion: cilium.io/v1alpha1
kind: TracingPolicy
metadata:
  name: "lsm"
spec:
  lsmhooks:
  - hook: "bprm_check_security"
    args:
      - index: 0
        type: "string"
        resolve: "file.f_path.dentry.d_name.name"
    selectors:
      - matchArgs:
        - index: 0
          operator: "Postfix"
          values:
            - "ls"
            - "sh"
            - "bash"

Release-note

Add attribute resolution

netlify · 2024-11-20T10:49:07Z

✅ Deploy Preview for tetragon ready!

Name	Link
🔨 Latest commit	`41783b6`
🔍 Latest deploy log	https://app.netlify.com/sites/tetragon/deploys/67ab981943add60008716111
😎 Deploy Preview	https://deploy-preview-3143--tetragon.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

olsajiri

I like it, left some comments

thanks for splitting the code in multiple logical changes, it's rare ;-)

we were originally thinking of using C expression parsing (there's some go lib for that) but I think that can be done later if it's ever needed, the basic parsing you did should be fine

please add tests, would be great to have some framework where we could easily add various 'ExtractParam' expressions for testing

pkg/sensors/tracing/genericuprobe.go

bpf/process/types/basic.h

pkg/btf/btf.go

bpf/process/generic_calls.h

bpf/process/types/basic.h

bpf/process/generic_calls.h

pkg/sensors/tracing/generickprobe.go

olsajiri

looks great, I left some comments, thanks

bpf/process/types/basic.h

pkg/sensors/tracing/generic.go

pkg/btf/btf.go

pkg/sensors/tracing/genericuprobe.go

pkg/btf/btf_test.go

examples/tracingpolicy/lsm_track_grandparent.yml

bpf/process/types/basic.h

mtardy

I've been pulled-in for the docs change, here's a review. Thanks for adding docs!

pkg/sensors/tracing/generic.go

docs/content/en/docs/concepts/tracing-policy/hooks.md

examples/tracingpolicy/lsm_track_grandparent.yml

docs/content/en/docs/concepts/tracing-policy/hooks.md

ScriptSathi · 2025-01-23T15:13:51Z

@olsajiri The CI tests are still crashing on all kernel versions. I ran my tests on a 6.6 kernel, and everything worked fine. I'll investigate further to figure out the issue and update you soon.

EDIT : The tests were failing because btfFile was not set in the pipeline test, but it was in local. So I've added a new commit and the LoadBTF function to make sure this won’t happen again.

EDIT 2 : CI fails on pkg.watcher.TestFastK8s. It looks like it is related to this issue #2972

bentekkie · 2025-02-06T20:18:56Z

Hi, This feature is amazing, thanks for implementing it as it will open up usage of all the security_path_* kprobes that have dentry arguments, which is something I have been waiting for 😄
Any idea when this will land?

olsajiri · 2025-02-07T14:22:38Z

Hi, This feature is amazing, thanks for implementing it as it will open up usage of all the security_path_* kprobes that have dentry arguments, which is something I have been waiting for 😄 Any idea when this will land?

I think @kkourt wanted to check that before merging

kkourt

Thanks! This PR looks fantastic!

Please find some comments below. Other than a few minor things, I think we are good to merge this. Some of the comments refer to additional work, but I would suggest to leave this as follow-ups since the PR is already a lot of commits.

bpf/process/types/basic.h

kkourt · 2025-02-10T06:54:27Z

pkg/api/tracingapi/client_kprobe.go

 const EventConfigMaxArgs = 5
+const MaxBtfArgDepth = 10 // Artificial value for compilation, may be extended


Not sure what you mean by "artificial" here. Shouldn't this value be the same as "MAX_BTF_ARG_DEPTH" from bpf side?

kkourt · 2025-02-10T07:05:29Z

bpf/process/generic_calls.h

+		return 1;
+	*data->arg = *data->arg + data->btf_config[i].offset;
+	if (data->btf_config[i].is_pointer)
+		probe_read((void *)data->arg, sizeof(char *), (void *)*data->arg);


Note sure how this works if ->is_pointer is false. Can you please provide an example/explanation in the commit message or in a comment?

When accessing task_struct->comm it is not needed since comm is not a pointer

https://elixir.bootlin.com/linux/v6.13-rc3/source/include/linux/sched.h#L1138:

char comm[TASK_COMM_LEN];

I'm sorry, but I still don't understand how things are supposed to work without pointers.
Can you provide an example of a hook and a "resolve:" field?

bpf/process/generic_calls.h

pkg/generictypes/generictypes.go

kkourt · 2025-02-10T08:00:31Z

pkg/btf/btf.go

+			(*btfArgs)[i].IsInitialized = uint16(1)
+			return ResolveBtfPath(btfArgs, t.Target, pathToFound, i+1)
+		}
+		(*btfArgs)[i-1].IsPointer = uint16(1)


Why is the above line needed?
As I commented in the bpf code, it's not clear to me what it means to have IsPonter equal to false but IsIntialized set to true.

kkourt · 2025-02-10T08:02:15Z

pkg/btf/btf.go

@@ -216,6 +216,8 @@ func processMembers(
 	if t, ok := currentType.(*btf.Pointer); ok {
 		(*btfArgs)[i].IsPointer = uint16(1)
 		currentType = t.Target
+	} else if _, ok := currentType.(*btf.Int); ok {
+		(*btfArgs)[i].IsPointer = uint16(1)


Any reason to not fold in this patch with the original version?

pkg/sensors/tracing/generic.go

kkourt · 2025-02-10T08:11:54Z

pkg/sensors/tracing/genericlsm.go

+				// https://lore.kernel.org/bpf/[email protected]/
+				return errFn(fmt.Errorf("Error: LSM programs could not use Resolve flag for your kernel version." +
+					"Please use Kprobe instead or update your kernel to version 5.8 or higher"))
+			}


Checking kernel versions does not always work as expected because there are kernels with older versions that backport a lot of functionality.

Am I correct to assume that the resolveBtfArg would fail anyway? If so, I would suggest something like:

lastBtfType, btfArg, err := resolveBtfArg("bpf_lsm_"+f.Hook, a) if err != nil { return errFn(fmt.Errorf("could not resolve path from btf. You can try to use a kprobe or update your kernel version to 5.8 or later")) }

docs/content/en/docs/concepts/tracing-policy/hooks.md

mtardy

Thanks, doc addition now looks good :)

This commit introduces the `struct config_btf_arg_depth`. It appends `btf_argN` to `struct event_config` as an array. This array stores the path to the searched data. Any `btf_argN` can have a list of elements as follow : file->f_path is 152 bytes, so the array will look like [{ offset: 152, is_pointer: 0, is_initialized: 1 }, ...]. The max value `MAX_BTF_ARG_DEPTH` as been set arbitrary as the verifier need a fixed size. In config_btf_arg, is_pointer and is_initialized are u16 because it must match padding of 64 bits long structure Signed-off-by: Tristan d'Audibert <[email protected]>

…_argN This commit introduces the “extract_arg_depth” function and loops over it to move into the “arg” buffer of config->btf_argN[i]->offset by iteration. The goal is to reach the requiered data by overwriting over arg with the new value. The `is_pointer` field is used to `probe_read` the current field to access the data. If this is false, this means the field does not need such a thing. For instance : - When accessing `linux_binprm->filename` the `filename` field is a pointer, so this means we have to `probe_read` it. (https://elixir.bootlin.com/linux/v4.19.325/source/include/linux/binfmts.h#L57) - When accessing `task_struct->comm` it is not needed since `comm` is not a pointer (https://elixir.bootlin.com/linux/v6.13-rc3/source/include/linux/sched.h#L1138) So when the loop reach that point, in the first case it will first reach the offset and then `probe_read` and on the second, just reach the offset. Signed-off-by: Tristan d'Audibert <[email protected]>

This commit checks if the btf.Type exists in Tetragon existing types. This is useful if we don't want to set the field `type` in the policy. Though this function does not work for many cases For instance: `struct file` with btf is called `file` and also exists in `GenericStringToType` with the same alias. However, the `struct sk_buff` is called `sk_buff` in btf but `skb` in Tetragon. So this type will return `GenericInvalidType`. The same thing happened with `linux_binprm->filename` as the type is `char` and does not exist in Tetragon types. Signed-off-by: Tristan d'Audibert <[email protected]>

ResolveBtfPath function recursively searches in a btf structure in order to find a specific path until it reaches the target or fails. The function also searches in embedded anonymous structures or unions to cover as much use cases as possible. For instance, mm_struct has 2 fields; anonymous struct and another type. But you are still able to look into the anonymous struct by specifying a path like "mm.pgd.pgd". For instance, if the search is in the linux_binprm structure and the path is `file.f_path.dentry.d_name.name`, the following actions will be done. - Look for the variable name `file` inside `linux_binprm`. - If it matches, it stores the offset from linux_binprm where the `file` variable could be found. - Then it takes the btf type `file` and searches for a parameter named `f_path`. The `ResolveNestedTypes` function has for only purpose to bypass types we have no interests in. Since BTF types are nested, we may encounter those "useless" information that we need to ignore. Signed-off-by: Tristan d'Audibert <[email protected]>

In order to read the data properly on BPF side, integer/long values must use `bpf_probe_read`. So now, every time the latest type retrieved is defined as an integer, it will be safely read before accessing the data. Signed-off-by: Tristan d'Audibert <[email protected]>

I've noticed that unit tests load differently the btf file than locally. So when using `NewBTF()` locally it worked, but during the pipeline it crashes. To avoid this behaviour, `LoadBTF` is added and ensure the btf file path is set before trying to load it. Otherwise, it run the `InitCachedBtf()`. Signed-off-by: Tristan d'Audibert <[email protected]>

The function `FindBtfFuncParamFromHook` has been added to identify the exact BTF type of the hook's parameters. This allows us to handle cases where the hook has parameters that include multiple pointers to structures. With those information, we will be able to dereference correctly any parameters until we reach the data Signed-off-by: Tristan d'Audibert <[email protected]>

This commit adds a new parameter to give the ability to the user to search for a specific attribute following a "path" as follow: ```yml ... - hook: "bprm_check_security" args: - index: 0 # index 0 is struct linux_binprm type: "string" resolve: "file.f_path.dentry.d_name.name" ... ``` The above config can be used to extract a specific attribute from the structure at index 0. Signed-off-by: Tristan d'Audibert <[email protected]>

Searches if every user defined type with Resolve exists as a BTF type and stores its corresponding offset in ConfigBtfArg. This function does a basic split on `Resolve` to obtain the "path" to the required data. Then, the array is given to `btf.ResolveBtfPath` to find the offset of each element until we reach the required data. The output is stored in EventConfig to keep the normal behaviour. For example, if the arg 0 is `struct linux_binprm` and Resolve is set to `file.f_path.dentry.d_name.name`, the output will give an array of all the offsets from their parents as follows : [{ offset: 96, is_pointer: 0 }, { offset: 152, is_pointer: 1 }, ...] Signed-off-by: Tristan d'Audibert <[email protected]>

This commit updates `addLsm` function to use the new `arg.Resolve` option in order to look for the attributes in BTF structure. Signed-off-by: Tristan d'Audibert <[email protected]>

Since the Resolve attribute feature uses the hook name to retrieve the hook parameters, if the LSM hook does not exist inside the BTF file, it is not possible to resolve attributes correctly. The feature that allows this was added to the kernel in v5.8 with the patch below: https://lore.kernel.org/bpf/[email protected]/ After this patch, LSM hooks can be retrieved in the BTF file with this format: `bpf_lsm_<hook_name>` PS: The commit has been backported to kernel 5.7, you can check this by running the below command `git branch -r --contains 9d3fdea789c8fab51381c2d609932fabe94c0517` Signed-off-by: Tristan d'Audibert <[email protected]>

As BTF types are not defined for Uprobes, their offsets can't be found in BTF file. Thus, if the user defines Resolve, an error is returned Signed-off-by: Tristan d'Audibert <[email protected]>

Add very similar code as in `genericlsm.go` file to handle Resolve feature. Signed-off-by: Tristan d'Audibert <[email protected]>

…licy `TestResolveBtfArgFromKprobePolicy` is introduced. It aims to test the access to the requested attribute with no errors and assert that the very last type corresponds to the requested policy type (string/int/etc). 3 different cases are handled: - Test a classic hook - Test resolve from double pointer parameter - Assert an error is raised when the resolve path is too long Signed-off-by: Tristan d'Audibert <[email protected]>

This test ensures 4 things : - It can retrieve correctly the parameter of an kprobe hook. - Is raises an error when btf.Type could not be found - It raises an error when the type found is not a function. - It raises an error when the requested parameter index is out of range for the FuncProto.Params array. Signed-off-by: Tristan d'Audibert <[email protected]>

This commit adds 3 tests for ResolveBtfPath algorithm : - `testAssertEqualBtfPath` to assert that a specific path has the exact same btfConfig as expected. - `testAssertPathIsAccessible` tries to reach the path and asserts that no errors are raised. - `testAssertErrorOnInvalidPath` asserts that the error messages raised if the path is incorrect. The chosen test cases have embed union/anonymous structs. To Test locally : ``` go test -exec "sudo" ./pkg/btf/ ``` Signed-off-by: Tristan d'Audibert <[email protected]>

Signed-off-by: Tristan d'Audibert <[email protected]>

ScriptSathi · 2025-02-11T18:39:13Z

I rebased from main to fix the CI issues after GitHub update (resolved with #3387), but it seems veristat tests are still in error because of that (https://github.com/cilium/tetragon/actions/runs/13269998947/job/37046941582?pr=3143 and https://github.com/cilium/tetragon/actions/runs/13269998979/job/37046941723?pr=3143)

ScriptSathi requested a review from a team as a code owner November 19, 2024 16:27

ScriptSathi requested a review from jrfastab November 19, 2024 16:27

ScriptSathi marked this pull request as draft November 19, 2024 16:28

ScriptSathi force-pushed the pr/ScriptSathi/add-dynamic-paramter-extraction branch from 9a9d21f to ce33ec7 Compare November 20, 2024 10:48

ScriptSathi force-pushed the pr/ScriptSathi/add-dynamic-paramter-extraction branch from ce33ec7 to 5ebb3e0 Compare November 20, 2024 16:00

mtardy linked an issue Nov 21, 2024 that may be closed by this pull request

Add dynamic parameter extration for specific use cases #3142

Open

2 tasks

mtardy added the release-note/major This PR introduces major new functionality label Nov 21, 2024

ScriptSathi force-pushed the pr/ScriptSathi/add-dynamic-paramter-extraction branch 2 times, most recently from e3117d7 to ba048b3 Compare November 21, 2024 10:59

olsajiri reviewed Nov 22, 2024

View reviewed changes

ScriptSathi force-pushed the pr/ScriptSathi/add-dynamic-paramter-extraction branch from ba048b3 to 1fb7dbc Compare November 23, 2024 17:54

pchaigno removed the request for review from jrfastab December 9, 2024 18:05

ScriptSathi force-pushed the pr/ScriptSathi/add-dynamic-paramter-extraction branch 2 times, most recently from 753fd10 to 2b1153f Compare January 10, 2025 14:12

ScriptSathi changed the title ~~Add dynamic parameter extraction~~ Add dynamic extraction of a parameter attribute Jan 10, 2025

ScriptSathi force-pushed the pr/ScriptSathi/add-dynamic-paramter-extraction branch from 2b1153f to a04e634 Compare January 10, 2025 16:36

ScriptSathi marked this pull request as ready for review January 10, 2025 16:38

ScriptSathi requested a review from mtardy as a code owner January 10, 2025 16:38

ScriptSathi force-pushed the pr/ScriptSathi/add-dynamic-paramter-extraction branch 2 times, most recently from 058c828 to 391c5a8 Compare January 10, 2025 18:32

kkourt self-requested a review January 14, 2025 14:55

olsajiri requested changes Jan 15, 2025

View reviewed changes

mtardy reviewed Jan 20, 2025

View reviewed changes

ScriptSathi force-pushed the pr/ScriptSathi/add-dynamic-paramter-extraction branch from 391c5a8 to dac4d2e Compare January 23, 2025 13:51

ScriptSathi force-pushed the pr/ScriptSathi/add-dynamic-paramter-extraction branch 4 times, most recently from e279770 to 3a85916 Compare January 26, 2025 17:00

kkourt reviewed Feb 10, 2025

View reviewed changes

mtardy approved these changes Feb 10, 2025

View reviewed changes

ScriptSathi force-pushed the pr/ScriptSathi/add-dynamic-paramter-extraction branch 2 times, most recently from 9d7f6b6 to 3624cef Compare February 10, 2025 15:09

kkourt mentioned this pull request Feb 11, 2025

Unexpected vmtest fail on 4.19 #3384

Closed

ScriptSathi force-pushed the pr/ScriptSathi/add-dynamic-paramter-extraction branch 2 times, most recently from 80f792c to 4f714e2 Compare February 11, 2025 16:45

ScriptSathi requested a review from kkourt February 11, 2025 17:00

ScriptSathi added 18 commits February 11, 2025 19:31

[genericlsm.go] Use arg.Resolve to build ConfigBtfArg for LSM

ff03267

This commit updates `addLsm` function to use the new `arg.Resolve` option in order to look for the attributes in BTF structure. Signed-off-by: Tristan d'Audibert <[email protected]>

[genericuprobe.go] Add exception for arg.Resolve for uprobes

d4bd83c

As BTF types are not defined for Uprobes, their offsets can't be found in BTF file. Thus, if the user defines Resolve, an error is returned Signed-off-by: Tristan d'Audibert <[email protected]>

[generickprobe.go] Add arg.Resolve usage for kprobes

6f8318c

Add very similar code as in `genericlsm.go` file to handle Resolve feature. Signed-off-by: Tristan d'Audibert <[email protected]>

Add new tracingpolicy example for dynamic parameter extraction feature

d290424

Signed-off-by: Tristan d'Audibert <[email protected]>

Add documentation for arg.Resolve feature

41783b6

Signed-off-by: Tristan d'Audibert <[email protected]>

ScriptSathi force-pushed the pr/ScriptSathi/add-dynamic-paramter-extraction branch from 4f714e2 to 41783b6 Compare February 11, 2025 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dynamic extraction of a parameter attribute #3143

Add dynamic extraction of a parameter attribute #3143

ScriptSathi commented Nov 19, 2024 •

edited

Loading

netlify bot commented Nov 20, 2024 •

edited

Loading

olsajiri left a comment

olsajiri left a comment

mtardy left a comment

ScriptSathi commented Jan 23, 2025 •

edited

Loading

bentekkie commented Feb 6, 2025

olsajiri commented Feb 7, 2025

kkourt left a comment

kkourt Feb 10, 2025

kkourt Feb 10, 2025

kkourt Feb 10, 2025

kkourt Feb 10, 2025

kkourt Feb 10, 2025

kkourt Feb 10, 2025

mtardy left a comment

ScriptSathi commented Feb 11, 2025

		const EventConfigMaxArgs = 5
		const MaxBtfArgDepth = 10 // Artificial value for compilation, may be extended

Add dynamic extraction of a parameter attribute #3143

Are you sure you want to change the base?

Add dynamic extraction of a parameter attribute #3143

Conversation

ScriptSathi commented Nov 19, 2024 • edited Loading

Description

Test the PR

Release-note

netlify bot commented Nov 20, 2024 • edited Loading

✅ Deploy Preview for tetragon ready!

olsajiri left a comment

Choose a reason for hiding this comment

olsajiri left a comment

Choose a reason for hiding this comment

mtardy left a comment

Choose a reason for hiding this comment

ScriptSathi commented Jan 23, 2025 • edited Loading

bentekkie commented Feb 6, 2025

olsajiri commented Feb 7, 2025

kkourt left a comment

Choose a reason for hiding this comment

kkourt Feb 10, 2025

Choose a reason for hiding this comment

kkourt Feb 10, 2025

Choose a reason for hiding this comment

kkourt Feb 10, 2025

Choose a reason for hiding this comment

kkourt Feb 10, 2025

Choose a reason for hiding this comment

kkourt Feb 10, 2025

Choose a reason for hiding this comment

kkourt Feb 10, 2025

Choose a reason for hiding this comment

mtardy left a comment

Choose a reason for hiding this comment

ScriptSathi commented Feb 11, 2025

ScriptSathi commented Nov 19, 2024 •

edited

Loading

netlify bot commented Nov 20, 2024 •

edited

Loading

ScriptSathi commented Jan 23, 2025 •

edited

Loading