Update(`prlimit`&`setrlimit`): Add `resource` arg for exit event #1348

Rohith-Raju · 2023-09-12T08:44:21Z

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/area tests

What this PR does / why we need it:
Adding the resource argument to the exit events of setrlimit and prlimit will detect and provide useful alerts.

Which issue(s) this PR fixes:

Fixes #1327

Special notes for your reviewer:

Does this PR introduce a user-facing change?: No

Update(`prlimit`&`setrlimit`):  Add `resource` arg for exit event

github-actions · 2023-09-12T08:45:32Z

Please double check driver/API_VERSION file. See versioning.

/hold

Andreagit97 · 2023-09-15T10:02:39Z

Thank you for this, we will take a look soon! :)

Andreagit97

Thank you for this! Left some comments
@Biagio-Dipalma after this PR it would be possible to access the resource field in setrlimit and prlimit syscalls through evt.arg.resource. Does it sound good to you?

Andreagit97 · 2023-10-18T10:04:51Z

driver/SCHEMA_VERSION

Here we need a minor, so 2.13.0.
Moreover, we need also a major bump in the API_VERSION since we removed some old fillers

Andreagit97 · 2023-10-18T10:12:30Z

driver/bpf/fillers.h

+	/*
+	 * Copy the user structure and extract cur and max
+	 */
+	if(retval >= 0 || data->state->tail_ctx.evt_type == PPME_SYSCALL_SETRLIMIT_X)


Suggested change

if(retval >= 0 || data->state->tail_ctx.evt_type == PPME_SYSCALL_SETRLIMIT_X)

if(retval >= 0)

Same in the setrlimit filler, now that we split them there is no more need for this if

Andreagit97 · 2023-10-18T10:14:36Z

driver/bpf/fillers.h

+		max = -1;
+	}
+
+	/* Parameter 2: cur (type: PT_ERRNO) */


Suggested change

/* Parameter 2: cur (type: PT_ERRNO) */

/* Parameter 2: cur (type: PT_INT64) */

Same in all other places

Andreagit97 · 2023-10-18T10:18:17Z

driver/bpf/fillers.h

+	CHECK_RES(res);
+
+	/* Parameter 4: resource (type: PT_ERRNO) */
+	unsigned long resource = bpf_syscall_get_argument(data, 0);


Suggested change

unsigned long resource = bpf_syscall_get_argument(data, 0);

uint32_t resource = bpf_syscall_get_argument(data, 0);

Same in all other drivers

Have a doubt: #1348 (comment)

driver/bpf/fillers.h

Andreagit97 · 2023-10-18T10:33:04Z

driver/ppm_fillers.c

 	 */
+	if(retval >= 0 || args->event_type == PPME_SYSCALL_SETRLIMIT_X)


Suggested change

if(retval >= 0 || args->event_type == PPME_SYSCALL_SETRLIMIT_X)

if(retval >= 0)

Andreagit97 · 2023-10-18T10:34:35Z

driver/ppm_fillers.c

 	res = val_to_ring(args, newcur, 0, false, 0);
 	CHECK_RES(res);
+	CHECK_RES(res);


probably we can remove this, same below

Andreagit97 · 2023-10-18T10:35:57Z

driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/setrlimit.bpf.c

