fix(libsinsp/modern_bpf): ancillary data improvements by wigol · Pull Request #2336 · falcosecurity/libs

wigol · 2025-04-08T16:00:41Z

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:
This change brings improvements in handling ancillary data retrieved by recvmsg and recvmmsg:

(modern_bpf) we explicitly limit stored ancillary data to buffers below 64k (due to maximum buffer length). Larger payloads are discarded.
(sinsp/parsers) additional check is added for ancillary data parsing for (rare) cases of malformed payloads.
(sinsp/parsers) ancillary data processing is now limited only to Unix sockets, since file descriptors processing only makes sense in this context.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Signed-off-by: Wiktor Gołgowski <wiktor.golgowski@sysdig.com>

ekoops · 2025-04-08T16:10:18Z

userspace/libsinsp/parsers.cpp

 	    cmsg = PPM_CMSG_NXTHDR(msg_ctrl, msg_ctrllen, cmsg)) {
+		// Check for malformed control message buffer:
+		if(reinterpret_cast<const char *>(cmsg) < msg_ctrl ||
+		   reinterpret_cast<const char *>(cmsg) > msg_ctrl + msg_ctrllen) {


Shouldn't the second part be reinterpret_cast<const char *>(cmsg) >= msg_ctrl + msg_ctrllen) ?

Correct, thank you!

I don't think so.

Consider the case where msg_ctrl = 0x1000, msg_ctrllen = 64.

If cmsg = 0x1064 then even if the read length is 1 byte, it's still reading past the end of the buffer (valid range is 0x1000 - 0x1063).

Maybe I'm missing something, but I believe this logic is correct.

Mhh, If i have a buffer msg_ctrl = 1000, with msg_ctrllen = 64, I can read from 1000 up to 1063 (both included).

Sorry for the confusion, I should stop leaving code review comments in a hurry

Ahaha np! 😄

nathan-b · 2025-04-08T16:13:54Z

userspace/libsinsp/parsers.cpp

 	    cmsg = PPM_CMSG_NXTHDR(msg_ctrl, msg_ctrllen, cmsg)) {
+		// Check for malformed control message buffer:
+		if(reinterpret_cast<const char *>(cmsg) < msg_ctrl ||
+		   reinterpret_cast<const char *>(cmsg) > msg_ctrl + msg_ctrllen) {


I don't think so.

Consider the case where msg_ctrl = 0x1000, msg_ctrllen = 64.

If cmsg = 0x1064 then even if the read length is 1 byte, it's still reading past the end of the buffer (valid range is 0x1000 - 0x1063).

Maybe I'm missing something, but I believe this logic is correct.

poiana · 2025-04-08T16:14:18Z

@nathan-b: changing LGTM is restricted to collaborators

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Signed-off-by: Wiktor Gołgowski <wiktor.golgowski@sysdig.com>

codecov · 2025-04-08T16:28:12Z

Codecov Report

Attention: Patch coverage is 40.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 77.17%. Comparing base (7f01ec8) to head (503d27f).
Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
userspace/libsinsp/parsers.cpp	40.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2336      +/-   ##
==========================================
- Coverage   77.18%   77.17%   -0.02%     
==========================================
  Files         227      227              
  Lines       30192    30199       +7     
  Branches     4611     4614       +3     
==========================================
+ Hits        23304    23306       +2     
- Misses       6888     6893       +5

Flag	Coverage Δ
libsinsp	`77.17% <40.00%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

poiana · 2025-04-08T16:39:20Z

LGTM label has been added.

Details

Git tree hash: dd24f914dfce16b3179f00410665a25ac05eea96

github-actions · 2025-04-08T17:10:13Z

Perf diff from master - unit tests

     5.58%     +0.66%  [.] sinsp_parser::reset
     0.98%     +0.23%  [.] user_group_updater::user_group_updater
     2.55%     +0.23%  [.] gzfile_read
     1.11%     -0.22%  [.] libsinsp::sinsp_suppress::process_event
    36.13%     -0.18%  [.] sinsp_thread_manager::create_thread_dependencies
     0.57%     +0.17%  [.] sinsp_parser::event_cleanup
     1.54%     -0.16%  [.] std::_Hashtable<long, std::pair<long const, std::shared_ptr<sinsp_threadinfo> >, std::allocator<std::pair<long const, std::shared_ptr<sinsp_threadinfo> > >, std::__detail::_Select1st, std::equal_to<long>, std::hash<long>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::_M_find_before_node
     1.43%     +0.16%  [.] is_conversion_needed
     0.81%     -0.16%  [.] sinsp_evt::get_direction
     0.67%     +0.15%  [.] sinsp_evt::get_syscall_return_value

Heap diff from master - unit tests

peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Benchmarks diff from master

Comparing gbench_data.json to /root/actions-runner/_work/libs/libs/build/gbench_data.json
Benchmark                                                         Time             CPU      Time Old      Time New       CPU Old       CPU New
----------------------------------------------------------------------------------------------------------------------------------------------
BM_sinsp_split_mean                                            +0.0109         +0.0108           148           149           148           149
BM_sinsp_split_median                                          +0.0134         +0.0133           147           149           147           149
BM_sinsp_split_stddev                                          -0.5460         -0.5470             1             0             1             0
BM_sinsp_split_cv                                              -0.5509         -0.5519             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_mean                  -0.0379         -0.0380            64            61            64            61
BM_sinsp_concatenate_paths_relative_path_median                -0.0316         -0.0317            63            61            63            61
BM_sinsp_concatenate_paths_relative_path_stddev                -0.8241         -0.8242             2             0             2             0
BM_sinsp_concatenate_paths_relative_path_cv                    -0.8171         -0.8172             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_mean                     -0.0073         -0.0074            24            24            24            24
BM_sinsp_concatenate_paths_empty_path_median                   -0.0071         -0.0072            24            24            24            24
BM_sinsp_concatenate_paths_empty_path_stddev                   -0.1540         -0.1561             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_cv                       -0.1478         -0.1498             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_mean                  -0.0087         -0.0088            64            64            64            64
BM_sinsp_concatenate_paths_absolute_path_median                -0.0240         -0.0241            65            63            65            63
BM_sinsp_concatenate_paths_absolute_path_stddev                -0.2620         -0.2616             1             1             1             1
BM_sinsp_concatenate_paths_absolute_path_cv                    -0.2555         -0.2551             0             0             0             0

FedeDP · 2025-04-09T06:59:07Z

/milestone 0.21.0

FedeDP

/approve

Molter73

/lgtm

poiana · 2025-04-09T08:58:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, Molter73, wigol

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [FedeDP,Molter73]

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

wigol added 3 commits April 8, 2025 15:52

fix(libsinsp): protect from malformed ancillary data

dfd3e99

Signed-off-by: Wiktor Gołgowski <wiktor.golgowski@sysdig.com>

fix(libsinsp): retrieve ancillary data only for Unix sockets

10b8c15

Signed-off-by: Wiktor Gołgowski <wiktor.golgowski@sysdig.com>

fix(modern_bpf): only store ancillary data under 64k

75d8653

Signed-off-by: Wiktor Gołgowski <wiktor.golgowski@sysdig.com>

github-project-automation bot added this to Falco Roadmap Apr 8, 2025

poiana added the kind/bug Something isn't working label Apr 8, 2025

github-project-automation bot moved this to Todo in Falco Roadmap Apr 8, 2025

poiana added release-note-none dco-signoff: yes area/driver-modern-bpf area/libsinsp size/M labels Apr 8, 2025

poiana requested review from hbrueckner and incertum April 8, 2025 16:01

ekoops requested changes Apr 8, 2025

View reviewed changes

poiana assigned ekoops Apr 8, 2025

github-project-automation bot moved this from Todo to In progress in Falco Roadmap Apr 8, 2025

nathan-b approved these changes Apr 8, 2025

View reviewed changes

fix(libsinsp): off-by-one error

503d27f

Signed-off-by: Wiktor Gołgowski <wiktor.golgowski@sysdig.com>

leogr requested review from FedeDP, LucaGuerra and ekoops April 8, 2025 16:28

ekoops approved these changes Apr 8, 2025

View reviewed changes

poiana added the lgtm label Apr 8, 2025

poiana added this to the 0.21.0 milestone Apr 9, 2025

FedeDP approved these changes Apr 9, 2025

View reviewed changes

poiana assigned FedeDP Apr 9, 2025

poiana added the approved label Apr 9, 2025

Molter73 approved these changes Apr 9, 2025

View reviewed changes

poiana assigned Molter73 Apr 9, 2025

poiana merged commit 59ad87c into falcosecurity:master Apr 9, 2025
53 of 55 checks passed

github-project-automation bot moved this from In progress to Done in Falco Roadmap Apr 9, 2025

bevanjkay mentioned this pull request May 20, 2025

falcosecurity-libs 0.21.0 Homebrew/homebrew-core#224095

Closed

deepskyblue86 mentioned this pull request Dec 19, 2025

fix(userspace/libsinsp): prevent infinite loop in ancillary data pars… #2764

Merged

Conversation

wigol commented Apr 8, 2025

Uh oh!

ekoops Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

wigol Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

nathan-b Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

ekoops Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

nathan-b Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

ekoops Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

nathan-b Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

poiana commented Apr 8, 2025

Uh oh!

codecov bot commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

poiana commented Apr 8, 2025

Uh oh!

github-actions bot commented Apr 8, 2025

Perf diff from master - unit tests

Heap diff from master - unit tests

Heap diff from master - scap file

Benchmarks diff from master

Uh oh!

FedeDP commented Apr 9, 2025

Uh oh!

FedeDP left a comment

Choose a reason for hiding this comment

Uh oh!

Molter73 left a comment

Choose a reason for hiding this comment

Uh oh!

poiana commented Apr 9, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Apr 8, 2025 •

edited

Loading