Create fixed, safe interface for MPRV and use it to copy data to/from SM

keystone-enclave / riscv-pk

Security monitor for Keystone Enclave (mirror of riscv-pk). Will be deprecated when openSBI port is ready

Other

36 stars 14 forks source link

Create fixed, safe interface for MPRV and use it to copy data to/from SM #54

Closed archshift closed 4 years ago

archshift commented 4 years ago

Before merging, make sure the corresponding PRs in linux-keystone-driver (https://github.com/keystone-enclave/linux-keystone-driver/pull/34) and the runtime (https://github.com/keystone-enclave/keystone-runtime/pull/25) are merged.

Also builds on cleanups in #57.

archshift commented 4 years ago

Phew. Finally!

archshift commented 4 years ago

@dayeol @dkohlbre This should be ready for proper review. The meat+potatoes of this is in mprv.S, but I also made a change to some locking behavior in the last commit which I'd like another pair of eyes on.

archshift commented 4 years ago

I tested it previously but I'll run tests again to double check after the refactoring

archshift commented 4 years ago

@dayeol It properly still catches errors I insert while copying. But it seems to have some issues properly returning from the SBI call. I'll investigate.

archshift commented 4 years ago

I think there are two issues: this PR doesn't properly restore mstatus, and that returning a failure code from the end of create_enclave in general causes some piece of keystone to segfault, even without calling the copying code.

dayeol commented 4 years ago

Does this require a new issue?

archshift commented 4 years ago

I think so. I'll make one and then update this issue to fix the other bug.

archshift commented 4 years ago

Filed at https://github.com/keystone-enclave/keystone/issues/169.

archshift commented 4 years ago

MPRV copying with an invalid address now results in the same failure as skipping the copy and returning an error.

archshift commented 4 years ago

@dayeol tested able to recover from errors injected into an MPRV copy with #66.

dayeol commented 4 years ago

LGTM, I'd like to merge this after @dkohlbre reviews as well.

dkohlbre commented 4 years ago

I'm running (I believe) the correct set of PRs in QEMU and getting attestation test failures:

# ./tests.ke 
Verifying archive integrity... All good.
Uncompressing Keystone vault archive
testing stack
testing fibonacci
testing long-nop
testing loop
testing malloc
testing fib-bench
testing untrusted
Enclave said: hello world!
Enclave said: 2nd hello world!
Enclave said value: 13
Enclave said value: 20
testing attestation
Attestation report is invalid

Can you confirm that with these 3 PRs you were seeing the attestation test pass? My assumption is if its failing its due to a slightly bad copy.

archshift commented 4 years ago

Can you try running the ./tests.ke directly in the QEMU console to see if it's printing out any messages?

The issue could also be applying the PRs without blowing away the build files in different locations... most sneakily, $KEYSTONE_DIR/tests/tests needs to be cleaned and rebuilt.

dkohlbre commented 4 years ago

That particular failure does come from directly running in QEMU, not travis tests/etc. I cleaned up tests, but its plausible I missed something. I'll make sure its fully clean next try.

archshift commented 4 years ago

I just tried a clean build again and am getting a pass from the attestation report. Other things that might be off:

$KEYSTONE_SDK_DIR
A clean rebuild in the eyrie/ directory?
A clean rebuild in the sdk/ directory?
Wiped build/ directory?

I'm just running CMake with the default settings, too, in case that's differing.

dkohlbre commented 4 years ago

OK Confirming that this does work for me on hifive board/etc. I'll be landing all these and putting together the dev bump.