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

bpf: fmodret override on security_ hooks is available from 5.7 #1349

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

tixxdz
Copy link
Member

@tixxdz tixxdz commented Aug 10, 2023

The override on security_ hook is available starting from 5.7, so check before loading the policy.

https://lore.kernel.org/all/[email protected]/

@tixxdz tixxdz requested a review from a team as a code owner August 10, 2023 15:44
@netlify
Copy link

netlify bot commented Aug 10, 2023

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit 83b4075
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/64df435848b3270008afd31f
😎 Deploy Preview https://deploy-preview-1349--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link
Contributor

@olsajiri olsajiri left a comment

Choose a reason for hiding this comment

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

looks good, could that be done with some feature check as well? I'd think Redhat kernel might have this backported even if the kernel version is 4.18.. but we can start with this for sure

@kkourt kkourt closed this Aug 17, 2023
@kkourt kkourt reopened this Aug 17, 2023
@kkourt
Copy link
Contributor

kkourt commented Aug 17, 2023

looks good, could that be done with some feature check as well? I'd think Redhat kernel might have this backported even if the kernel version is 4.18.. but we can start with this for sure

Agreed.
What would that feature check look like? I guess we could try and loading a program and see if it works?

// https://lore.kernel.org/all/[email protected]/
if !f.Syscall &&
(strings.HasPrefix(f.Call, "security_") == false || !kernels.MinKernelVersion("5.7")) {
return fmt.Errorf("Error override action can be used only with syscalls or on security_ hooks with a minimum kernel version 5.7")
Copy link
Contributor

Choose a reason for hiding this comment

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

Until we have a feature check, would it make sense to have a flag so that we can ignore the kernel check here? As @olsajiri said, some vendor kernels backport many patches and the kernel version does not necessarily reflect the feature set of the kernel, and it would be nice to have a way to override the kernel check.

We can leave this as a follow up though.

@tixxdz
Copy link
Member Author

tixxdz commented Aug 17, 2023

looks good, could that be done with some feature check as well? I'd think Redhat kernel might have this backported even if the kernel version is 4.18.. but we can start with this for sure

Agreed. What would that feature check look like? I guess we could try and loading a program and see if it works?

Oh I see redhat kernels, so in that case I will convert this patch to be: document where override applies syscalls, security_ hooks with the corresponding kernels version or patch, and let it fail for other cases. Thoughts?

@kkourt
Copy link
Contributor

kkourt commented Aug 17, 2023

looks good, could that be done with some feature check as well? I'd think Redhat kernel might have this backported even if the kernel version is 4.18.. but we can start with this for sure

Agreed. What would that feature check look like? I guess we could try and loading a program and see if it works?

Oh I see redhat kernels, so in that case I will convert this patch to be: document where override applies syscalls, security_ hooks with the corresponding kernels version or patch, and let it fail for other cases. Thoughts?

You mean not have the kernel check? Yes, I think that's fine from my side.

@tixxdz tixxdz force-pushed the pr/tixxdz/fmode_ret_security_kernel_5.7 branch from 19b6437 to d102401 Compare August 18, 2023 09:59
@tixxdz tixxdz requested a review from mtardy as a code owner August 18, 2023 09:59
@tixxdz tixxdz force-pushed the pr/tixxdz/fmode_ret_security_kernel_5.7 branch 3 times, most recently from f6b3487 to 83b4075 Compare August 18, 2023 10:09
Let's improve override action documentation and try to be precise
on security_ hooks overriding being possible starting from kernel
5.7

If some distro has backported the patch to allow fmod_ret override
on security_ hooks then all good, otherwise if it fails we are covered
by the documentation.

Also change the caution point to be more addressed to kernel developers
if they want to leverage tetragon kprobe capabilities and use error
injections, as they should get it right in the first place.

Rerefence:  https://lore.kernel.org/all/[email protected]/

Signed-off-by: Djalal Harouni <[email protected]>
@tixxdz tixxdz force-pushed the pr/tixxdz/fmode_ret_security_kernel_5.7 branch from 83b4075 to 4b33adc Compare August 18, 2023 10:12
@tixxdz
Copy link
Member Author

tixxdz commented Aug 18, 2023

looks good, could that be done with some feature check as well? I'd think Redhat kernel might have this backported even if the kernel version is 4.18.. but we can start with this for sure

Agreed. What would that feature check look like? I guess we could try and loading a program and see if it works?

Oh I see redhat kernels, so in that case I will convert this patch to be: document where override applies syscalls, security_ hooks with the corresponding kernels version or patch, and let it fail for other cases. Thoughts?

You mean not have the kernel check? Yes, I think that's fine from my side.

Yes, improved the doc. If no objection will merge it later

@tixxdz tixxdz merged commit 196c2d0 into main Aug 18, 2023
5 checks passed
@tixxdz tixxdz deleted the pr/tixxdz/fmode_ret_security_kernel_5.7 branch August 18, 2023 16:41
@mtardy
Copy link
Member

mtardy commented Aug 21, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants