fix(driver): fix bpf probe verifier on linux 5.14 and llvm 12.0.1

[FedeDP]

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Co-authored-by: Jason Dellaluce <jasondellaluce@gmail.com>

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area driver-ebpf

What this PR does / why we need it:

So far, the eBPF probe needs to be compiled with clang 7. Although newer versions of clang can successfully compile the probe, it then gets rejected by the kernel verifier. This has been an open issue for a while.

This PR applies a small set of fixes that allow our eBPF probe to be accepted by the kernel verifier even when compiled with recent versions of clang.
So far, we tested this on various versions of the linux kernel and of clang.
These are the results of our test grid, which we ran on amd64 architecture:

Kernel Ver.	5.14	5.11	5.8	5.4	4.19	4.17	4.15
clang-7	--	OK	OK	OK	OK	OK	OK
clang-8	--	OK	OK	OK	OK	OK	OK
clang-9	--	OK	OK	OK	OK	OK	OK
clang-10	--	OK	OK	OK	OK	OK	OK
clang-11	--	OK	OK	OK	OK	OK	OK
clang-12	OK	OK	OK	OK	OK	OK	OK
clang-14	--	OK	OK	OK	OK	OK	KO

Additional context

At a high level, the kernel verifier rejects the eBPF for three main reasons:

1. Unclear exit conditions in bounded loops. In some cases where the exit condition of a unrolled loop is a little ambiguous, the verifier is not able to predict if some memory accesses can go out of bounds. An example is visible here. In this case, a check on the off variable is required because it is used to access memory inside data->buf[off & ...]. We solved the error by checking the value of off at the beginning of the loop.
2. Unprotected memory accesses. Accessing memory with patterns like &buf[off & SCRATCH_SIZE_HALF], visible here, is often rejected if compiled with newest clang versions. This is due to an optimization applied during the compilation. If a check such as if (off > SCRATCH_SIZE_HALF) /* fail /* appears before a memory access such as &buf[off & SCRATCH_SIZE_HALF], the compiler avoids performing the & SCRATCH_SIZE_HALF operation by assuming that the value of off is already bounded because it is checked inside the if statement. However, the compiled eBPF bytecode gets then rejected by the kernel verifier because it has no way to predict the value of off when accessing the memory buffer. We fixed this by storing the result of off & SCRATCH_SIZE_HALF in an additional variable, so that the compiler is forced to perform the bitwise bounding operation.
3. Passing long integers to eBPF helpers. Helpers such as bpf_probe_read, accept a size argument that is defined as a signed integer. However, in multiple instances (such as here) we pass unsigned long variables as size arguments. We assume the safety of this operation by checking that the value does not go beyond a certain threshold, or by bouding its value through a bitwise and operation, but this is not sufficient. The kernel verifier recognizes an attempt of converting an u64 to a s32, and predicts the possibility of the value to become negative due to the potential bit overflow. We solved this problem by defining the passed value as u16, which was quite sufficient to cover our use cases (our max value fits in 16 bits comfortably).

Which issue(s) this PR fixes:
Fixes #58

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

[poiana]

Hi @FedeDP. Thanks for your PR.

I'm waiting for a falcosecurity member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

[ghost]

Does it finally fix this issue? #58 It's been around for a while!

[FedeDP]

In my case, it fixed the issue.
I think #58 is related as it talks about bpf_sys_readv_preadv_x that internally calls bpf_parse_readv_writev_bufs, that is one of the two fixed functions in this PR.

[leogr]

/ok-to-test

[FedeDP]

Sorry, i had to force push because we missed a Signed-off in the commit suggested by @zuc and committed via github.

[leogr]

Just found two unneeded whitespaces (see suggestions below), otherwise looks awesome 🤩

I reserve some time to test it before the definitive approval.

Thank you!

[leogr]

/cc @leodido
/cc @fntlnz

[leogr]

LGTM 🥳
Btw, many thanks for the detailed explanation, and congrats: these fixes are excellent! 👏

/approve

[poiana]

LGTM label has been added.

Git tree hash: 336f3b3282f27ca9335d1a6c6190881c358f1a16

[poiana]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, leogr

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 [leogr]

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