fix(fillers): remove user-triggerable assertions in flag conversion code #1764

gnosek · 2024-03-25T17:02:21Z

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?

API patch version bump? 🤷‍♂️

/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:

With debug builds, it's trivial to hit an ASSERT in fcntl or shutdown, just by passing invalid values for cmd or how, respectively.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Signed-off-by: Grzegorz Nosek <[email protected]>

github-actions · 2024-03-25T17:02:45Z

Please double check driver/SCHEMA_VERSION file. See versioning.

/hold

FedeDP

/approve

poiana · 2024-03-25T17:21:23Z

LGTM label has been added.

Git tree hash: c20a7fc4aa703f7a200a18439f745c0825343791

FedeDP · 2024-03-25T17:21:34Z

/milestone next-driver

FedeDP · 2024-03-26T08:30:15Z

driver/ppm_events_public.h

@@ -273,6 +273,7 @@ or GPL2.txt for full copies of the license.
 #define PPM_SHUT_RD 0
 #define PPM_SHUT_WR 1
 #define PPM_SHUT_RDWR 2
+#define PPM_SHUT_UNKNOWN 0xffff


We should add the string mapping here:

libs/driver/flags_table.c

Line 202 in 04d4cb7

const struct ppm_name_value shutdown_how[] = {

leogr · 2024-03-26T08:32:50Z

Please double check driver/SCHEMA_VERSION file. See versioning.

/hold

This PR does not change the event schema, so there is no need to bump the SCHEMA_VERSION
/unhold

On the other hand, if I understood correctly, this change patches the way the kernel-space<>user-space communicates and, thus, the driver API. Still, the change seems to be backward compatible, so we should only bump the patch number of the API_VERSION. Right? @gnosek

driver/ppm_flag_helpers.h

FedeDP · 2024-03-26T09:41:56Z

driver/API_VERSION

@@ -1 +1 @@
-8.0.2
+8.0.1


api_version--

-ENOCOFFEE. Thanks! :)

For example, <linux/net.h> defines the constants as enum variants, not as preprocessor macros, making the original code work by accident (because SHUT_* are the same as PPM_SHUT_*). Signed-off-by: Grzegorz Nosek <[email protected]>

Signed-off-by: Grzegorz Nosek <[email protected]>

FedeDP

/approve

poiana · 2024-03-26T11:40:25Z

LGTM label has been added.

Git tree hash: 210f652e793778e101ef41d27501f65112d4efaa

Andreagit97

/approve

poiana · 2024-03-26T14:50:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, gnosek

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:

~~driver/OWNERS~~ [Andreagit97,FedeDP,gnosek]

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

fix(fillers): use the proper PPM_SHUT_* constants

554e29f

Signed-off-by: Grzegorz Nosek <[email protected]>

poiana added release-note-none kind/bug Something isn't working dco-signoff: yes area/driver-kmod area/driver-bpf area/driver-modern-bpf size/S labels Mar 25, 2024

poiana requested review from hbrueckner and leogr March 25, 2024 17:02

poiana added the approved label Mar 25, 2024

poiana added the do-not-merge/hold label Mar 25, 2024

FedeDP previously approved these changes Mar 25, 2024

View reviewed changes

poiana assigned FedeDP Mar 25, 2024

poiana added the lgtm label Mar 25, 2024

poiana added this to the next-driver milestone Mar 25, 2024

FedeDP reviewed Mar 26, 2024

View reviewed changes

FedeDP self-requested a review March 26, 2024 08:30

poiana removed the do-not-merge/hold label Mar 26, 2024

therealbobo reviewed Mar 26, 2024

View reviewed changes

driver/ppm_flag_helpers.h Show resolved Hide resolved

gnosek dismissed FedeDP’s stale review via 31b1c5d March 26, 2024 09:38

gnosek force-pushed the flag-fixes branch from 94448d9 to 31b1c5d Compare March 26, 2024 09:38

poiana removed the lgtm label Mar 26, 2024

FedeDP reviewed Mar 26, 2024

View reviewed changes

gnosek force-pushed the flag-fixes branch from 31b1c5d to e1c0874 Compare March 26, 2024 09:42

fix(fillers): do not check for SHUT_RD as a cpp macro

5fc3ea3

For example, <linux/net.h> defines the constants as enum variants, not as preprocessor macros, making the original code work by accident (because SHUT_* are the same as PPM_SHUT_*). Signed-off-by: Grzegorz Nosek <[email protected]>

gnosek added 3 commits March 26, 2024 10:55

fix(fillers): do not ASSERT on an invalid shutdown flag

80795c1

Signed-off-by: Grzegorz Nosek <[email protected]>

fix(fillers): do not ASSERT on an invalid fcntl

5ee2eb7

Signed-off-by: Grzegorz Nosek <[email protected]>

chore(driver): bump API_VERSION by 0.0.1

eecea71

Signed-off-by: Grzegorz Nosek <[email protected]>

gnosek force-pushed the flag-fixes branch from e1c0874 to eecea71 Compare March 26, 2024 09:55

FedeDP approved these changes Mar 26, 2024

View reviewed changes

poiana added the lgtm label Mar 26, 2024

Andreagit97 approved these changes Mar 26, 2024

View reviewed changes

poiana assigned Andreagit97 Mar 26, 2024

poiana merged commit c66b891 into falcosecurity:master Mar 26, 2024
52 of 53 checks passed

gnosek deleted the flag-fixes branch March 26, 2024 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(fillers): remove user-triggerable assertions in flag conversion code #1764

fix(fillers): remove user-triggerable assertions in flag conversion code #1764

gnosek commented Mar 25, 2024

github-actions bot commented Mar 25, 2024

FedeDP left a comment

poiana commented Mar 25, 2024

FedeDP commented Mar 25, 2024

FedeDP Mar 26, 2024

leogr commented Mar 26, 2024 •

edited

Loading

FedeDP Mar 26, 2024 •

edited

Loading

gnosek Mar 26, 2024

FedeDP left a comment

poiana commented Mar 26, 2024

Andreagit97 left a comment

poiana commented Mar 26, 2024

		@@ -1 +1 @@
		8.0.2
		8.0.1

fix(fillers): remove user-triggerable assertions in flag conversion code #1764

fix(fillers): remove user-triggerable assertions in flag conversion code #1764

Conversation

gnosek commented Mar 25, 2024

github-actions bot commented Mar 25, 2024

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Mar 25, 2024

FedeDP commented Mar 25, 2024

FedeDP Mar 26, 2024

Choose a reason for hiding this comment

leogr commented Mar 26, 2024 • edited Loading

FedeDP Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

gnosek Mar 26, 2024

Choose a reason for hiding this comment

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Mar 26, 2024

Andreagit97 left a comment

Choose a reason for hiding this comment

poiana commented Mar 26, 2024

leogr commented Mar 26, 2024 •

edited

Loading

FedeDP Mar 26, 2024 •

edited

Loading