IMA hashes in LSM events

[anfedotoff]

Adding support for collecting file hashes calculated by Linux integrity subsystem (IMA). IMA hashes will be contained in LSM events. To make it work you need to:

• enable LSM BPF
• enable IMA

Limitations of bpf_ima helpers (from #2409):

1. bpf_ima_inode_hash/bpf_ima_file_hash can be called only from BPF_F_SLEEPABLE lsm programs: lsm.s.
2. lsm.s programs are strictly limited for maps usage. Only arrays, hashes and ringbuffer are allowed. We can't use BPF_MAP_TYPE_PROG_ARRAY and BPF_MAP_TYPE_PERF_EVENT_ARRAY. It means for us that we can't use bpf-to-bpf calls from lsm.s programs and send events from lsm.s programs with perfbuffer.
3. only trusted pointers are allowed to call bpf_ima helpers. Some verifier error example:
   38: (85) call bpf_ima_file_hash#193 R1 type=scalar expected=ptr_, trusted_ptr.

Approach from #2409 is meant to overcome limitations above has a race condition. User mode handler can receive LSM event before IMA hash is stored to the separate map.

This approach was redesigned to get rid of race condition. The idea is follows. We split LSM generic sensor into three parts, which are loaded using bpf trampolines feature.

1. bpf_generic_lsm_core does everything but sending LSM event. It also initializes an empty ima_hash, which is used in later trampoline calls.
2. bpf_generic_lsm_ima_* (optional) is triggered after bpf_generic_lsm_core, calculates IMA hash and stores it in separate hash map.
3. bpf_generic_lsm_output just sends event using perf buffer. Also at this stage IMA hash is collected from the map (stored in p. 2).

Using this scheme we can avoid race condition.
Note: if we don't need to collect IMA hash, step 2 is skipped.

[netlify]

✅ Deploy Preview for tetragon ready!

Name	Link
🔨 Latest commit	d5c5ec2
🔍 Latest deploy log	https://app.netlify.com/sites/tetragon/deploys/66feb22fa7347900086e433f
😎 Deploy Preview	https://deploy-preview-2818--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.

[anfedotoff]

@kkourt , hi! May I ask you for a code review? Next Monday, on Tetragon community meeting I'll show a demo. I want to talk about some design decisions I made. Also I'm going to speak about problems that I overcame and those are still exists.

[kkourt]

@kkourt , hi! May I ask you for a code review? Next Monday, on Tetragon community meeting I'll show a demo. I want to talk about some design decisions I made. Also I'm going to speak about problems that I overcame and those are still exists.

Hello :)

Thanks for the heads up! Will try and have a look before the community meeting. Looking forward to the demo!

[kkourt]

Thanks @anfedotoff! I had a first look. Please find some comments below.

[olsajiri]

I refreshed on LSM programs and I think we can do this differently

so LSM programs are attached through trampolines on specific bpf_lsm_XXX
function which is invoked as part of the lsm modules call chain on particular
lsm call back

and trampoline allows to attach and invoke multiple bpf programs to
specific function (or lsm hook)

so I think we could have the sleepable lsm hooks invoked first to get the
hash and update the map and then we will have generic lsm invoked that
would run the hash action, which would get the hash from the map and
send the event

  bpf_lsm_XXX
    -> trampoline
         run lsm.s/file_open
               map_update_elem(&ima_hash_map, &e->current, &hash, BPF_ANY);
         run lsm/generic_lsm
               map_lookup_elem(&ima_hash_map, &e->current, ...)

to ensure the generic lsm is invoked AFTER lsm.s/XXX hook we can rely
on kernel using standard lists when queuing programs for trampoline,
so you should just attach lsm.s hook first and then generic lsm

I guess there might be some hiccups when you'll implement that
(there always are) but so far it seems straightforward

WDYT?

[anfedotoff]

I refreshed on LSM programs and I think we can do this differently

so LSM programs are attached through trampolines on specific bpf_lsm_XXX function which is invoked as part of the lsm modules call chain on particular lsm call back

and trampoline allows to attach and invoke multiple bpf programs to specific function (or lsm hook)

