Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement callx instruction #584

Merged
merged 3 commits into from
Feb 29, 2024
Merged

Implement callx instruction #584

merged 3 commits into from
Feb 29, 2024

Conversation

dthaler
Copy link
Contributor

@dthaler dthaler commented Feb 3, 2024

"callx" instructions are generated by both clang (under -O0 or -O1) and gcc (if the
experimental -mxbpf option is passed to the compiler), but are not supported by Linux.

Per mailing list discussion at
https://mailarchive.ietf.org/arch/msg/bpf/CDQjTO8R8gdPdfeKVnoxWco8_Lw/ the intent is for the Linux verifier to support them eventually, and with this PR, PREVAIL and dependent projects like ebpf-for-windows can support them now. This will also unblock the ability to use versions of clang later than clang-11 by using -O1 instead of -O2, which generates correlated branches PREVAIL can't deal with.

The approach taken by this PR is:

  • Fail verification unless the register is known to always hold a single
    integer value at the time of the instruction. This covers the common case.
  • Using the single integer, do the same verifier checks as would have
    been done with the normal BPF_CALL instruction.

The mailing list discussion at
https://mailarchive.ietf.org/arch/msg/bpf/Vx1H3ViPUWoGKNssCO22lOIjyXU/
agreed that inst.dst is where to put the register to use going forward.
Clang v.19 will do so but earlier versions of clang all used inst.imm.

@dthaler dthaler marked this pull request as draft February 3, 2024 21:47
@dthaler dthaler force-pushed the callx branch 2 times, most recently from 13c247e to 4bd4861 Compare February 3, 2024 21:52
@coveralls
Copy link

coveralls commented Feb 3, 2024

Coverage Status

coverage: 90.346% (+0.1%) from 90.234%
when pulling 479a894 on dthaler:callx
into e0c25fc on vbpf:main.

@dthaler dthaler marked this pull request as ready for review February 4, 2024 06:02
@dthaler dthaler marked this pull request as draft February 4, 2024 19:05
@dthaler dthaler marked this pull request as ready for review February 4, 2024 19:18
@dthaler dthaler marked this pull request as draft February 5, 2024 04:32
@dthaler dthaler marked this pull request as ready for review February 5, 2024 21:12
@dthaler dthaler marked this pull request as draft February 5, 2024 21:44
@dthaler dthaler marked this pull request as ready for review February 5, 2024 21:59
@dthaler dthaler marked this pull request as draft February 12, 2024 17:42
@dthaler dthaler marked this pull request as ready for review February 13, 2024 17:27
These instructions are generated by both clang (under -O0 or -O1)
and gcc (if the experimental -mxbpf option is passed to the compiler),
but are not supported by Linux.

Per mailing list discussion at
https://mailarchive.ietf.org/arch/msg/bpf/CDQjTO8R8gdPdfeKVnoxWco8_Lw/
the intent is to support them eventually, and with this PR,
ebpf-for-windows can support them now.  This will also unblock
the ability to use versions of clang later than clang-11 by using -O1
instead of -O2 (which generates correlated branches PREVAIL can't
deal with).

The mailing list discussion at
https://mailarchive.ietf.org/arch/msg/bpf/Vx1H3ViPUWoGKNssCO22lOIjyXU/
agreed that inst.dst is where to put the register to use going forward
and clang v.19 will do so but earlier versions of clang used inst.imm.

Signed-off-by: Dave Thaler <[email protected]>
src/crab/ebpf_domain.cpp Outdated Show resolved Hide resolved
src/crab/ebpf_domain.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@elazarg elazarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I will merge after the style comments are addressed.

I'd do some of it differently, but it can probably wait for a future PR:
Instead of a dedicated Callx struct, we can have a Value function as a field in Call.
Other fields in Call go either into a Signature struct or left as platform details (name).

The value of the register will not be replaced by multiple instruction, but with the meet-over-signatures of possible values (signature, as a type, is itself an abstract domain; meet can be as simple as "either a single value or BOTTOM").

get_assertions() will not be needed as a general interface to the assertions module; we only need it for callx, and we pass instruction because we don't have a "signature" struct/abstract domain.

src/test/test_marshal.cpp Outdated Show resolved Hide resolved
src/test/test_marshal.cpp Outdated Show resolved Hide resolved
test-data/callx.yaml Outdated Show resolved Hide resolved
Signed-off-by: Dave Thaler <[email protected]>
@dthaler
Copy link
Contributor Author

dthaler commented Feb 29, 2024

@elazarg I believe I've now addressed your comments. Please re-review.

@elazarg elazarg merged commit 4203e99 into vbpf:main Feb 29, 2024
13 checks passed
@dthaler dthaler deleted the callx branch February 29, 2024 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants