Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Emulator: Improve usability by restricting memory region access for guest programs #727

Merged
merged 5 commits into from
Jul 20, 2023
Merged

Emulator: Improve usability by restricting memory region access for guest programs #727

merged 5 commits into from
Jul 20, 2023

Conversation

SchmErik
Copy link
Contributor

@SchmErik SchmErik commented Jul 19, 2023 •
edited

Memory regions outside of the address range TEXT_START to SYSTEM.start() - 1 is
used to store information related to the zkVM. Tampering with this region will
cause the prover to crash. Restrict the guest from writing to this region.

Fixes: #672

@github-actions
Copy link

Benchmark for Linux-cuda d32cbd0

Click to hide benchmark
Test Base PR %
fib/100/execute 5.1±0.07ms 5.0±0.09ms -1.96%
fib/100/prove 893.3±2.29ms 889.9±0.75ms -0.38%
fib/100/total 898.5±2.20ms 896.7±2.53ms -0.20%
fib/1000/execute 5.5±0.10ms 5.5±0.11ms 0.00%
fib/1000/prove 932.6±1.83ms 929.7±1.06ms -0.31%
fib/1000/total 937.5±1.55ms 937.1±2.57ms -0.04%
fib/10000/execute 9.8±0.17ms 9.8±0.12ms 0.00%
fib/10000/prove 3.7±0.01s 3.7±0.01s 0.00%
fib/10000/total 3.7±0.01s 3.7±0.01s 0.00%

Benchmark for Linux-default

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for macOS-default d32cbd0

Click to hide benchmark
Test Base PR %
fib/100/execute 2.7±0.10ms 2.7±0.05ms 0.00%
fib/100/prove 3.7±0.05s 3.7±0.04s 0.00%
fib/100/total 3.7±0.05s 3.7±0.05s 0.00%
fib/1000/execute 2.9±0.11ms 2.9±0.06ms 0.00%
fib/1000/prove 3.7±0.04s 3.7±0.05s 0.00%
fib/1000/total 3.7±0.06s 3.7±0.06s 0.00%
fib/10000/execute 5.1±0.08ms 5.0±0.04ms -1.96%
fib/10000/prove 15.3±0.12s 15.2±0.07s -0.65%
fib/10000/total 15.3±0.14s 15.3±0.08s 0.00%

Benchmark for macOS-metal

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

@SchmErik
Copy link
Contributor Author

I'm not sure if we want this commit bd843b8

I realized while adding all of those changes that returning Result on everything is a related but separate task

@github-actions
Copy link

Benchmark for Linux-cuda

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for Linux-default e58a871

Click to hide benchmark
Test Base PR %
fib/100/execute 9.0±4.72ms 4.8±0.17ms -46.67%
fib/100/prove 2.7±0.88s 1715.7±100.57ms -36.46%
fib/100/total 2.4±0.22s 2.0±0.12s -16.67%
fib/1000/execute 9.6±4.38ms 5.3±0.24ms -44.79%
fib/1000/prove 2.6±0.36s 1734.3±34.04ms -33.30%
fib/1000/total 2.4±0.27s 2.2±0.31s -8.33%
fib/10000/execute 11.9±1.39ms 9.7±0.08ms -18.49%
fib/10000/prove 8.1±1.50s 6.7±0.24s -17.28%
fib/10000/total 7.7±0.70s 7.4±0.38s -3.90%

Benchmark for macOS-default

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for macOS-metal

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

@SchmErik SchmErik requested a review from flaub July 19, 2023 22:16
SchmErik
SchmErik commented Jul 19, 2023
View reviewed changes
risc0/zkvm/src/lib.rs Outdated Show resolved Hide resolved
flaub
flaub approved these changes Jul 19, 2023
View reviewed changes
Copy link
Member

@flaub flaub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the unstable feature, I think this is great!

@SchmErik SchmErik force-pushed the erik/guest-mem-access branch 2 times, most recently from 1702264 to bc9dec3 Compare July 20, 2023 05:59
@SchmErik SchmErik enabled auto-merge (squash) July 20, 2023 06:01
@SchmErik SchmErik mentioned this pull request Jul 20, 2023
Open
@SchmErik
tests: add out of bounds memory access test
04cda48
@SchmErik
Emulator: Improve usability by restricting memory access for guest pr…
8feb63d 
…ograms

Memory regions outside of the address range `TEXT_START` to `SYSTEM.start() -
1` are used to store information related to the zkVM. Tampering with this
region will cause the prover to crash. Restrict the guest from writing to this
region.
@SchmErik
Executor: fix panic by guarding against invalid page indicies
5d4460f 
It was possible for the executor to panic when accessing higher regions of
memory. This change fixes the panics by checking that the address trying to be
accessed has a valid page index.
@SchmErik
replace unstable feature
3c4b2de
@SchmErik
Add Frank's suggestion
97c483f
@github-actions
Copy link

Benchmark for Linux-cuda

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for Linux-default

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for macOS-default

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for macOS-metal

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

@SchmErik SchmErik merged commit b0c7fce into main Jul 20, 2023
12 checks passed
@SchmErik SchmErik deleted the erik/guest-mem-access branch July 20, 2023 07:29
capossele pushed a commit that referenced this pull request Aug 7, 2023
@SchmErik @capossele
Emulator: Improve usability by restricting memory region access for g…
8206a5c 
…uest programs (#727)

Memory regions outside of the address range `TEXT_START` to `SYSTEM.start() -
1` are used to store information related to the zkVM. Tampering with this
region will cause the prover to crash. Restrict the guest from writing to this
region.

* Executor: fix panic by guarding against invalid page indicies

It was possible for the executor to panic when accessing higher regions of
memory. This change fixes the panics by checking that the address trying to be
accessed has a valid page index.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flaub flaub flaub approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fix panic when guest accesses higher regions of memory
2 participants
@SchmErik @flaub