Stream: git-wasmtime

Topic: wasmtime / PR #4723 Fix a soundness issue with lowering v...

Wasmtime GitHub notifications bot (Aug 16 2022 at 20:04):

alexcrichton opened PR #4723 from fix-host-leakage to main:

This commit fixes a soundness issue lowering variants in the component
model where host memory could be leaked to the guest module by accident.
In reviewing code recently for Val::lower I noticed that the variant
lowering was extending the payload with ValRaw::u32(0) to
appropriately fit the size of the variant. In reading this it appeared
incorrect to me due to the fact that it should be ValRaw::u64(0) since
up to 64-bits can be read. Additionally this implementation was also
incorrect because the lowered representation of the payload itself was
not possibly zero-extended to 64-bits to accommodate other variants.

It turned out these issues were benign because with the dynamic
surface area to the component model the arguments were all initialized
to 0 anyway. The static version of the API, however, does not initialize
arguments to 0 and I wanted to initially align these two implementations
so I updated the variant implementation of lowering for dynamic values
and removed the zero-ing of arguments.

To test this change I updated the debug mode of adapter module
generation to assert that the upper bits of values in wasm are always
zero when the value is casted down (during stack_get which only
happens with variants). I then threaded through the debug boolean
configuration parameter into the dynamic and static fuzzers.

To my surprise this new assertion tripped even after the fix was
applied. It turns out, though, that there was other leakage of bits
through other means that I was previously unaware of. At the primitive
level lowerings of types like u32 will have a Lower representation
of ValRaw and the lowering is simply dst.write(ValRaw::i32(self)),
or the equivalent thereof. The problem, that the fuzzers detected, with
this pattern is that the ValRaw type is 16-bytes, and
ValRaw::i32(X) only initializes the first 4. This meant that all the
lowerings for all primitives were writing up to 12 bytes of garbage from
the host for the wasm module to read.

It turned out that this write of a ValRaw was sometimes 16 bytes and
sometimes the appropriate size depending on the number of optimizations
in play. With enough inlining for example dst.write(ValRaw::i32(self))
would only write 4 bytes, as expected. In debug mode though without
inlining 16 bytes would be written, including the garbage from the upper
bits.

To solve this issue I ended up taking a somewhat different approach. I
primarily updated the ValRaw constructors to simply always extend the
values internally to 64-bits, meaning that the low 8 bytes of a ValRaw
is always initialized. This prevents any undefined data from leaking
from the host into a wasm module, and means that values are also
zero-extended even if they're only used in 32-bit contexts outside of a
variant. This felt like the best fix for now, though, in terms of
not really having a performance impact while additionally not requiring
a rewrite of all lowerings.

This solution ended up also neatly removing the "zero out the entire
payload" logic that was previously require. Now after a payload is
lowered only the tail end of the payload, up to the size of the variant,
is zeroed out. This means that each lowered argument is written to at
most once which should hopefully be a small performance boost for
calling into functions as well.

<!--

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 (Aug 16 2022 at 20:04):

alexcrichton requested fitzgen for a review on PR #4723.

Wasmtime GitHub notifications bot (Aug 16 2022 at 20:46):

alexcrichton updated PR #4723 from fix-host-leakage to main.

Wasmtime GitHub notifications bot (Aug 16 2022 at 21:47):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Aug 16 2022 at 21:56):

alexcrichton has enabled auto merge for PR #4723.

Wasmtime GitHub notifications bot (Aug 16 2022 at 22:33):

alexcrichton merged PR #4723.

Last updated: Nov 22 2024 at 16:03 UTC