-
Notifications
You must be signed in to change notification settings - Fork 4
ROX-30296: track POSIX ACL changes via inode_set_acl LSM hook #878
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
base: main
Are you sure you want to change the base?
Changes from all commits
60ab038
19a175a
3e30921
ffcdc1d
fff48a3
5a64ed2
198156f
be7fb22
1dc7ae0
fc73ed7
516433e
4be488a
77c7d4a
df19623
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,6 +4,7 @@ use log::debug; | |||||||||||||||||
|
|
||||||||||||||||||
| pub(super) struct Checks { | ||||||||||||||||||
| pub(super) path_hooks_support_bpf_d_path: bool, | ||||||||||||||||||
| pub(super) supports_inode_set_acl: bool, | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| impl Checks { | ||||||||||||||||||
|
|
@@ -12,15 +13,37 @@ impl Checks { | |||||||||||||||||
| .load(fact_ebpf::CHECKS_OBJ) | ||||||||||||||||||
| .context("Failed to load checks.o")?; | ||||||||||||||||||
|
|
||||||||||||||||||
| let prog = obj | ||||||||||||||||||
| .program_mut("check_path_unlink_supports_bpf_d_path") | ||||||||||||||||||
| .context("Failed to find 'check_path_unlink_supports_bpf_d_path' program")?; | ||||||||||||||||||
| let prog: &mut Lsm = prog.try_into()?; | ||||||||||||||||||
| let path_hooks_support_bpf_d_path = prog.load("path_unlink", btf).is_ok(); | ||||||||||||||||||
| debug!("path_unlink_supports_bpf_d_path: {path_hooks_support_bpf_d_path}"); | ||||||||||||||||||
| let path_hooks_support_bpf_d_path = Self::probe_hook( | ||||||||||||||||||
| &mut obj, | ||||||||||||||||||
| "check_path_unlink_supports_bpf_d_path", | ||||||||||||||||||
| "path_unlink", | ||||||||||||||||||
| btf, | ||||||||||||||||||
| ); | ||||||||||||||||||
| debug!("path_hooks_support_bpf_d_path: {path_hooks_support_bpf_d_path}"); | ||||||||||||||||||
|
|
||||||||||||||||||
| let supports_inode_set_acl = | ||||||||||||||||||
| Self::probe_hook(&mut obj, "check_inode_set_acl", "inode_set_acl", btf); | ||||||||||||||||||
| debug!("supports_inode_set_acl: {supports_inode_set_acl}"); | ||||||||||||||||||
|
|
||||||||||||||||||
| Ok(Checks { | ||||||||||||||||||
| path_hooks_support_bpf_d_path, | ||||||||||||||||||
| supports_inode_set_acl, | ||||||||||||||||||
| }) | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| fn probe_hook(obj: &mut aya::Ebpf, prog_name: &str, hook: &str, btf: &Btf) -> bool { | ||||||||||||||||||
| let Some(prog) = obj.program_mut(prog_name) else { | ||||||||||||||||||
| return false; | ||||||||||||||||||
| }; | ||||||||||||||||||
| let Ok(prog): Result<&mut Lsm, _> = prog.try_into() else { | ||||||||||||||||||
| return false; | ||||||||||||||||||
| }; | ||||||||||||||||||
| match prog.load(hook, btf) { | ||||||||||||||||||
| Ok(()) => true, | ||||||||||||||||||
| Err(e) => { | ||||||||||||||||||
| debug!("Probe {prog_name} failed to load for hook {hook}: {e:#}"); | ||||||||||||||||||
| false | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+41
to
+47
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nitpick] AFAICT, the previous 2 checks in this method should never fail (the first one checks the program exist and we use hardcoded strings for that, second one check the program is of type Lsm which is the only type of program we have), I would simplify this block and just let the upper level log the result.
Suggested change
|
||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,7 @@ const RINGBUFFER_NAME: &str = "rb"; | |
|
|
||
| pub struct Bpf { | ||
| obj: Ebpf, | ||
| checks: Checks, | ||
|
|
||
| tx: mpsc::Sender<Event>, | ||
|
|
||
|
|
@@ -64,6 +65,7 @@ impl Bpf { | |
| let paths = Vec::new(); | ||
| let mut bpf = Bpf { | ||
| obj, | ||
| checks, | ||
| tx, | ||
| paths, | ||
| paths_config, | ||
|
|
@@ -178,28 +180,39 @@ impl Bpf { | |
| let Some(hook) = name.strip_prefix("trace_") else { | ||
| bail!("Invalid hook name: {name}"); | ||
| }; | ||
|
|
||
| // Skip hooks that the kernel doesn't support | ||
| if hook == "inode_set_acl" && !self.checks.supports_inode_set_acl { | ||
| info!("Skipping {hook}: not supported on this kernel"); | ||
| continue; | ||
| } | ||
|
|
||
| match prog { | ||
| Program::Lsm(prog) => prog.load(hook, btf)?, | ||
| u => unimplemented!("{u:?}"), | ||
| } | ||
| }; | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Attaches all BPF programs. If any attach fails, all previously | ||
| /// attached programs are automatically detached via drop. | ||
| /// Attaches all loaded BPF programs. Programs that were not loaded | ||
| /// (e.g. optional hooks on unsupported kernels) are skipped. | ||
| /// If any attach fails, all previously attached programs are | ||
| /// automatically detached via drop. | ||
|
Comment on lines
+200
to
+201
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now I think about it this is no longer true. With the previous approach I think failing to attach causes fact to stop altogether, so it is not a big deal, but we should update the comment or use an intermediate event to mimic the previous behaviour, up to you. |
||
| fn attach_progs(&mut self) -> anyhow::Result<()> { | ||
| self.links = self | ||
| .obj | ||
| .programs_mut() | ||
| .map(|(_, prog)| match prog { | ||
| self.links.clear(); | ||
| for (_, prog) in self.obj.programs_mut() { | ||
| match prog { | ||
| Program::Lsm(prog) => { | ||
| if prog.fd().is_err() { | ||
| continue; | ||
| } | ||
| let link_id = prog.attach()?; | ||
| prog.take_link(link_id) | ||
| self.links.push(prog.take_link(link_id)?); | ||
| } | ||
| u => unimplemented!("{u:?}"), | ||
| }) | ||
| .collect::<Result<_, _>>()?; | ||
| } | ||
| } | ||
|
Comment on lines
+204
to
+215
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new approach is fine, but if you wanted to keep the functional one you could do so using self.links = self
.obj
.programs_mut()
.filter_map(|(_, prog)| match prog {
Program::Lsm(prog) => {
if prog.fd().is_err() {
return None;
}
let link_id = match prog.attach() {
Ok(link_id) => link_id,
Err(e) => return Some(Err(e)),
};
Some(prog.take_link(link_id))
}
u => unimplemented!("{u:?}"),
})
.collect::<Result<_, _>>()?;There is also one small change that I think won't matter, but the new approach does not drop any existing links from the vector before pushing the new ones, we might want to clear the vector just in case if we want to keep this new approach. |
||
| Ok(()) | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.