Apply Stackrox changes to falcosecurity libs by Molter73 · Pull Request #1 · stackrox/falcosecurity-libs

Molter73 · 2021-10-05T07:49:18Z

This PR applies the changes that were made by stackrox staff to the sysdig fork unto the falcosecurity-libs fork.

Some extra changes were needed in order to ensure collector plays nice with this new submodule:

Since Falco engineers split the main CMakeLists into several cmake modules, some adjustments were necessary to enable collector and kernel objects to be build correctly.
A double free was being triggered in very specific cases when processing threads, some additional checks were added to prevent this condition.

misberner

We definitely should do a more thorough audit of some of these to check if they are necessary, but for now this looks good!

misberner · 2021-10-06T20:37:07Z

CMakeLists.txt

 	set(SYSDIG_VERSION "0.1.1dev")
 endif()

+if(NOT DEFINED PROBE_NAME)


TBH, not sure if this is still needed. As it is guarded by a if(NOT DEFINED check, we should just be able to set it via -DPROBE_NAME=collector when invoking cmake.

It's not necessary, but we would need to set it every time we need to build a probe, when using CI it's not a big deal, but if you are trying to build it manually it could feel like a nuisance if we need to add it to the CLI command. Maybe we can move it closer to the actual probe build so it doesn't clutter the main CMakeLists, let me take a look.

misberner · 2021-10-06T20:37:52Z

cmake/modules/CompilerFlags.cmake


 	set(SYSDIG_COMMON_FLAGS "-Wall -ggdb")
-	set(SYSDIG_DEBUG_FLAGS "-D_DEBUG")
+	set(SYSDIG_DEBUG_FLAGS "-D_DEBUG -g")


curious, do you have a sense of whether somebody might object to this upstream? It seems pretty natural to add -g for debug

Actually, now that you point it out, 2 lines above this one -ggdb is being set as a common flag, this might be completely redundant.

I think we can probably remove this change. -g adds debug symbols in OS native format and -ggdb adds gdb specific symbols. I would assume the latter is sufficient. https://gcc.gnu.org/onlinedocs/gcc/Debugging-Options.html#Debugging-Options

misberner · 2021-10-06T20:38:39Z

cmake/modules/b64.cmake

 elseif(NOT USE_BUNDLED_B64)
 	find_path(B64_INCLUDE NAMES b64/encode.h)
-	find_library(B64_LIB NAMES b64)
+	find_library(B64_LIB NAMES libb64.a b64)


We would need to go back look at when/why this was added, but I'd be curious if there is some "global" way to tell CMake's find_library to consider static libraries as well.

My guess is this was done to have the static libraries prevail over the shared ones, still there might be a better way to do this, I'll take a look.

misberner · 2021-10-06T20:39:51Z

cmake/modules/libscap.cmake

+        set(PROBE_VERSION "${SYSDIG_VERSION}")
+    endif()
+    if(NOT DEFINED PROBE_NAME)
+        set(PROBE_NAME "sysdig-probe")


It's odd - why would we add this code? I wonder if something went wrong at an earlier time, when somebody (not you) cherrypicked an upstream change

misberner · 2021-10-06T20:41:04Z

cmake/modules/libscap.cmake

+
+if(BUILD_USERSPACE)
+	add_subdirectory(${LIBSCAP_DIR}/userspace/libscap ${PROJECT_BINARY_DIR}/libscap)
+elseif(((BUILD_DRIVER) OR (BUILD_BPF)) AND (CMAKE_SYSTEM_NAME MATCHES "Linux"))


This is a bit weird logic, it basically means we can only either build the driver, or the userspace probes, but not both. Might be good to revisit this if we need to upstream it

I think that's the whole point, since we only ever build userspace (with collector) or the probes/modules (through CI usually). But I agree that it could potentially be annoying and should be handled differently.

Actually, I've just gone through this one more time and the driver directory is added by libscap in the following line:

falcosecurity-libs/userspace/libscap/CMakeLists.txt

Line 98 in 95a283f

add_subdirectory(../../driver ${PROJECT_BINARY_DIR}/driver)

Meaning that the actual output of this is either:

BUILD_USERSPACE=ON will configure libscap which in turn configures the driver too.

BUILD_USERSPACE=OFF and either BUILD_DRIVER or BUILD_BPF set to ON will configure the driver directory only.

misberner · 2021-10-06T20:41:38Z

driver/CMakeLists.txt

 # http://public.kitware.com/Bug/view.php?id=8438
 if(BUILD_DRIVER)
 	add_custom_target(driver ALL
+		COMMAND "${CMAKE_COMMAND}" -E copy_if_different ${CMAKE_CURRENT_BINARY_DIR}/src/Makefile ${CMAKE_CURRENT_SOURCE_DIR}/Makefile


do you know what these are for?

I actually just figured what these lines are for while working on RS-276. When we build kernel modules and eBPF probes we run the make command from the actual directory where the source is instead of running cmake on a separate build directory or storing the configured directory, these 2 lines provide bothe the needed Makefile as well as a driver_config.h header that is usually created by cmake.

misberner · 2021-10-06T20:43:21Z

driver/bpf/Makefile

+	KBUILD_CPPFLAGS+= -DCOS_73_WORKAROUND
+endif
+# clang-7 does not support -fmacro-prefix-map
+KBUILD_CPPFLAGS:=$(filter-out -fmacro-prefix-map=%,$(KBUILD_CPPFLAGS))


Another weird thing: we filter this out unconditionally, regardless of clang version. This is unlikely to be behind the issues we are seeing, but if for some reason it is specified, we should probably pass it through whenever poossible.

misberner · 2021-10-06T21:38:59Z

userspace/libscap/scap_bpf.c

 #else
 	int online_cpu;
 	int j;
+	/* Begin StackRox */


This looks like a potentially universally useful change

misberner · 2021-10-06T21:41:18Z

userspace/libsinsp/container.cpp

 void sinsp_container_manager::notify_new_container(const sinsp_container_info& container_info)
 {
-	sinsp_evt *evt = new sinsp_evt();
+	auto cevt = std::make_shared<sinsp_evt>();


This is a definitely upstreamable change. It's only a small improvement, but generally std::make_shared is preferred over creating a shared_ptr afterwards, as make_shared allows using a single allocation for storing both the object as well as the ref counter

0e7769a Apply all StackRox changes from before sysdig release 0.12.1 5d4b9d6 set ringbuffer to 16MB f7b262a fix off by one in stackrox modifications in driver c1c3260 use SYSDIG_HOST_ROOT env var for ebpf 27275de Add Makefile check for struct audit_task_info 9119d72 Only check hotplug online status of cpus if /sys/devices/system/cpu/hotplug/states exists 88191a8 Support running on RHEL 7.6 kernels with backported eBPF (#8) 5644dde add better error messages when proc scraping fails d212e8b Fix potential nullpointer issue 9af9a4b driver makefile changes 00e0e46 change ifdef from 3.11 to 3.15 0582a71 handle older kernels with ebpf support eda343d Remove unnecessary changes to CMakeLists.txt and conf/dev.conf and set PROBE_NAME to collector ce00a0e - Add static paths for non-bundled dependencies - Disable compilation of sysdig binary bc1577f memory usage fixes 59ce369 Changes in preparation for informatica e6b1a49 Rename SYSDIG_HOST_ROOT env to COLLECTOR_HOST_ROOT b4397fd Log error and continue instead of throwing exception when /proc scrape fails (#16) 696852d Allow omitting gRPC libraries from linking (#17) 941bb18 ROX-5597 Collect subset of system calls under eBPF (#18) cbbe5d4 Free memory allocated if fd read fails during proc scrape in libscap (#19) f53b6bb ROX-6200 Remove image label and env var parsing (#20) e32e5e5 ROX-5396 - Fix probe compilation on 5.7-5.10 kernels (#21) a723117 Change libsinsp binary directory to project b47a0ad Configure BPF driver only when being built 8661537 Ensure driver is configured without userspace 56a3d6f Move some kobject definitions 0e8b65e remove invalid PATH 74f0ec4 Trying to add -g to sysdig/src/CMakeList.txt 65694d7 Fix potential segfault 95a283f Add extra checks to prevent memory leak 7389e5e Remove duplicated probe name definition ef283bb Apply upstream fixes to eBPF probes dd16bd7 Remove unneeded include

Signed-off-by: Lovel Rishi <lovelworks@gmail.com> Address review comments #1: 1. Remove extra lookup for exe file 2. Directive to unroll loops 3. Fix compilation issues in modern bpf probe Signed-off-by: Lovel Rishi <lovelworks@gmail.com>

glibc-2.42 added __inet_ntop_chk fortification, which started to fail: *** buffer overflow detected ***: terminated Program received signal SIGABRT, Aborted. 0x00007ffff629b0dc in __pthread_kill_implementation () from /lib64/libc.so.6 (gdb) bt #0 0x00007ffff629b0dc in __pthread_kill_implementation () from /lib64/libc.so.6 #1 0x00007ffff6242572 in raise () from /lib64/libc.so.6 #2 0x00007ffff6229f3b in abort () from /lib64/libc.so.6 #3 0x00007ffff622b148 in __libc_message_impl.cold () from /lib64/libc.so.6 #4 0x00007ffff6327337 in __fortify_fail () from /lib64/libc.so.6 #5 0x00007ffff6326c92 in __chk_fail () from /lib64/libc.so.6 #6 0x00007ffff6327a62 in __inet_ntop_chk () from /lib64/libc.so.6 #7 0x000055555569da3d in inet_ntop (__af=10, __src=0x555555ee0800, __dst=0x7fffffff4f90 "\260P\377\377\377\177", __dst_size=100) at /usr/include/bits/inet-fortified.h:36 #8 ipv6tuple_to_string[abi:cxx11](ipv6tuple*, bool) (tuple=0x555555ee0800, resolve=false) at /tmp/portage/dev-debug/sysdig-0.40.1/work/libs-0.20.0/userspace/libsinsp/utils.c Use INET6_ADDRSTRLEN as destination buffer size. Fixes: falcosecurity/libs#2573 Signed-off-by: Holger Hoffstätte <holger@applied-asynchrony.com>

Molter73 requested review from JoukoVirtanen and misberner October 5, 2021 07:49

Molter73 self-assigned this Oct 5, 2021

misberner approved these changes Oct 6, 2021

View reviewed changes

Molter73 requested a review from robbycochran November 2, 2021 16:47

robbycochran approved these changes Nov 22, 2021

View reviewed changes

Molter73 changed the base branch from master to mauro/pull-falco-changes November 24, 2021 12:06

Molter73 force-pushed the mauro/ROX-7482-migrate-sysdig-to-falco branch from dd16bd7 to 0e3f2c0 Compare November 24, 2021 12:06

Molter73 force-pushed the mauro/ROX-7482-migrate-sysdig-to-falco branch from 259de25 to 45ff474 Compare December 15, 2021 16:13

Molter73 changed the base branch from mauro/pull-falco-changes to master January 31, 2022 09:12

Molter73 added 4 commits January 31, 2022 10:13

X-Smart-Branch-Parent: mauro/pull-falco-changes

eac6db1

Add missing curl dependency

5e5cd2d

Fix eBPF errors after rebase

a80e0b7

Molter73 force-pushed the mauro/ROX-7482-migrate-sysdig-to-falco branch from 45ff474 to a80e0b7 Compare January 31, 2022 09:13

Molter73 merged commit d7ff366 into master Jan 31, 2022

Stringy pushed a commit that referenced this pull request Mar 1, 2022

Apply Stackrox changes to falcosecurity libs (#1)

72af61d

Stringy pushed a commit that referenced this pull request Mar 1, 2022

Apply Stackrox changes to falcosecurity libs (#1)

27992b1

Stringy pushed a commit that referenced this pull request Jan 20, 2023

Apply Stackrox changes to falcosecurity libs (#1)

5a43f42

Stringy pushed a commit that referenced this pull request Jan 25, 2023

Apply Stackrox changes to falcosecurity libs (#1)

3622192

Stringy pushed a commit that referenced this pull request Jan 25, 2023

Apply Stackrox changes to falcosecurity libs (#1)

4b631b8

Stringy pushed a commit that referenced this pull request Jan 25, 2023

Apply Stackrox changes to falcosecurity libs (#1)

81df179

Stringy pushed a commit that referenced this pull request Apr 18, 2023

Apply Stackrox changes to falcosecurity libs (#1)

1c355d0

Stringy pushed a commit that referenced this pull request Apr 19, 2023

Apply Stackrox changes to falcosecurity libs (#1)

2d8699c

Stringy pushed a commit that referenced this pull request Apr 27, 2023

Apply Stackrox changes to falcosecurity libs (#1)

52b58fe

Stringy pushed a commit that referenced this pull request Apr 27, 2023

Apply Stackrox changes to falcosecurity libs (#1)

f59a628

Stringy pushed a commit that referenced this pull request May 4, 2023

Apply Stackrox changes to falcosecurity libs (#1)

20008c5

Stringy pushed a commit that referenced this pull request May 4, 2023

Apply Stackrox changes to falcosecurity libs (#1)

ff0869a

Stringy pushed a commit that referenced this pull request May 5, 2023

Apply Stackrox changes to falcosecurity libs (#1)

b3fc9ec

Stringy pushed a commit that referenced this pull request May 10, 2023

Apply Stackrox changes to falcosecurity libs (#1)

0cf2e7b

erthalion added a commit that referenced this pull request Jul 6, 2023

Apply Stackrox changes to falcosecurity libs (#1)

fa743d3

erthalion added a commit that referenced this pull request Aug 29, 2023

Apply Stackrox changes to falcosecurity libs (#1)

749532c

Conversation

Molter73 commented Oct 5, 2021

Uh oh!

misberner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Molter73 Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robbycochran Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Molter73 Oct 7, 2021 •

edited

Loading

robbycochran Nov 22, 2021 •

edited

Loading