Stream: git-wasmtime

Topic: wasmtime / PR #5950 cranelift-interpreter: Add cross stac...

Wasmtime GitHub notifications bot (Mar 07 2023 at 09:35):

jan-justin opened PR #5950 from cranelift-interpreter-trap-xss-access to main:

This PR adds a check and trap on InterpreterState::checked_{load,store} across stack slots, which should be prevented since stack slots may be re-ordered.

The relevant details of the bug can be found over at #5927.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Mar 07 2023 at 09:40):

jan-justin submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 09:40):

jan-justin created PR review comment:

I saw that InterpreterState::stack_address utilises sized_stack_slots and as far as I can tell the stack slot order is preserved by PrimaryMap.

However, I am not sure how to account for padding between the slots at this point in time.

Wasmtime GitHub notifications bot (Mar 07 2023 at 16:18):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 16:18):

afonso360 created PR review comment:

We shouldn't have to worry about padding between slots since the interpreter doesn't do anything like that. And yeah, we always insert them in order, so we should be ok there too.

Wasmtime GitHub notifications bot (Mar 07 2023 at 17:14):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Mar 07 2023 at 18:29):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 18:29):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 18:29):

afonso360 created PR review comment:

We probably don't want to panic here. We can reach this condition by doing something like:

v0 = stack_addr ss0
v1 = iadd_imm v0, 0xFFFFFF

Which would normally generate a OutOfBoundsLoad / Store instead of panicking

Wasmtime GitHub notifications bot (Mar 07 2023 at 19:06):

jan-justin submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 19:06):

jan-justin created PR review comment:

I see. Thanks for the clarification!

Wasmtime GitHub notifications bot (Mar 07 2023 at 19:19):

jan-justin submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 19:19):

jan-justin created PR review comment:

I originally thought that since the check that precedes this is the check for storing/loading beyond the stack, it would be caught. However, in hindsight it does not appear to be sufficient. I will gladly change it.

Wasmtime GitHub notifications bot (Mar 07 2023 at 19:36):

jan-justin updated PR #5950 from cranelift-interpreter-trap-xss-access to main.

Wasmtime GitHub notifications bot (Mar 07 2023 at 19:41):

jan-justin updated PR #5950 from cranelift-interpreter-trap-xss-access to main.

Wasmtime GitHub notifications bot (Mar 29 2023 at 17:36):

jameysharp closed without merge PR #5950.

Last updated: Dec 23 2024 at 12:05 UTC