Wasmtime GitHub notifications bot (Jan 20 2023 at 16:37): Wasmtime GitHub notifications bot (Jan 20 2023 at 16:37):alexcrichton commented on issue #5608:
I'll note that I've confirmed at least on x86_64 and arm64 that this does not slow down the
Module::deserialize_fileoperation. Specifying the write permission, and not actually writing to anything, means everything is still fast on Linux.
Wasmtime GitHub notifications bot (Jan 20 2023 at 18:25):bjorn3 commented on issue #5608:
Would it be possible to use GOT relative relocations for libcalls and then only make the GOT writable while applying the relocations and make it read-only again afterwards? Or maybe even store libcall addresses in the VMContext to keep the whole module read-only?
Wasmtime GitHub notifications bot (Jan 20 2023 at 20:11):alexcrichton commented on issue #5608:
If an attacker did manage to write to the non-executable pages, what would the impact be?
My current understanding is that if an attacker could pave over the image then that means they have an arbitrary write primitive so while making the image readonly wouldn't hurt it's not like that would protect the actual malloc heap or any other state in a process. In that sense I don't think the impact would be any worse or better with a readonly image.
My main motivation for switching things to read/write though was the aspect that in-memory compiled modules are already read/write. I don't think it's out of the cards by any means to make the entire image readonly but if we were to implement that I think we'd want to implement it for all compiled images and not just those deserialized from files.
Would it be possible to use GOT relative relocations for libcalls ... ?
While I'm not 100% sure how the GOT works my impression is that it's a table of function pointers that's filled in at runtime but nothing else needs to be filled in at runtime. If that's true then while it would improve the performance of resolving these relocations (today it's O(relocations) and with a GOT it'd be O(symbols-to-relocate-against)) I don't think it would change the calculus here much. Today file-mapped images are completely readonly so we still couldn't apply relocations.
Now you're right, though, that ideally we wouldn't have any relocations here at all. The problem with these though is that the libcall is introduced very late in the codegen pipeline (at lowering) which means there's no guaranteed access to a vmctx so loading from the vmctx isn't necessarily possible. We'd have to patch out the clif instructions during the wasm -> clif lowering stage to do libcalls via the vmctx but that means we'd be baking in target-specific lowering information to that lowering pass which would be a bit of a bummer.
I should clarify, I don't think that a readonly image would really cause issues at all. There's a part of the publication process for an image to make it executable where we make the text section read/execute (e.g. removing the write bit), and we could pretty easily make the whole image readonly just before that (this would happen just after relocations are applied)
Wasmtime GitHub notifications bot (Jan 20 2023 at 21:19):bjorn3 commented on issue #5608:
While I'm not 100% sure how the GOT works my impression is that it's a table of function pointers that's filled in at runtime but nothing else needs to be filled in at runtime.
Indeed. This only applies to code relocations. Data relocations are unchanged as it is impossible to handle the indirection. For Wasmtime that isn't relevant though as it only has code relocations.
The problem with these though is that the libcall is introduced very late in the codegen pipeline (at lowering) which means there's no guaranteed access to a vmctx so loading from the vmctx isn't necessarily possible.
AFAIK the backend does know about vmctx, albeit as an opaque value. I guess it would be possible to add a flag to define the offset to load libcall addresses. Even so I do agree this is probably not the way to go.
Wasmtime GitHub notifications bot (Jan 23 2023 at 15:15):github-actions[bot] commented on issue #5608:
Subscribe to Label Action
cc @fitzgen
<details>
This issue or pull request has been labeled: "fuzzing"Thus the following users have been cc'd because of the following labels:
- fitzgen: fuzzing
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Jan 25 2023 at 15:52):alexcrichton commented on issue #5608:
@jameysharp would you be up for reviewing this? If not no worries, I can pass it along.
Wasmtime GitHub notifications bot (Jan 25 2023 at 16:54):jameysharp commented on issue #5608:
I think this PR would be a clear improvement over the _status quo_ if it left non-executable pages read-only (for both disk-backed and memory-only maps). You may very well be correct that if someone can write to these pages then we have bigger problems, but I'm not personally comfortable giving it an r+ as-is. If other stakeholders who rely on Wasmtime for multi-instance isolation decide that's safe, I'd be happy to see this merged.
Wasmtime GitHub notifications bot (Jan 25 2023 at 17:15):alexcrichton commented on issue #5608:
Ok I've pushed a commit to do that. To be clear though I'm still wary of this. Changing memory permissions can often lead to scaling issue w.r.t. IPIs and such. I continue to believe it's not necessary here since if you can overwrite random memory in the image there's basically no reason you can't overwrite anything else. I won't disagree it's a defense-in-depth thing, though.
Last updated: Oct 23 2024 at 20:03 UTC