Skip to content

Matched rules #4484

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

Draft
wants to merge 44 commits into
base: main
Choose a base branch
from
Draft

Matched rules #4484

wants to merge 44 commits into from

Conversation

yanivagman
Copy link
Collaborator

1. Explain what the PR does

WIP

2. Explain how to test it

3. Other comments

PoliciesConfig: *cfg,
TraceePid: uint32(os.Getpid()),
Options: t.getOptionsConfig(),
CgroupV1Hid: uint32(t.cgroups.GetDefaultCgroupHierarchyID()),

Check failure

Code scanning / CodeQL

Incorrect conversion between integer types High

Incorrect conversion of an integer with architecture-dependent bit size from
strconv.Atoi
to a lower bit size type uint32 without an upper bound check.
Copy link
Member

Choose a reason for hiding this comment

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

I've mentioned about this in #4482 (comment).

@yanivagman yanivagman force-pushed the matched_rules branch 7 times, most recently from 386226b to 3ad4cf4 Compare January 16, 2025 18:02
THIS PATCH IS NOT COMPLETE. NEED TO ADD USERSPACE PART.
Look for proc_tree and process_tree in userspace and remove them as well
@yanivagman yanivagman force-pushed the matched_rules branch 2 times, most recently from f5f0967 to 1f411ed Compare January 19, 2025 15:44
Copy link
Member

@geyslan geyslan left a comment

Choose a reason for hiding this comment

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

Just a quick (so quick) pass. Tomorrow I'm going to dive into this.

Comment on lines +15 to +20
func (e *policyError) Is(target error) bool {
t, ok := target.(*policyError)
if !ok {
return false
}
return e.msg == t.msg
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it should be better to rely on the errors.Is() since it takes care of unwrapping.

PoliciesConfig: *cfg,
TraceePid: uint32(os.Getpid()),
Options: t.getOptionsConfig(),
CgroupV1Hid: uint32(t.cgroups.GetDefaultCgroupHierarchyID()),
Copy link
Member

Choose a reason for hiding this comment

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

I've mentioned about this in #4482 (comment).

Copy link
Member

@geyslan geyslan left a comment

Choose a reason for hiding this comment

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

I did another passthrough.

// Retrieve the string filter for the current event
// TODO: Dynamically determine the filter and type based on policy configuration
string_filter_config_t *str_filter = &p->event->config.data_filter.string;

if (!(str_filter->exact_enabled || str_filter->prefix_enabled || str_filter->suffix_enabled))
return policies_cfg->enabled_policies;
return ~0ULL;
Copy link
Member

Choose a reason for hiding this comment

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

Opportunistic fix? @rscampos FYI.

// Combine rules that use string filters with those that do not
res |= mask_no_str_filter_rules;

return res;
Copy link
Member

Choose a reason for hiding this comment

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

Same as above?

@@ -3193,7 +3173,7 @@ statfunc int capture_file_write(struct pt_regs *ctx, u32 event_id, bool is_buf)
del_args(event_id);

program_data_t p = {};
if (!init_program_data(&p, ctx, NO_EVENT_SUBMIT))
if (!init_program_data(&p, ctx, POLICY_SCOPES))
Copy link
Member

Choose a reason for hiding this comment

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

What does the internal POLICY_SCOPES event must communicate?

@@ -386,8 +372,12 @@ typedef struct data_filter_config {
} data_filter_config_t;

typedef struct event_config {
u64 submit_for_policies;
u16 rules_version;
u8 has_overflow;
Copy link
Member

Choose a reason for hiding this comment

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

Could you elaborate more about has_overflow?

Copy link
Member

@geyslan geyslan left a comment

Choose a reason for hiding this comment

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

I've finished an overall review. 👍🏻 This is neat.

continue
// range through each userland filterable rule
for _, rule := range t.policyManager.GetUserlandRules(eventID) {
if rule.ID > 63 {
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to use a MaxRules alike const.

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.

2 participants