Add support for fuchsia's CPRNG source #765

erickt · 2019-01-18T20:15:04Z

This is another attempt for ring to support fuchsia, by directly calling zx_cprng_draw to generate random byte strings. This avoids having to pull in an extra dependency (which #634 did). With this change, all the ring tests pass on fuchsia.

Closes #428

briansmith · 2019-01-18T20:32:46Z

Hi, although when I was reviewing #634 I was initially resistant to pulling in the Rust crate, my understanding is that somebody fixed the issue in cargo where the Fuchsia crate was being downloaded for non-Fuchsia targets, so my concern there doesn't apply anymore. Therefore, if you would prefer to have ring use the Rust wrapper around that function, I am happy to take that change. I would actually prefer to do it that way.

The second issue I had with the original PR was the way it handled the short read behavior of the older Fuchsia PRNG API. My understanding is that there are no short reads to worry about any more, right?

The final issue with the original PR was that it isn't clear, to me, how to test this. Originally I was hoping somebody would provide a way to have Fuchsia builds and tests done automatically in CI just like we have for all other platforms. I think that should be a long-term goal. But for now, could you share the minimal instructions for running the ring tests on Fucshia, so I can do it manually?

erickt · 2019-01-18T22:04:53Z

Hello Brian!

Yeah, it looks like the cargo issue you filed about limiting downloading target dependencies Add the ability to restrict what crates cargo fetch downloads by target rust-lang/cargo#5216 was fixed in cargo-fetch: add option to fetch for a target rust-lang/cargo#5349. I'll switch over to using fuchsia-zircon crate.
Regarding the short read, I forgot to mention we fixed the API on our side. Now zx_cprng_draw you get back the number of bytes you requested.
Regarding testing, I completely understand. I'm trying to see if we can publish nightly-ish qemu images that can be consumed by travis. In the meantime, to run the test suite you need to follow this guide which I'll try to summarize below:

Check out a subset of our code (which is zircon, the kernel, and garnet, the user space system layer):

% curl -s "https://fuchsia.googlesource.com/scripts/+/master/bootstrap?format=TEXT" | base64 --decode | bash -s garnet

Next, tell the build to produce x64 debug build with:

% fx set x64 --args 'extra_authorized_keys_file="//.ssh/authorized_keys"'

If you want to use ccache, throw in a --ccache to that.

Next, start the build and get something to drink :)

% fx full-build

Next, bring up a VM (note this requires setting up a tun/tap device):

% fx run -N

Now everything should be built, so we now have to build ring. For that, we have a wrapper tool around cargo called fargo that will build crates setup to point at the fuchsia system you just built:

% git clone https://fuchsia.googlesource.com/fargo/
% cd fargo
% cargo install --force --path .

Finally, to actually build and run the tests do:

% cd ring
% fargo test
 % fargo test 
    Finished dev [unoptimized + debuginfo] target(s) in 0.05s
     Running target/x86_64-fuchsia/debug/deps/ring-4af73f10c2056908
running 99 tests
test aead::aes_gcm::tests::max_input_len_test ... ok
test aead::block::tests::test_bitxor_assign ... ok
test aead::chacha::tests::chacha20_tests ... FAILED
...
test result: FAILED. 51 passed; 48 failed; 0 ignored; 0 measured; 0 filtered out

error: ssh failed: exit code: 101
error: test failed, to rerun pass '--lib'
error: cargo exited with status ExitStatus(ExitStatus(256))

Those tests are failing because fargo doesn't know that it has to copy around artifacts like src/aead/aes_tests.txt. So to get around it, I just did:

% fx scp -r src "[`fx netaddr -fuchsia`]:/tmp/src"
% fx scp -r crypto "[`fx netaddr -fuchsia`]:/tmp/crypto"
% fx shell "cd tmp; ./ring-4af73f10c2056908_stripped"

running 99 tests
...
test result: ok. 99 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

Hurrah! It worked! But that wasn't super pleasant. Hopefully we can find some better way to do this.

