kprobes: support having the same argument twice

[kkourt]

If we write a policy with the same argument twice,

apiVersion: cilium.io/v1alpha1
kind: TracingPolicy
metadata:
  name: "sys-lseek"
spec:
  kprobes:
  - call: "sys_lseek"
    syscall: true
    args:
    - index: 0
      type: "int"
      label: "index 0"
    - index: 0
      type: "int"
      label: "index 0 (again)"

The agent will accept it, and then produce a wrong event. For example, if we execute:

echo "100 100 100" | ./contrib/tester-progs/lseek-pipe

We will get:

[
  {
    "int_arg": 100,
    "label": "index 0"
  },
  {
    "int_arg": 0,
    "label": "index 0 (again)"
  }
]

And a warning in the logs:

time="2025-05-07T13:46:20+02:00" level=warning msg="Int type error" arg.usertype= error=EOF

Note that accessing the same argument twice is useful when used with "resolve:". For example:

apiVersion: cilium.io/v1alpha1
kind: TracingPolicy
metadata:
  name: "lsm"
spec:
  kprobes:
  - call: "security_bprm_check"
    syscall: false
    args:
      - index: 0
        type: "string"
        resolve: "mm.owner.comm"
        label: "proc"
      - index: 0
        type: "string"
        resolve: "mm.owner.real_parent.comm"
        label: "parent"
      - index: 0
        type: "string"
        resolve: "mm.owner.real_parent.real_parent.comm"
        label: "grand-parent"
    selectors:
      - matchActions:
        - action: Post

I'm marking this as a bug because we provide the wrong data to the user and we should (at minimum) reject the policy. The best solution, however, is to support multiple arguments on the same index (which, arguably, is a new feature).

See also: #3710

[olsajiri]

yes, with the resolve it makes sense now to allow to retrieve multiple argument values for single argument, which is new feature

but current code should fail for such spec now, will fix, thanks

[tdaudi]

Hello ! Super glad this is part of the roadmap.
I've been working on this feature few months ago, but it stayed as a WIP and was not ready for upstream. The final policy look like this. I acknowledge that "duplicateIndex" is not very user-friendly, but it allows an easier reuse in the matchArgs.

@olsajiri If you have not started development on this yet, I'll be happy to continue pushing it.

apiVersion: cilium.io/v1alpha1
kind: TracingPolicy
metadata:
  name: "deny-flag-access"
spec:
  lsmhooks:
  - hook: "file_open"
    args:
    - index: 0
      type: "file"
    - index: 1
      type: "int"
      duplicateIndex: 0
      resolve: "f_mode"
    selectors:
    - matchActions:
      - action: Post
      - action: Override
        argError: -1
      matchArgs:
      - index: 0
        operator: "Postfix"
        values:
        - "flag.txt"
      - index: 1
        operator: "Mask"
        values:
          - "1" #FMODE_READ
          - "2" #FMODE_WRITE

[kkourt]

Hello ! Super glad this is part of the roadmap.
I've been working on this feature few months ago, but it stayed as a WIP and was not ready for upstream. The final policy look like this. I acknowledge that "duplicateIndex" is not very user-friendly, but it allows an easier reuse in the matchArgs.

Cool!

I think the first thing we need to figure out is what the "index: " means. Does it mean the index in the function or does it mean the index in the arguments we attach to the events. See also #3710.

I would argue that index (in the args:) should mean the index in the function, and the index in the arguments we attach should just be implicit by the order of the arguments we have. In that case, it should be possible to have the same argument twice.

[tdaudi]

I would argue that index (in the args:) should mean the index in the function, and the index in the arguments we attach should just be implicit by the order of the arguments we have. In that case, it should be possible to have the same argument twice.

Yes, I like this idea. To do so, we can use the BTF file to build the first index, and use label to build an matchArg index implicitly.

The downside is that the resolve change is breaking change

apiVersion: cilium.io/v1alpha1
kind: TracingPolicy
metadata:
  name: "lsm"
