-
Notifications
You must be signed in to change notification settings - Fork 179
SIMD-0219: Stricter ABI and Runtime Constraints #219
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
base: main
Are you sure you want to change the base?
SIMD-0219: Stricter ABI and Runtime Constraints #219
Conversation
6a9502d
to
9c1578c
Compare
9c1578c
to
eaf4fe6
Compare
eaf4fe6
to
1f3dda9
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.
Nice! I think the fixes for the frame gaps and the memory accesses across regions make sense. However, I pushed back on the stack/heap pointers for CPI verification.
Also, I'm wondering if it makes sense to break this into three SIMDs? It seems like we could do each in isolation, which might help speed up the approval & implementation process.
- The following pointers must be on the stack or heap, | ||
meaning their virtual address is inside `0x200000000..0x400000000`, | ||
otherwise `SyscallError::InvalidPointer` must be thrown: | ||
- The pointer in the array of `&[AccountInfo]` / `SolAccountInfo*` | ||
- The `AccountInfo::data` field, | ||
which is a `RefCell<&[u8]>` in `sol_invoke_signed_rust` | ||
- The `AccountInfo::lamports` field, | ||
which is a `RefCell<&u64>` in `sol_invoke_signed_rust` | ||
- The following pointers must point to what was originally serialized in the | ||
input regions by the program runtime, | ||
otherwise `SyscallError::InvalidPointer` must be thrown: | ||
- `AccountInfo::key` / `SolAccountInfo::key` | ||
- `AccountInfo::owner` / `SolAccountInfo::owner` | ||
- `AccountInfo::lamports` / `SolAccountInfo::lamports` | ||
- `AccountInfo::data::ptr` / `SolAccountInfo::data` |
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.
In my opinion, this constraint forces programs to waste limited stack/heap space just to hold these pointer structures, when we can probably validate these in a different way, and keep them in the input region.
On Agave, when we serialize all of the accounts into the input region, we actually hang onto a bunch of the pointers in SyscallContext
/SerializedAccountMetadata
. We can just store more pointer information in the syscall context, and perform a very quick pointer analysis:
- Before each CPI frame is created
- When the program execution finishes
We can catch the violations then, and throw the SyscallError::InvalidPointer
. This way, instead of imposing location constraints, we simply validate that all AccountInfo
pointers match their original pointers when the VM was created.
As mentioned in the proposal, when direct mapping is enabled, these kinds of pointer violations will throw immediately, which means pushing the pointers to the stack/heap would only be necessary as a temporary measure, until the DM feature is enabled. Afterwards, programs are stuck with this constraint for no reason.
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 think you are misunderstanding. This SIMD says that the AccountInfo
structs need to be on stack or heap, not that the data they point to has to be there. The data they point to remains in the account serialization. This is compatible with the SDK entrypoint as is, and has to be so that we don't break existing programs.
Think part of the confusion comes from the fact that AccountInfo::data
and AccountInfo::lamports
have two levels of indirection and both outer and inner pointer need to not be in the account serialization (the final deref however must be), what no program ever does anyway.
We can just store more pointer information in the syscall context, and perform a very quick pointer analysis
Yes exactly, that is how direct mapping is implemented and what the SIMD is ought to describe. Seems I should reformulate it.
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 think you are misunderstanding. This SIMD says that the
AccountInfo
structs need to be on stack or heap, not that the data they point to has to be there.
I never considered this SIMD to be implying we should move any serialized data to the stack or heap. I believe I'm understanding that part correctly.
What I'm maybe misunderstanding is what the goal is by limiting where AccountInfo
pointers can be created. The sol_invoke_signed
syscall accepts only pointers.
fn sol_invoke_signed_c(
instruction_addr: *const u8,
account_infos_addr: *const u8, // <-- Pointer to slice of `SolAccountInfo`
account_infos_len: u64,
signers_seeds_addr: *const u8,
signers_seeds_len: u64,
) -> u64
struct SolAccountInfo {
key_addr: u64,
lamports_addr: u64,
data_len: u64,
data_addr: u64,
owner_addr: u64,
rent_epoch: u64,
is_signer: bool,
is_writable: bool,
executable: bool,
}
Since translation of accounts just deref's out of the RefCell
trackers, we can consider the two translations (Rust and C) identical for serialized accounts.
Enforcing that each account pointer lives on stack or heap doesn't seem to actually solve the problem, which is the ability to pass a pointer to an invalid input-region SolAccountInfo
into CPI. Furthermore, there are multiple perfectly valid reasons you'd pass a pointer to already-serialized legitimate accounts, such as avoiding copies.
My initial point of just evaluating all pointers against the SerializedAccountMetadata
would solve this problem at the fundamental level. It can happen during translate_accounts
or translate_account_infos
, where you've already got the stack/heap check implemented.
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.
My initial point of just evaluating all pointers against the
SerializedAccountMetadata
would solve this problem at the fundamental level.
That is what the SIMD already proposes anyway: "The following pointers must point to what was originally serialized in the input regions by the program runtime" which refers to the SerializedAccountMetadata
.
But, that alone is insufficient, CPI also writes to AccountInfo
as it returns, it should never be writing to an account during the returning (returning takes multiple steps after all) as that can result in iterate-while-modifying issues and violates Rust borrow checker rules.
This entire SIMD is describing the account data direct mapping feature, which is already implemented, just not feature gated. I know from a dApp perspective they look like a bunch thrown together, but in the program runtime they are all interconnected. So I don't think that splitting them would speed things up, but rather make it much more complex. |
Actually I am talking about the program runtime! Right now, the plan is to just switch on direct mapping all at once with a feature gate, which will enable all of these constraints - as well as the implementation itself - immediately. That's a lot of change area in one feature gate, and I think some contributors are getting a bit nervous about the "all at once" approach. What I'm instead suggesting is that we introduce these constraints piecemeal. You can break VM constraints into three separate SIMDs, with three separate feature gates, and activate them one by one:
This approach would allow us to more easily address any security issues that might arise from just one "phase" of direct mapping constraints. Later, once all three are activated, you can make direct mapping a validator startup flag for a while, before we just remove the flag altogether and make it the de facto hot path.
Overall I think this seems like a much safer approach to getting this all in. What do you think? |
I think the change to the stack frame gaps is risky and not necessary anymore for the current implementation of direct mapping so we could revert it. The restrictions to the Finally, the tough nut is the memory access violations. These are interwoven with the implementation switching to direct mapping. It is hard to correctly emulate these restrictions without implementing direct mapping. Decoupling them into a feature gate of pure restrictions which still uses the copy based serialization path is complex. |
40f9989
to
56f2cc5
Compare
56f2cc5
to
25933db
Compare
- The access is completely within the rest of the account growth budget of the | ||
transaction, otherwise `InstructionError::InvalidRealloc` must be thrown. | ||
- The access is completely within the current length of the account, | ||
otherwise extend the the account with zeros to the maximum allowed by 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.
otherwise extend the the account with zeros to the maximum allowed by the | |
otherwise extend the account with zeros to the maximum allowed by the |
25933db
to
2c6fc74
Compare
No description provided.