Oh and one thing to note if you do get into fuchsia, ctrl-c doesn't work. It'll just kill your session, rather than interrupt something that's running. The joys of writing a new operating system :)

Please let me know if you run into any trouble.

erickt · 2019-01-18T22:56:06Z

Also, if you aren't used to qemu, to quit, do ctrl-A x to quit the virtual machine.

erickt · 2019-01-19T04:57:45Z

fyi, we need to do a bump of fuchsia-zircon, so that'll push this PR into next week at the earliest.

erickt · 2019-01-24T22:58:59Z

@briansmith I've updated the PR to use a new crate we just published called fuchsia-cprng that only exposes the CPRNG syscall. This reduces your exposure to us changing syscalls, since we don't expect to change this particular syscall. Please let me know if there's anything else you need from us.

Also, if you do accept this, would it be possible to get a new release cut so that we can use ring on fuchsia? Thanks so much for everything!

coveralls · 2019-01-25T00:21:35Z

Coverage increased (+0.004%) to 93.615% when pulling 184a5f2 on erickt:fuchsia into 88d3279 on briansmith:master.

briansmith · 2019-01-26T02:52:01Z

Thanks.

Please update the fuchsia-cprng crate's crate metadata with useful license metadata and a useful link to the source code. More specifically, where in the big garnet repo is fuchsia-cprng? What is the actual text of the license? If it is one of the standard licenses, then which SPDX license code?.

Now I see I don't understand what's going on with fuchsia-zircon. In the previous PR, I wanted to avoid depending on fuchsia-zircon and Fuchsia team responded that it preferred us to use fuchsia-zircon instead of directly accessing the zircon library. So, my request in this PR to use fuchsia-zircon was really about following that previous advice. Now it seems like we're supposed to to avoid the fuchsia-zircon crate completely? Is it really better to use this fuchsia-csprng crate then, or would it be better to use the original iteration of your PR?

Will you be contributing the Aarch64 (And ARM, if you support 32-bit ARM) CPU feature detection for Fuchsia in a separate PR? My understanding is that Fuchsia has its own getauxval() like mechanism for that, which ring doesn't support right now. See #773 for how I've started redoing the Linux feature detection for Aarch64 and 32-bit ARM. I would appreciate it if Fuchsia team could review that PR and write a corresponding one on top of it for Fuchsia. In the meantime, I'll spend some time getting this built and tested for x86-64.

briansmith · 2019-01-26T03:21:54Z

My understanding is that Fuchsia has its own getauxval() like mechanism

To further clarify what I'm getting at: It seems like ring needs to access two Fuchsia-specific functions: zx_system_get_features() (on AAarch and ARM) and zx_cprng_draw(). If we rely on fuchsia-csprng for zx_cprng_draw() then how would we access zx_system_get_features()? It seems like we should be consistent in how we access these two functions. In particular, I'm not sure it makes sense to access one through fuchsia-csprng and another through the (C) zircon library. WDYT?

erickt · 2019-01-26T05:50:14Z

Hello Brian!

You can find the source in our repository here, or you can see it on docs.rs. Good idea about pointing the crate repo link deeper in the tree. I'll make that update.

Regarding our licensing, I'm told that SPDX does not yet cover our license with our patent grant, so I was told we were supposed to use the license-file until SPDX adds support for them.

Regarding CPU feature detection, shucks, I didn't realize you'd need that. I'll have to do some investigation. Regarding 32bit vs 64bit, we only support arm64 and x86-64.

erickt · 2019-01-26T06:01:51Z

cc @joshlf regarding helping to review #773.

erickt · 2019-01-28T19:31:16Z

@briansmith I talked with our team, and at the moment we are still really interested in removing our dependencies on fuchsia-zircon so we can make some breaking changes to it (that are unrelated to these syscalls) without having to push changes through our external dependencies. For now, I suggest that either we skip having acceleration on aarch64 for now, or you just explicitly call out to zx_system_get_features().

Any luck getting the tests inside fuchsia up and running?

briansmith · 2019-01-28T19:53:48Z

