-
Notifications
You must be signed in to change notification settings - Fork 98
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
SIMD-0179: SBPF stricter verification constraints #179
base: main
Are you sure you want to change the base?
Conversation
d601cb6
to
90a02dc
Compare
90a02dc
to
206c40e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these stricter rules, especially those around restricting jump destinations at runtime, have performance implications. Are these stricter checks absolutely necessary? If so, I think there are other ways of achieving this security without compromising performance.
## Detailed Design | ||
|
||
The following must go into effect if and only if a program indicates the SBF | ||
version XX or higher in its ELF header e_flags field, according to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's specify this version
`call imm` (opcode `0x85`) must only be allowed to jump to a program counter | ||
previously registered as the start of a function. Otherwise | ||
`VerifierError::InvalidFunction` must be thrown. Functions must be registered | ||
if they are present in the symbol table. The entrypoint to the program must | ||
also define a valid function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this require a double pass through the bytecode to pick up all valid call destinations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The symbol table is not the bytecode. Call destinations in the bytecode are not registered as functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better wording might be:
Functions are registered by presence in the symbol table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functions are registered by presence in the symbol table.
I was going to adopt your wording, but basically I already say this:
Functions must be registered if they are present in the symbol table.
Is this dubious or ambiguious?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue is that it is unclear if there are other ways for functions to be registered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea - could we specify that we are treating the symbol table as the source of truth for what is considered a valid call dest? 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I included more details on this.
The jump destination of `callx` (opcode `0x8D`) must be checked during | ||
execution time to match the initial address of a registered function. If this | ||
is not the case, a `EbpfError::UnsupportedInstruction` must be thrown. This | ||
measure is supposed to improve security of programs, disallowing the malicious | ||
use of callx. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has performance implications - is this absolutely necessary? If this check is necessary, can we instead have the compiler mark the beginning of valid jump destinations by emitting a marker bytecode opcode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we specify exactly how these rules are enforced:
- Whether the check is performed at runtime or verification time
- The exact logic (in pesudo-code)
- Whether these can be checked in a single pass
Several of these seem like they would have performance implications, and precisely specifying them would help to determine if this is the case or not.
All jump instructions, except for `call` (opcode `0x85`) and `callx` (opcode | ||
`0x8D`), must now jump to a code location inside their own function. Jumping | ||
to arbitrary locations hinders a precise program verification. | ||
`VerifierError::JumpOutOfCode` must be thrown for offending this rule. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this enforced? Is this enforced in the verifier or at runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a check at verification time (see error code) and is easy to enforce:
The verifier scans all instructions and keeps track of which function it is currently in, then checks jump targets against that range. The current function tracking can be done in constant time (per scanned instruction) by checking the next symbol in the dynamic symbol table and then advancing that pointer once a function boundary is crossed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add this description^ to the SIMD? 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
|
Could we add more details about these checks and their performance properties to the SIMD itself, in pseudo-code? I was reading the SIMD without looking at the code, and a lot of these performance properties weren't clear until I read the code. After reading the code most of my concerns turned out to have been unfounded - it would be great to move towards a world where SIMDs can be read independently of the code. |
Just to be clear, our suggestion was
|
d202929
to
128cf8f
Compare
No description provided.