so I think we could have the sleepable lsm hooks invoked first to get the hash and update the map and then we will have generic lsm invoked that would run the hash action, which would get the hash from the map and send the event

  bpf_lsm_XXX
    -> trampoline
         run lsm.s/file_open
               map_update_elem(&ima_hash_map, &e->current, &hash, BPF_ANY);
         run lsm/generic_lsm
               map_lookup_elem(&ima_hash_map, &e->current, ...)

to ensure the generic lsm is invoked AFTER lsm.s/XXX hook we can rely on kernel using standard lists when queuing programs for trampoline, so you should just attach lsm.s hook first and then generic lsm

I guess there might be some hiccups when you'll implement that (there always are) but so far it seems straightforward

WDYT?

Thanks for making it clear!
IIUC, we will calculate hash on every triggered hook. It might cause some performance issues. Now our calls of bpf_ima_file_hash are filtered by tracing policy. When we move lsm.s/ima_* programe to the top, we will call helper no matter if tracing policy is satisfied or not. I think, it will be OK, if ima_policy is satisfied. Let's consider the following example:

We have a default ima_policy=tcb:

measure func=MMAP_CHECK mask=MAY_EXEC
measure func=BPRM_CHECK mask=MAY_EXEC # binary executed
measure func=FILE_CHECK mask=^MAY_READ euid=0
measure func=FILE_CHECK mask=^MAY_READ uid=0 # root opened r/o, r/w
measure func=MODULE_CHECK
measure func=FIRMWARE_CHECK
measure func=POLICY_CHECK

In particular, this ima_policy will measure hashes when files owned by root are opened for reading. So, in solution that you proposed will call bpf_ima_file_hash on every hook. The question is what helper will return, if ima_policy is not satisfied? If error, than it is OK, I think. But if it calculates hash despite the ima_policy it is not OK. I think, I can check this.

To sum up, I think, you solution is a nice compromise if ima_policy is taking to account during bpf_ima_file_hash call. What do you think about this?

[anfedotoff]

It works! No race condition! ✨

[olsajiri]

nice! left some comment, thanks

[olsajiri]

leeks great, thanks

[kkourt]

Thanks, this looks good to me overall!

I have some requests for changes. Please let me know if they do not make sense to you. I have not followed the discussion with Jiri. Can you please add a summary of the discussion in the PR header. Having a discussion of what the previous approach was, why it did not work, and what this PR does that works would be very helpful!

Thanks!

[anfedotoff]

Thanks, this looks good to me overall!

I have some requests for changes. Please let me know if they do not make sense to you. I have not followed the discussion with Jiri. Can you please add a summary of the discussion in the PR header. Having a discussion of what the previous approach was, why it did not work, and what this PR does that works would be very helpful!

Thanks!

Sure! I updated the PR description. I hope, now it became more clear about what we've done to avoid the race condition.

[kkourt]

Thanks, this looks good to me overall!
I have some requests for changes. Please let me know if they do not make sense to you. I have not followed the discussion with Jiri. Can you please add a summary of the discussion in the PR header. Having a discussion of what the previous approach was, why it did not work, and what this PR does that works would be very helpful!
Thanks!

Sure! I updated the PR description. I hope, now it became more clear about what we've done to avoid the race condition.

Great Thanks! Can you please split the change that changes the return value into a separate commit with a short explanation of why it OK to do as discussed in another comment? Other than that, LGTM!

[anfedotoff]

Thanks, this looks good to me overall!
I have some requests for changes. Please let me know if they do not make sense to you. I have not followed the discussion with Jiri. Can you please add a summary of the discussion in the PR header. Having a discussion of what the previous approach was, why it did not work, and what this PR does that works would be very helpful!
Thanks!

Sure! I updated the PR description. I hope, now it became more clear about what we've done to avoid the race condition.

Great Thanks! Can you please split the change that changes the return value into a separate commit with a short explanation of why it OK to do as discussed in another comment? Other than that, LGTM!

@kkourt, can you look one more time plz)?

[kkourt]

Many thanks!

[kkourt]

All tests ✅, merging! Thanks!