OK, I will spend some time today trying to get zx_system_get_features() working in ring on AAarch64 Fuchsia. Then once that gets reviewed & merged we can merge this PR on top of it.

If we're going to use FFI to call zx_system_get_features() we should go back to your previous approach to using FFI to call zx_cprng_draw(). I only suggested we avoid directly using FFI for zx_cprng_draw() because that was Fuchsia team's strong preference the previous time we tried this.

erickt · 2019-01-28T23:11:30Z

I updated the PR to switch back to directly calling zx_cprng_draw and confirmed it works on my test device.

Is there a particular reason to block this PR on getting zx_system_get_features() working first? I'd be happy to help with that patch in a separate PR, but this patch (with hopefully a 0.14.4 release to go with it) is blocking us from getting ring and rustls working in general.

erickt · 2019-01-29T03:04:22Z

Is there a particular reason to block this PR on getting zx_system_get_features() working first? I'd be happy to help with that patch in a separate PR, but this patch (with hopefully a 0.14.4 release to go with it) is blocking us from getting ring and rustls working in general.

Ah, because you just merged in the cpu feature detection. I'll see what I can do on a patch for that.

erickt · 2019-01-29T04:21:31Z

I opened a separate PR for feature detection in #776.

briansmith · 2019-01-31T02:21:05Z

OK, thanks for the instructions for testing.

Even before applying this PR, I get this bad behavior using your instructions:

When I build using fargo test and run the tests using your instructions (copying the test files and running the text executable through fx shell), chacha20_tests times out. Maybe this is just because the test requires a lot of CPU time and QEMU is slow.
When I build using fargo test --release linking fails:

note: rust-lld: error: undefined symbol: __fprintf_chk
          >>> referenced by stdio2.h:97 (/usr/include/x86_64-linux-gnu/bits/stdio2.h:97)
          >>>               constant_time_test.o:(bssl_constant_time_test_main) in archive /home/m/ring/target/x86_64-fuchsia/release/build/ring-3ff4a56ba936b704/out/libring-test.a

          rust-lld: error: undefined symbol: __fprintf_chk
          >>> referenced by stdio2.h:97 (/usr/include/x86_64-linux-gnu/bits/stdio2.h:97)
          >>>               constant_time_test.o:(bssl_constant_time_test_main) in archive /home/m/ring/target/x86_64-fuchsia/release/build/ring-3ff4a56ba936b704/out/libring-test.a

          rust-lld: error: undefined symbol: __fprintf_chk
          >>> referenced by stdio2.h:97 (/usr/include/x86_64-linux-gnu/bits/stdio2.h:97)
          >>>               constant_time_test.o:(bssl_constant_time_test_main) in archive /home/m/ring/target/x86_64-fuchsia/release/build/ring-3ff4a56ba936b704/out/libring-test.a

          rust-lld: error: undefined symbol: __fprintf_chk
          >>> referenced by stdio2.h:97 (/usr/include/x86_64-linux-gnu/bits/stdio2.h:97)
          >>>               constant_time_test.o:(bssl_constant_time_test_main) in archive /home/m/ring/target/x86_64-fuchsia/release/build/ring-3ff4a56ba936b704/out/libring-test.a

          rust-lld: error: undefined symbol: __fprintf_chk
          >>> referenced by stdio2.h:97 (/usr/include/x86_64-linux-gnu/bits/stdio2.h:97)
          >>>               constant_time_test.o:(bssl_constant_time_test_main) in archive /home/m/ring/target/x86_64-fuchsia/release/build/ring-3ff4a56ba936b704/out/libring-test.a

          rust-lld: error: undefined symbol: __fprintf_chk
          >>> referenced by stdio2.h:97 (/usr/include/x86_64-linux-gnu/bits/stdio2.h:97)
          >>>               constant_time_test.o:(bssl_constant_time_test_main) in archive /home/m/ring/target/x86_64-fuchsia/release/build/ring-3ff4a56ba936b704/out/libring-test.a