spec:
  kprobes:
  - call: "security_bprm_check"
    syscall: false
    args:
    - resolve: "bprm.mm.owner.comm"
      type: "string"
      label: "proc"
    - resolve: "bprm.mm.owner.real_parent.comm"
      type: "string"
      label: "parent"
    selectors:
      - matchActions:
        - action: Post
        matchArgs:
         - label: "proc"
           operator: "Equal"
           values:
           - "some process"
         - label: "parent"
           operator: "Equal"
           values:
           - "some parent"

To avoid multiple changes, we can use the policy args array index (proc label would be 0, parent would be 1, ...) as the actual index (from 1 to 5) to reference the label and "hide" this complexity to the user. And on bpf side, we add a new param to the event_config like this __u8 arg_idx[EVENT_CONFIG_MAX_ARG];. So Tetragon workflow will not change and iterate from 1 to 5, and we can easily compare things afterward.

[olsajiri]

@olsajiri If you have not started development on this yet, I'll be happy to continue pushing it.

no problem with that, go ahead ;-)

I took the issue because I'd like to push some speed up changes I have in the queue for the argument processing and wanted to avoid conflicts.. would be great if you could wait for next week and base on my changes, but if not we'll figure it out anyway

thanks

[kkourt]

    - resolve: "bprm.mm.owner.real_parent.comm"
      type: "string"
      label: "parent"

I believe we still need the index to specify which of the function arguments we are referring to. Or am I missing something?

[tdaudi]

We can get the argument index from the btf.FuncParam type. In the code below, we specify the index to get the BTF type, but we simply need to change this a bit and loop over all params that matches bprm and get the index.

tetragon/pkg/btf/btf.go

Line 133 in 0921cdf

return &btfHookProto.Params[argIndex], nil

The only thing I did not figure out yet is how I identify the label in args and bind this implicit index to use it next on the bpf side for matchArg

[kkourt]

We can get the argument index from the btf.FuncParam type. In the code below, we specify the index to get the BTF type, but we simply need to change this a bit and loop over all params that matches bprm and get the index.

tetragon/pkg/btf/btf.go

Line 133 in 0921cdf
return &btfHookProto.Params[argIndex], nil

The only thing I did not figure out yet is how I identify the label in args and bind this implicit index to use it next on the bpf side for matchArg

I'm not sure if the type is enough to identify the argument. There could be two arguments with the same type no?

[tdaudi]

I was more expecting to use the parameter name to identify the type because there can only have a single parameter with the same name.

int security_bprm_check(struct linux_binprm *bprm)

So in this case, the user will identify struct linux_binprm type by using bprm.

FuncParam have an attribute Name which is equal to bprm and an attribute Type which points to the btf type of struct linux_binprm
https://pkg.go.dev/github.com/cilium/ebpf/internal/btf#FuncParam

---

But maybe the policy should look like this.

    - param: "bprm"
      resolve: "mm.owner.real_parent.comm"
      type: "string"
      label: "parent"

And the major downside would be that we should deprecate LSM usage for kernel < 5.4 because the hooks does not exist in the BTF file at this point.
Or maybe we should ask the user to specify the index explicitly, in this case

[kkourt]

I was more expecting to use the parameter name to identify the type because there can only have a single parameter with the same name.

Ah I see. A potential problem here is that the parameter name can change across different kernel versions, and this btf files. That being said, I think it would be defintely useful to have a way to refer to function parameters by name.

I also see this somewhat orthogonal in supporting argument entries that refer to the same argument.

I think there are two different questions:

• how do we refer to an argument in the elements of the args: list?
• how do we refer to an argument in the matchArgs: section

In other words, in the args: section we need to refer to the function arguments, and in the selector we need to refer to the selector section arguments.

