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

RFC: new(modern_bpf): add security_file_mprotect #1601

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dwindsor
Copy link
Contributor

@dwindsor dwindsor commented Dec 22, 2023

After #963, it is now possible to add new event types to Falco. With this, we are aiming to add the LSM hook security_file_mprotect via the guidance provided in #963.

Please note that this is currently NOT WORKING, and some help would be appreciated to get this in.

As it stands, events for security_file_mprotect are not being submitted to userspace. See my auto-review of security_file_mprotect.bpf.c.

cc: @FedeDP @Andreagit97 @ecbadeaux

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area API-version

/area build

/area CI

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/area libscap-engine-bpf

/area libscap-engine-gvisor

/area libscap-engine-kmod

/area libscap-engine-modern-bpf

/area libscap-engine-nodriver

/area libscap-engine-noop

/area libscap-engine-source-plugin

/area libscap-engine-savefile

/area libscap

/area libpman

/area libsinsp

/area tests

/area proposals

Does this PR require a change in the driver versions?

/version driver-API-version-major

/version driver-API-version-minor

/version driver-API-version-patch

/version driver-SCHEMA-version-major

/version driver-SCHEMA-version-minor

/version driver-SCHEMA-version-patch

What this PR does / why we need it:

Which issue(s) this PR fixes:

#252

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Signed-off-by: David Windsor <dawindso@cisco.com>
@poiana
Copy link
Contributor

poiana commented Dec 22, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dwindsor
Once this PR has been reviewed and has the lgtm label, please assign andreagit97 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

Please double check driver/SCHEMA_VERSION file. See versioning.

/hold

auxmap__store_u32_param(auxmap, prot);

/*=============================== COLLECT PARAMETERS ===========================*/
//auxmap__submit_event(auxmap, ctx);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The auxmap API is wrong (we should be using fixed-size ringbuf), but to illustrate the problem I'm facing, let's just go with this for now.

If this line is not commented out, the verifier fails:

libbpf: prog 'file_mprotect': BPF program load failed: Invalid argument
libbpf: prog 'file_mprotect': -- BEGIN PROG LOAD LOG --
processed 623 insns (limit 1000000) max_states_per_insn 4 total_states 50 peak_states 48 mark_read 3
-- END PROG LOAD LOG --
libbpf: prog 'file_mprotect': failed to load: -22
libbpf: failed to load object 'bpf_probe'
libbpf: failed to load BPF skeleton 'bpf_probe': -22
libpman: failed to load BPF object (errno: 22 | message: Invalid argument)/home/dave/src/libs/test/libscap/test_suites/engines/modern_bpf/modern_bpf.cpp:148: Failure
Value of: !h || ret != 0
  Actual: true
Expected: false

Rather than digging deeper into this now, could I get a sanity check to see if this is the correct approach generally given the guidance in #963? @FedeDP

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this approach; i don't know why this is failing though ;)
Anyway, as shared privately, we surely need more work to understand how to integrate LSM/kprobes inside current libs architecture:

  • we will have a single event (instead of pushing a fake empty exit event), while we always had enter/exit events until now
  • kprobes/LSM don't carry a return code, therefore we need to understand how to deal with this too
  • how to avoid enriching same state from different hooks (ie: both kprobe and syscalls)?

In the end, there are multiple questions looking for a good answer before we can start.
Hopefully maintainers will share some proposals soonish :)

Signed-off-by: David Windsor <dawindso@cisco.com>
Signed-off-by: David Windsor <dawindso@cisco.com>
Signed-off-by: David Windsor <dawindso@cisco.com>
@FedeDP
Copy link
Contributor

FedeDP commented Jan 15, 2024

/milestone TBD

@poiana poiana added this to the TBD milestone Jan 15, 2024
@poiana
Copy link
Contributor

poiana commented Apr 14, 2024

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

@poiana
Copy link
Contributor

poiana commented May 14, 2024

Stale issues rot after 30d of inactivity.

Mark the issue as fresh with /remove-lifecycle rotten.

Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle rotten

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

Successfully merging this pull request may close these issues.

None yet

3 participants