SIMD-0189: SBPF stricter ELF headers #189
Conversation
Lichtso
force-pushed
the
sbpf-stricter-elf-headers
branch
6 times, most recently
from
fd54891 to
084da2b
Compare
Lichtso
force-pushed
the
sbpf-stricter-elf-headers
branch
from
084da2b to
eb17010
Compare
| @@ -0,0 +1,131 @@ | |||
| --- | |||
| simd: '0187' | |||
There was a problem hiding this comment.
Can you update the SIMD number (and PR title) to match the PR number, per https://github.com/solana-foundation/solana-improvement-documents/blob/main/proposals/0001-simd-process.md#draft
Lichtso
changed the title
Lichtso
force-pushed
the
sbpf-stricter-elf-headers
branch
from
eb17010 to
51bec42
Compare
| - `p_memsz` must fit in 32 bits / be less than `1 << 32` | ||
| - `p_align` is ignored | ||
|
|
||
| If any check fails `ElfParserError::InvalidProgramHeader` must be thrown. |
There was a problem hiding this comment.
Do you mind specifying where in the program lifecycle this will be thrown? (i.e. will these new checks be enforced at deployment time? when loading into the program cache for execution? both?)
What is the plan for currently deployed programs?
There was a problem hiding this comment.
Or is this SIMD associated with an SBPF version?
There was a problem hiding this comment.
It is associated with a SBPF version (see title), though the specific one is not decided yet.
And these checks will all happen at load time (deployment or feature set change), not at runtime.
There was a problem hiding this comment.
Could we specify in the SIMD where the checks will be performed, and what happens if they fail?
| | stack | PT_GNU_STACK | PF_R, PF_W | 2 << 32 | | ||
| | heap | PT_LOAD | PF_R, PF_W | 3 << 32 | |
There was a problem hiding this comment.
Why do we allow these headers? These regions are configured independently of the ELF. Do we allow these headers just so that we can reduce LLVM modifications? Just curious.
There was a problem hiding this comment.
It is so that the memory regions are correctly reflected in the ELF header. This makes it easier for tooling to detect issues (such as overlap in section allocation) and could simplify the validator implementation in the future.
Also, with the upcoming CPI syscall restrictions (necessary for direct mapping) we will limit pointers provided by the user to come from the stack and heap. Thus their layout will be further solidified.
There was a problem hiding this comment.
Not all programs make use of stack/heap/symbols. A noop program, for example, should be just 128 bytes (ELF header + 1 PT_LOAD PF_X + 1 exit insn). Enforcing 5 headers, 4 of which aren't even used, would bloat it to 352 bytes - even larger than the current mainnet-beta minimum of 336 bytes.
On the other hand, it's great to be able to handle for memory layouts that diverge from the default of a specific version without having to bump versions should the need arise. As such, I think it would be better not to strictly enforce these headers, but to instead opt for a solution like:
- First header is always PT_LOAD PF_X. This is the only mandatory header.
- For anything with .rodata, add a PT_LOAD PF_R header.
- For anything with symbols, add PT_NONE 0x00.
- For anything with non-standard stack layout for its version, add PT_GNU_STACK PF_R | PF_W
- For anything with non-standard heap layout for its version, add PT_LOAD PF_R | PF_W
This solution would make the minimum program use 1 program header, the average program use 3 program headers, and if we ever had memory layout issues, they could be solvable for the small price of 1 or 2 extra program headers.
|
|
||
| If any check fails `ElfParserError::InvalidProgramHeader` must be thrown. | ||
|
|
||
| ### Dynamic symbol table |
There was a problem hiding this comment.
Why did you choose the dynamic symbol table entry SHT_DYNAMIC and not the symbol table elf entry SHT_SYMTAB?
There was a problem hiding this comment.
No description provided.