For example, we could refer to function arguments by index, and to selector section arguments by label:

    args:
      - index: 0             # first argument from the function
        type: "string"
        resolve: "mm.owner.comm"
        label: "proc"
      - index: 0             # first argument from the function
        type: "string"
        resolve: "mm.owner.real_parent.comm"
        label: "parent"
    selectors:
      - matchActions:
        - action: Post
        matchArgs:
         - label: "proc"         # match the argument from args: with label "proc"
           operator: "Equal"
           values:
           - "some process"
         - label: "parent"     # match the argument from args: with label "parent'
           operator: "Equal"
           values:
           - "some parent"

We could also refer to function arguments by index, and to selector section arguments by index:

    args:
      - index: 0             # first argument from the function
        type: "string"
        resolve: "mm.owner.comm"
        label: "proc"
      - index: 0             # first argument from the function
        type: "string"
        resolve: "mm.owner.real_parent.comm"
        label: "parent"
    selectors:
      - matchActions:
        - action: Post
        matchArgs:
         - index: 0         # first argument in the args section (proc)
           operator: "Equal"
           values:
           - "some process"
         - index: 1          # second argument in the args section (parent)
           operator: "Equal"
           values:
           - "some parent"

Since we already have args[].index and matchArgs.index, the above seems like a sane way to interpret the values that we have now in the spec. I'm not sure it breaks current behaviour, but I think it's worth considering as an option. Note, that this option does not exclude adding a way to refer to function parameters by parameter name in args:, and a way to refer to args: arguments by label in the matchArgs section.

[tdaudi]

I was more expecting to use the parameter name to identify the type because there can only have a single parameter with the same name.

Ah I see. A potential problem here is that the parameter name can change across different kernel versions, and this btf files. That being said, I think it would be defintely useful to have a way to refer to function parameters by name.

I agree, but the resolve flag already comes with this kind of issue. For example, in struct dentry the attribute d_iname was removed between 6.13.13 and 6.14-rc1.
That said, users who are familiar with kernel code and write policies based on it would probably expect to revisit those policies when updating the kernel. For instance, if a kprobe is renamed or removed, or if parameter indexes change.
Maybe it could still be helpful to keep the args: index and let users choose whether they prefer referring to parameters by name or by position?

I also see this somewhat orthogonal in supporting argument entries that refer to the same argument.

I think there are two different questions:

* how do we refer to an argument in the elements of the `args:` list?

* how do we refer to an argument in the `matchArgs:` section

In other words, in the args: section we need to refer to the function arguments, and in the selector we need to refer to the selector section arguments.

[...]
Since we already have args[].index and matchArgs.index, the above seems like a sane way to interpret the values that we have now in the spec. I'm not sure it breaks current behaviour, but I think it's worth considering as an option. Note, that this option does not exclude adding a way to refer to function parameters by parameter name in args:, and a way to refer to args: arguments by label in the matchArgs section.

The second option feels a bit confusing to me, as it requires users to distinguish between the parameter index and the position of the variable they defined in the args: list.
Using a label seems more intuitive and makes the policy easier to read and reason about.

[kkourt]

Maybe it could still be helpful to keep the args: index and let users choose whether they prefer referring to parameters by name or by position?

Yap, I agree (and I guess this was where I was getting). Especially since we already have the index field there. There is no reason why we cannot support both.

Since we already have args[].index and matchArgs.index, the above seems like a sane way to interpret the values that we have now in the spec. I'm not sure it breaks current behaviour, but I think it's worth considering as an option. Note, that this option does not exclude adding a way to refer to function parameters by parameter name in args:, and a way to refer to args: arguments by label in the matchArgs section.

The second option feels a bit confusing to me, as it requires users to distinguish between the parameter index and the position of the variable they defined in the args: list. Using a label seems more intuitive and makes the policy easier to read and reason about.

The labels are easier to understand -- no arguments there. (Although, I'm not a huge fan the label name, but that's a different discussion).

But we already have the index field and we still need to have old policies keep working.
Given that, having a way to reference arguments by index (in addition to referencing them by labels) seems worthwhile to me.