I edited that file to add some #if defined(__Fuchsia__) logic to avoid calling fprintf but it seems __Fuchsia__ is not defined. That makes me think fargo test is not using the right C compiler or it isn't using the right settings for the C compiler. What am I missing?

briansmith · 2019-01-31T02:30:29Z

When I build using fargo test and run the tests using your instructions (copying the test files and running the text executable through fx shell), chacha20_tests times out. Maybe this is just because the test requires a lot of CPU time and QEMU is slow.

If I reduce the value of max_offset in chacha.rs substantially, then the tests do pass (with this PR) in a debug build.

erickt · 2019-01-31T02:55:45Z

That's wonderful it worked! I'll do some digging into why the release build is running into issues.

This adds support for detecting arm features on a fuchsia device, which uses the `zx_system_get_features` syscall to extract out this information, which is described here: https://fuchsia.googlesource.com/zircon/+/master/docs/syscalls/system_get_features.md The feature constants can be found here: https://fuchsia.googlesource.com/zircon/+/master/system/public/zircon/features.h The type `zx_status_t` and ZX_OK are defined here: https://fuchsia.googlesource.com/zircon/+/master/system/public/zircon/types.h#39 https://fuchsia.googlesource.com/zircon/+/master/system/public/zircon/errors.h#14 I agree to license my contributions to each file under the terms given at the top of each file I changed.

