-
Notifications
You must be signed in to change notification settings - Fork 86
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-0189: SBPF stricter ELF headers #189
base: main
Are you sure you want to change the base?
Conversation
fd54891
to
084da2b
Compare
084da2b
to
eb17010
Compare
@@ -0,0 +1,131 @@ | |||
--- | |||
simd: '0187' |
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 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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is this SIMD associated with an SBPF version?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
If any check fails `ElfParserError::InvalidProgramHeader` must be thrown. | ||
|
||
### Dynamic 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No description provided.