@@ -68,6 +68,10 @@ int BPF_PROG(setrlimit_x,
 	/* Parameter 3: max (type: PT_INT64)*/
 	ringbuf__store_s64(&ringbuf, rl.rlim_max);

+	/* Parameter 4: resource (type: PT_ENUMFLAGS8) */
+	unsigned long resource = extract__syscall_argument(regs, 0);


Suggested change

unsigned long resource = extract__syscall_argument(regs, 0);

uint32_t resource = extract__syscall_argument(regs, 0);

Andreagit97 · 2023-10-18T10:39:03Z

test/drivers/test_suites/syscall_exit_suite/setrlimit_x.cpp

we could add a test both in setrlimit and prlimit in which we use a valid resource, for example RLIMIT_MEMLOCK, and an invalid pointer to a struct rlimit, so something like syscall(__NR_setrlimit, RLIMIT_MEMLOCK, NULL), WDYT?

Yes, Sounds good!!

Andreagit97 · 2023-10-18T10:40:58Z

driver/bpf/fillers.h

+
+		val = bpf_syscall_get_argument(data, 1);
+		if(bpf_probe_read_user(&rl, sizeof(rl), (void *)val))
+			return PPM_FAILURE_INVALID_USER_MEMORY;


Instead of returning this code probably here we want to return cur = 0; and max = 0; See the modern bpf implementation. Same for the kmod

We can do something like this

struct rlimit rl = {0}; val = bpf_syscall_get_argument(data, 1); bpf_probe_read_user(&rl, sizeof(rl), (void *)val); cur = rl.rlim_cur; max = rl.rlim_max;

instead of returning PPM_FAILURE_INVALID_USER_MEMORY if we fail in bpf_probe_read_user

Signed-off-by: rohith-raju <[email protected]>

Signed-off-by: Andrea Terzolo <[email protected]>

Andreagit97 · 2023-11-21T12:07:13Z

The PR is great, thank you!
Since I was there I just added a commit to increase consistency between drivers, feel free to revert it if you don't like it :)

Andreagit97 · 2023-11-21T12:08:31Z

driver/API_VERSION

unfortunately, due to an inconsistency (#1283) we need a new major API version every time we add a new filler in the old probe

@Andreagit97 Understood!!

FedeDP

/approve

poiana · 2023-11-21T13:42:42Z

LGTM label has been added.

Git tree hash: 1730c644404b29a94e3f7e48a796c58cf6938783

poiana · 2023-11-21T14:59:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, jasondellaluce, Rohith-Raju

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [FedeDP,jasondellaluce]

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

FedeDP · 2023-11-21T15:14:55Z

/unhold

poiana added release-note kind/feature New feature or request dco-signoff: yes area/driver-kmod area/driver-bpf area/driver-modern-bpf area/tests size/L labels Sep 12, 2023

poiana requested review from hbrueckner and Molter73 September 12, 2023 08:44

poiana added the do-not-merge/hold label Sep 12, 2023

Rohith-Raju marked this pull request as draft September 12, 2023 09:29

poiana added the do-not-merge/work-in-progress label Sep 12, 2023

Rohith-Raju force-pushed the prlimit branch 4 times, most recently from b8e9130 to bef3ef4 Compare September 13, 2023 07:45

Rohith-Raju marked this pull request as ready for review September 13, 2023 07:45

poiana removed the do-not-merge/work-in-progress label Sep 13, 2023

poiana requested a review from LucaGuerra September 13, 2023 07:45

Andreagit97 added this to the 0.14.0 milestone Sep 15, 2023

Rohith-Raju force-pushed the prlimit branch 3 times, most recently from a630808 to b5ccaca Compare October 17, 2023 11:33

Andreagit97 reviewed Oct 18, 2023

View reviewed changes

Rohith-Raju force-pushed the prlimit branch from b5ccaca to f04e396 Compare October 30, 2023 08:09

poiana added size/XL and removed size/L labels Oct 30, 2023

Rohith-Raju requested a review from Andreagit97 November 1, 2023 06:53

poiana added the do-not-merge/contains-merge-commits label Nov 18, 2023

Rohith-Raju added 4 commits November 18, 2023 06:01

update(prlimit): update args in prlimit exit event

ec1d020

Signed-off-by: rohith-raju <[email protected]>

update(setrlimit): update args in setrlimit exit event

8acaba9

Signed-off-by: rohith-raju <[email protected]>

update: seperate getrlimit and setrlimit

355a51a

Signed-off-by: rohith-raju <[email protected]>

update: retain original parameter order

debf97a

Signed-off-by: rohith-raju <[email protected]>

Rohith-Raju force-pushed the prlimit branch from d2db78e to b02c3f7 Compare November 18, 2023 06:03

poiana removed the do-not-merge/contains-merge-commits label Nov 18, 2023

Rohith-Raju force-pushed the prlimit branch from b02c3f7 to 7dfa709 Compare November 20, 2023 12:17

update: address review requests

db5f0cc

Signed-off-by: rohith-raju <[email protected]>

Rohith-Raju force-pushed the prlimit branch from 7dfa709 to db5f0cc Compare November 20, 2023 14:41

Andreagit97 added 2 commits November 21, 2023 12:39

chore: bump API and SCHEMA version

fbc2432

Signed-off-by: Andrea Terzolo <[email protected]>

update: adopt the same behavior in all drivers

c320275

Signed-off-by: Andrea Terzolo <[email protected]>

poiana added size/XXL and removed size/XL labels Nov 21, 2023

Andreagit97 reviewed Nov 21, 2023

View reviewed changes

Andreagit97 mentioned this pull request Nov 21, 2023

update(drivers): add cmd to exit events #1419

Merged

FedeDP approved these changes Nov 21, 2023

View reviewed changes

poiana assigned FedeDP Nov 21, 2023

poiana added the lgtm label Nov 21, 2023

poiana added the approved label Nov 21, 2023

jasondellaluce approved these changes Nov 21, 2023

View reviewed changes

poiana assigned jasondellaluce Nov 21, 2023

poiana removed the do-not-merge/hold label Nov 21, 2023

poiana merged commit 4e9a3cc into falcosecurity:master Nov 21, 2023
38 checks passed

Rohith-Raju deleted the prlimit branch December 11, 2023 09:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update(`prlimit`&`setrlimit`): Add `resource` arg for exit event #1348

Update(`prlimit`&`setrlimit`): Add `resource` arg for exit event #1348

Rohith-Raju commented Sep 12, 2023 •

edited

Loading

github-actions bot commented Sep 12, 2023 •

edited

Loading

Andreagit97 commented Sep 15, 2023

Andreagit97 left a comment •

edited

Loading

Andreagit97 Oct 18, 2023

Andreagit97 Oct 18, 2023

Andreagit97 Oct 18, 2023

Andreagit97 Oct 18, 2023

Andreagit97 Oct 18, 2023

Andreagit97 Oct 18, 2023

Andreagit97 Oct 18, 2023

Rohith-Raju Oct 23, 2023

Andreagit97 Oct 18, 2023

Andreagit97 Oct 18, 2023

Andreagit97 Oct 18, 2023

Andreagit97 Oct 18, 2023

Rohith-Raju Oct 23, 2023

Andreagit97 Oct 18, 2023

Andreagit97 Oct 30, 2023

Andreagit97 commented Nov 21, 2023

Andreagit97 Nov 21, 2023

Rohith-Raju Nov 21, 2023

FedeDP left a comment

poiana commented Nov 21, 2023

poiana commented Nov 21, 2023

FedeDP commented Nov 21, 2023

	if(retval >= 0 \|\| data->state->tail_ctx.evt_type == PPME_SYSCALL_SETRLIMIT_X)
	if(retval >= 0)

	/* Parameter 2: cur (type: PT_ERRNO) */
	/* Parameter 2: cur (type: PT_INT64) */

	unsigned long resource = bpf_syscall_get_argument(data, 0);
	uint32_t resource = bpf_syscall_get_argument(data, 0);

		*/
		if(retval >= 0 \|\| args->event_type == PPME_SYSCALL_SETRLIMIT_X)

	unsigned long resource = extract__syscall_argument(regs, 0);
	uint32_t resource = extract__syscall_argument(regs, 0);

Update(prlimit&setrlimit): Add resource arg for exit event #1348

Update(prlimit&setrlimit): Add resource arg for exit event #1348

Conversation

Rohith-Raju commented Sep 12, 2023 • edited Loading

github-actions bot commented Sep 12, 2023 • edited Loading

Andreagit97 commented Sep 15, 2023

Andreagit97 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andreagit97 commented Nov 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Nov 21, 2023

poiana commented Nov 21, 2023

FedeDP commented Nov 21, 2023

Update(`prlimit`&`setrlimit`): Add `resource` arg for exit event #1348

Update(`prlimit`&`setrlimit`): Add `resource` arg for exit event #1348

Rohith-Raju commented Sep 12, 2023 •

edited

Loading

github-actions bot commented Sep 12, 2023 •

edited

Loading

Andreagit97 left a comment •

edited

Loading