This is another attempt for ring to support fuchsia, by directly calling `zx_cprng_draw` to generate random byte strings. This avoids having to pull in an extra dependency (which briansmith#634 did). With this change, all the ring tests pass on fuchsia. I agree to license my contributions to each file under the terms given at the top of each file I changed. Closes briansmith#428

erickt · 2019-01-31T03:59:10Z

I just pushed up a fix for getting the build to work on x86, but I wasn't able to reproduce the issue you ran into with fargo test --release. Could you try updating your fuchsia checkout to see if there you got an unlucky commit? You can do it by doing:

% cd fuchsia
% jiri update
... # wait a bit for all the git pulls to land
% fx full-build
... # wait a while

Hopefully that'll get the tests to pass.

Also, if for some reason you want to use straight cargo, you can use fargo write-config, which will save all the config values into .cargo/config. Then in that directory you should be able to just run cargo build --tests and it should link in the artifacts from the ~/fuchsia/out/x64 build directory.

briansmith · 2019-02-01T22:13:48Z

The problem seems to be that fargo doesn't configure TARGET_CC or other things that are needed for Rust crates that use C code. For example, on Aarch64, I had to do this:

TARGET_CC="/home/m/f/fuchsia/buildtools/linux-x64/clang/bin/clang --sysroot=/home/m/f/fuchsia/out/build-zircon/build-arm64/sysroot" cargo test --target=aarch64-fuchsia --no-run

Otherwise, without manually setting TARGET_CC, we'd get errors like this during linking for AAarch64:

rust-lld: error: /home/m/ring/target/aarch64-fuchsia/debug/build/ring-4f3157642d57675c/out/libring-core.a(gcm.o) is incompatible with /home/m/f/fuchsia/out/arm64/sdk/exported/zircon_sysroot/arch/arm64/sysroot/lib/Scrt1.o

Similarly, previously I was getting linking errors for X86_64 builds, and __Fuchsia__ wasn't defined in C code, because I was using a Linux-targetting compiler and sysroot. Apparently Fuchsia is similarly enough to Linux that almost everything works fine even in that case.

briansmith · 2019-02-02T23:25:58Z

Those tests are failing because fargo doesn't know that it has to copy around artifacts like src/aead/aes_tests.txt. So to get around it, I just did:
% fx scp -r src "[`fx netaddr -fuchsia`]:/tmp/src"
% fx scp -r crypto "[`fx netaddr -fuchsia`]:/tmp/crypto"
% fx shell "cd tmp; ./ring-4af73f10c2056908_stripped"

running 99 tests
...
test result: ok. 99 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
Hurrah! It worked! But that wasn't super pleasant. Hopefully we can find some better way to do this.

As of ab0726d, this is no longer necessary. fargo test will work.

briansmith · 2019-02-03T00:18:28Z

When I build using fargo test and run the tests using your instructions (copying the test files and running the text executable through fx shell), chacha20_tests times out. Maybe this is just because the test requires a lot of CPU time and QEMU is slow.

Actually, the test never timed out. Instead the test driver reports "test aead::chacha::tests::chacha20_tests has been running for over 60 seconds" but if you wait a long time the test will complete successfully. This happens for other tests too sometimes. Simply, QEMU is much slower than a real machine. (32-bit ARM and 32-bit x86 are special cased in this test to avoid part of the test that isn't relevant for them.)

2. When I build using fargo test --release linking fails:

This was caused by me not setting TARGET_CC appropriately. Once it is set correctly it works fine.

erickt · 2019-02-04T21:47:36Z

Wonderful! Thanks @briansmith for helping me get this landed!

erickt force-pushed the fuchsia branch from b28fd5f to 18f844b Compare January 24, 2019 22:55

erickt force-pushed the fuchsia branch from 18f844b to 27054a6 Compare January 28, 2019 23:00

erickt force-pushed the fuchsia branch from 27054a6 to 462a875 Compare January 31, 2019 03:09

erickt mentioned this pull request Jan 31, 2019

Add support for fuchsia system feature detection on aarch64 #776

Closed

erickt added 2 commits January 30, 2019 19:46

erickt force-pushed the fuchsia branch from 462a875 to 184a5f2 Compare January 31, 2019 03:47

briansmith approved these changes Feb 1, 2019

View reviewed changes

briansmith merged commit e08b833 into briansmith:master Feb 1, 2019

erickt deleted the fuchsia branch February 4, 2019 21:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for fuchsia's CPRNG source #765

Add support for fuchsia's CPRNG source #765

erickt commented Jan 18, 2019

briansmith commented Jan 18, 2019

erickt commented Jan 18, 2019

erickt commented Jan 18, 2019 •

edited

Loading

erickt commented Jan 19, 2019

erickt commented Jan 24, 2019

coveralls commented Jan 25, 2019 •

edited

Loading

briansmith commented Jan 26, 2019

briansmith commented Jan 26, 2019

erickt commented Jan 26, 2019

erickt commented Jan 26, 2019

erickt commented Jan 28, 2019

briansmith commented Jan 28, 2019

erickt commented Jan 28, 2019

erickt commented Jan 29, 2019

erickt commented Jan 29, 2019

briansmith commented Jan 31, 2019 •

edited

Loading

briansmith commented Jan 31, 2019

erickt commented Jan 31, 2019

erickt commented Jan 31, 2019

briansmith commented Feb 1, 2019

briansmith commented Feb 2, 2019

briansmith commented Feb 3, 2019

erickt commented Feb 4, 2019

Add support for fuchsia's CPRNG source #765

Add support for fuchsia's CPRNG source #765

Conversation

erickt commented Jan 18, 2019

briansmith commented Jan 18, 2019

erickt commented Jan 18, 2019

erickt commented Jan 18, 2019 • edited Loading

erickt commented Jan 19, 2019

erickt commented Jan 24, 2019

coveralls commented Jan 25, 2019 • edited Loading

briansmith commented Jan 26, 2019

briansmith commented Jan 26, 2019

erickt commented Jan 26, 2019

erickt commented Jan 26, 2019

erickt commented Jan 28, 2019

briansmith commented Jan 28, 2019

erickt commented Jan 28, 2019

erickt commented Jan 29, 2019

erickt commented Jan 29, 2019

briansmith commented Jan 31, 2019 • edited Loading

briansmith commented Jan 31, 2019

erickt commented Jan 31, 2019

erickt commented Jan 31, 2019

briansmith commented Feb 1, 2019

briansmith commented Feb 2, 2019

briansmith commented Feb 3, 2019

erickt commented Feb 4, 2019

erickt commented Jan 18, 2019 •

edited

Loading

coveralls commented Jan 25, 2019 •

edited

Loading

briansmith commented Jan 31, 2019 •

edited

Loading