| Message ID | 20190313062630.30568-1-richard.henderson@linaro.org |
|---|---|
| Headers | show |
| Series | Add qemu_getrandom and ARMv8.5-RNG | expand |
Patchew URL: https://patchew.org/QEMU/20190313062630.30568-1-richard.henderson@linaro.org/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20190313062630.30568-1-richard.henderson@linaro.org Subject: [Qemu-devel] [PATCH for-4.1 0/7] Add qemu_getrandom and ARMv8.5-RNG === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu t [tag update] patchew/20190312091031.5185-1-eric.auger@redhat.com -> patchew/20190312091031.5185-1-eric.auger@redhat.com t [tag update] patchew/20190312224923.25709-1-richard.henderson@linaro.org -> patchew/20190312224923.25709-1-richard.henderson@linaro.org * [new tag] patchew/20190313062630.30568-1-richard.henderson@linaro.org -> patchew/20190313062630.30568-1-richard.henderson@linaro.org Switched to a new branch 'test' f605897f87 target/arm: Implement ARMv8.5-RNG bd69ce4cd9 ui/vnc: Use qemu_getrandom for make_challenge c1324306af linux-user: Remove srand call cdca1c1a38 linux-user/aarch64: Use qemu_getrandom for arm_init_pauth_key 95ea39c668 linux-user: Use qemu_getrandom for AT_RANDOM f7e7a71b23 util: Use getrandom for qemu_getrandom if available 11d9c16454 util: Add qemu_getrandom and support functions === OUTPUT BEGIN === 1/7 Checking commit 11d9c16454fc (util: Add qemu_getrandom and support functions) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #90: new file mode 100644 ERROR: Use g_assert or g_assert_not_reached #290: FILE: util/random.c:33: + g_assert_cmpuint(len, <=, 256); total: 1 errors, 1 warnings, 284 lines checked Patch 1/7 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 2/7 Checking commit f7e7a71b2340 (util: Use getrandom for qemu_getrandom if available) ERROR: Use g_assert or g_assert_not_reached #115: FILE: util/random.c:81: + g_assert_cmpuint(len, <=, 256); ERROR: Use g_assert or g_assert_not_reached #171: FILE: util/random.c:135: + g_assert_cmpint(errno, ==, ENOSYS); total: 2 errors, 0 warnings, 149 lines checked Patch 2/7 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 3/7 Checking commit 95ea39c66897 (linux-user: Use qemu_getrandom for AT_RANDOM) 4/7 Checking commit cdca1c1a385b (linux-user/aarch64: Use qemu_getrandom for arm_init_pauth_key) 5/7 Checking commit c1324306af95 (linux-user: Remove srand call) 6/7 Checking commit bd69ce4cd97a (ui/vnc: Use qemu_getrandom for make_challenge) 7/7 Checking commit f605897f877f (target/arm: Implement ARMv8.5-RNG) === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20190313062630.30568-1-richard.henderson@linaro.org/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
Patchew URL: https://patchew.org/QEMU/20190313062630.30568-1-richard.henderson@linaro.org/
Hi,
This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===
CC hw/acpi/trace.o
CC hw/alpha/trace.o
/tmp/qemu-test/src/util/random.c: In function 'do_jrand48':
/tmp/qemu-test/src/util/random.c:41:15: error: implicit declaration of function 'jrand48'; did you mean 'do_jrand48'? [-Werror=implicit-function-declaration]
val = jrand48(xsubi);
^~~~~~~
do_jrand48
/tmp/qemu-test/src/util/random.c:41:15: error: nested extern declaration of 'jrand48' [-Werror=nested-externs]
/tmp/qemu-test/src/util/random.c: In function 'initialize':
/tmp/qemu-test/src/util/random.c:127:5: error: implicit declaration of function 'srand48'; did you mean 'srand'? [-Werror=implicit-function-declaration]
srand48(0);
^~~~~~~
srand
/tmp/qemu-test/src/util/random.c:127:5: error: nested extern declaration of 'srand48' [-Werror=nested-externs]
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: util/random.o] Error 1
make: *** Waiting for unfinished jobs....
The full log is available at
http://patchew.org/logs/20190313062630.30568-1-richard.henderson@linaro.org/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On 3/12/19 11:41 PM, no-reply@patchew.org wrote: > time make docker-test-mingw@fedora SHOW_ENV=1 J=14 NETWORK=1 > === TEST SCRIPT END === > > CC hw/acpi/trace.o > CC hw/alpha/trace.o > /tmp/qemu-test/src/util/random.c: In function 'do_jrand48': > /tmp/qemu-test/src/util/random.c:41:15: error: implicit declaration of function 'jrand48'; did you mean 'do_jrand48'? [-Werror=implicit-function-declaration] > val = jrand48(xsubi); Hmm, no jrand48 in mingw. I assumed it would be there because of POSIX. Perhaps I should just bite the bullet and inline my own DRNG... r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 3/12/19 11:41 PM, no-reply@patchew.org wrote: >> time make docker-test-mingw@fedora SHOW_ENV=1 J=14 NETWORK=1 >> === TEST SCRIPT END === >> >> CC hw/acpi/trace.o >> CC hw/alpha/trace.o >> /tmp/qemu-test/src/util/random.c: In function 'do_jrand48': >> /tmp/qemu-test/src/util/random.c:41:15: error: implicit declaration of function 'jrand48'; did you mean 'do_jrand48'? [-Werror=implicit-function-declaration] >> val = jrand48(xsubi); > > Hmm, no jrand48 in mingw. I assumed it would be there because of POSIX. > Perhaps I should just bite the bullet and inline my own DRNG... Have you considered GLib's? Mersenne Twister under the hood. https://developer.gnome.org/glib/stable/glib-Random-Numbers.html
On Wed, Mar 13, 2019 at 08:25:45AM -0700, Richard Henderson wrote: > On 3/12/19 11:41 PM, no-reply@patchew.org wrote: > > time make docker-test-mingw@fedora SHOW_ENV=1 J=14 NETWORK=1 > > === TEST SCRIPT END === > > > > CC hw/acpi/trace.o > > CC hw/alpha/trace.o > > /tmp/qemu-test/src/util/random.c: In function 'do_jrand48': > > /tmp/qemu-test/src/util/random.c:41:15: error: implicit declaration of function 'jrand48'; did you mean 'do_jrand48'? [-Werror=implicit-function-declaration] > > val = jrand48(xsubi); > > Hmm, no jrand48 in mingw. I assumed it would be there because of POSIX. > Perhaps I should just bite the bullet and inline my own DRNG... We already have an internal API for providing strong random bytes in QEMU qcrypto_random_bytes. It is preferentially backed by gnutls or gcrypt, but if those aren't built-in it falls back to a platform native API like /dev/random. I've got a todo item to make that use getrandom on Linux/BSD when available. I don't think we should be adding a new APIs for getting random numbers that aren't backed by the qcrypto_random_bytes. By all means add wrappers around qcrypto_random_bytes to make it more convenient to get random numbers with a given data types (int64, int32 etc) though. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 3/13/19 10:57 AM, Daniel P. Berrangé wrote: > We already have an internal API for providing strong random bytes in > QEMU qcrypto_random_bytes. It is preferentially backed by gnutls or > gcrypt, but if those aren't built-in it falls back to a platform > native API like /dev/random. I've got a todo item to make that use > getrandom on Linux/BSD when available. > > I don't think we should be adding a new APIs for getting random > numbers that aren't backed by the qcrypto_random_bytes. That's all very well and good for one particular use case, when we really want random numbers. But with -seed, we want to be able to replicate one particular execution, with fully deterministic numbers. But certainly I can look into making the non-deterministic execution use your existing interface. I simply wasn't aware of it. Do you happen to know off-hand the maximum latency of the gnutls/gcrypt interfaces? I do want the interface to be able to return "timed-out" in certain cases. With getrandom(2) there is a handy GRND_NONBLOCK parameter that pretty much matches exactly what I want. With /dev/{u,}random, one would have to use O_NONBLOCK. With BSD getentropy, I think you'd need an alarm to time out the operation (unless EIO covers all sorts of failures like empty entropy pool; the manual isn't clear)? r~
On 3/13/19 10:38 AM, Markus Armbruster wrote: > Have you considered GLib's? Mersenne Twister under the hood. > > https://developer.gnome.org/glib/stable/glib-Random-Numbers.html I hadn't considered glib's, but I am considering MT19337-64. r~
On 3/13/19 11:49 AM, Richard Henderson wrote: > On 3/13/19 10:38 AM, Markus Armbruster wrote: >> Have you considered GLib's? Mersenne Twister under the hood. >> >> https://developer.gnome.org/glib/stable/glib-Random-Numbers.html > > I hadn't considered glib's, but I am considering MT19337-64. Thanks. glib's version is just as convenient as anything else. I'll use that for the deterministic path. r~
On Wed, Mar 13, 2019 at 11:35:33AM -0700, Richard Henderson wrote: > On 3/13/19 10:57 AM, Daniel P. Berrangé wrote: > > We already have an internal API for providing strong random bytes in > > QEMU qcrypto_random_bytes. It is preferentially backed by gnutls or > > gcrypt, but if those aren't built-in it falls back to a platform > > native API like /dev/random. I've got a todo item to make that use > > getrandom on Linux/BSD when available. > > > > I don't think we should be adding a new APIs for getting random > > numbers that aren't backed by the qcrypto_random_bytes. > > That's all very well and good for one particular use case, when we really want > random numbers. But with -seed, we want to be able to replicate one particular > execution, with fully deterministic numbers. > > But certainly I can look into making the non-deterministic execution use your > existing interface. I simply wasn't aware of it. The random interfact is pluggable with different impls though they are chosen at build time currently. I guess we could provide an impl that is backed by a deterministic source and require a special CLI option or env var to switch to this insecure mode at runtime. > Do you happen to know off-hand the maximum latency of the gnutls/gcrypt > interfaces? I do want the interface to be able to return "timed-out" in > certain cases. With getrandom(2) there is a handy GRND_NONBLOCK parameter that > pretty much matches exactly what I want. With /dev/{u,}random, one would have > to use O_NONBLOCK. With BSD getentropy, I think you'd need an alarm to time > out the operation (unless EIO covers all sorts of failures like empty entropy > pool; the manual isn't clear)? I don't think there's any documented latency rules for the library interfaces. Most of the time I think they should have deterministic execution time as they'll just be running a cryptographic hash algorithm of some kind, but they might periodically re-seed entropy from the host OS. They're really supposed to be considered a black box from the application user POV. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 3/14/19 2:54 AM, Daniel P. Berrangé wrote: > The random interfact is pluggable with different impls though they are > chosen at build time currently. I guess we could provide an impl that > is backed by a deterministic source and require a special CLI option or > env var to switch to this insecure mode at runtime. I would think that some of the existing users of the crypto backend would not need an insecure switch, as they're not specifically visible to the guest. E.g. crypto/block-luks.c. (Although, see below.) What I have done, and better in the now-posted v2, is copy exactly this CLI option from linux-user/main.c to system mode as well. Then, use that in only the places that are guest visible -- and currently use rand(3). So, within the instances I change, I am in all cases producing better quality random numbers than we do currently. In the case of -seed, using a Mersenne Twister algorithm instead of the rather tiny linear congruence algorithm of rand(3). In the case of no -seed, by using a cryptographic quality source. The final patch is the new ARM built-in hardware random number generation instruction. The spec for the insn requires crypto quality results (complying to NIST...). But, as previously stated, as a debugging tool we need to be able to force reproduciblity. Re-checking, I now see that the uses within hw/misc/ are in fact guest visible emulations of hardware random number generators, and so should probably be switched to use my new interface, just as with the ARM instruction. If you have a better name for "qemu_getrandom" that emphasizes this usage, I'm happy to change that. r~
On Thu, Mar 14, 2019 at 09:06:42AM -0700, Richard Henderson wrote: > On 3/14/19 2:54 AM, Daniel P. Berrangé wrote: > > The random interfact is pluggable with different impls though they are > > chosen at build time currently. I guess we could provide an impl that > > is backed by a deterministic source and require a special CLI option or > > env var to switch to this insecure mode at runtime. > > I would think that some of the existing users of the crypto backend would not > need an insecure switch, as they're not specifically visible to the guest. > E.g. crypto/block-luks.c. (Although, see below.) > > What I have done, and better in the now-posted v2, is copy exactly this CLI > option from linux-user/main.c to system mode as well. Then, use that in only > the places that are guest visible -- and currently use rand(3). > > So, within the instances I change, I am in all cases producing better quality > random numbers than we do currently. In the case of -seed, using a Mersenne > Twister algorithm instead of the rather tiny linear congruence algorithm of > rand(3). In the case of no -seed, by using a cryptographic quality source. > > The final patch is the new ARM built-in hardware random number generation > instruction. The spec for the insn requires crypto quality results (complying > to NIST...). But, as previously stated, as a debugging tool we need to be able > to force reproduciblity. Yes, I understand now. > Re-checking, I now see that the uses within hw/misc/ are in fact guest visible > emulations of hardware random number generators, and so should probably be > switched to use my new interface, just as with the ARM instruction. Yeah, that probably makes sense. > > If you have a better name for "qemu_getrandom" that emphasizes this usage, I'm > happy to change that. Yep, back in your v2 thread I had suggested qemu_cpu_getrandom, but with your note about hw/misc, it looks a bit more general than that. Essentially the debugging reproducability is useful for anything that is guest facing frontend. Where we don't want it used is in any of the backends. So far best alternative is 'qemu_traceable_random' / tracable-random.h but its a bit of a verbose name. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 3/14/19 9:15 AM, Daniel P. Berrangé wrote: > So far best alternative is 'qemu_traceable_random' / tracable-random.h > but its a bit of a verbose name. How about qemu_guest_getrandom / guest-random.h? r~
On Thu, Mar 14, 2019 at 09:23:12AM -0700, Richard Henderson wrote: > On 3/14/19 9:15 AM, Daniel P. Berrangé wrote: > > So far best alternative is 'qemu_traceable_random' / tracable-random.h > > but its a bit of a verbose name. > > How about qemu_guest_getrandom / guest-random.h? Yeah, that's ok with me. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
While the comment for AT_RANDOM is still apropos, "not cryptically secure but it's not the aim of QEMU", I think we can still do better than N calls to rand(3). The first patch sets up an interface that allows deterministic random numbers across different threads, using jrand48. This function is: (1) in POSIX, so is easy to assume, (2) produces full 32-bit random numbers, as opposed to RAND_MAX, making it easier to fill N bytes, (3) has a much larger periodicity, (4) is thread-safe (with restricted usage). The second patch allows the use of getrandom(2), if available. But if the -seed command-line option is used, we continue to use the deterministic algorithm. I leave the task of adding support for Windows BCryptGenRandom, and BSD getentropy, to someone else. I didn't think it was worth it to do anything with /dev/urandom, in case getrandom isn't present. I replaced the existing major users of rand(3). There are a few left, mostly within hw/. I'm really not sure whether it's worth changing those, or what to do about them. This could quickly be used to implement Power9's helper_darn{32,64}, or for implementing RDRAND for x86_64. I'm less sure about S390's PRNO instruction; that seems to expose a lot of the DRNG at an architectural level. r~ Richard Henderson (7): util: Add qemu_getrandom and support functions util: Use getrandom for qemu_getrandom if available linux-user: Use qemu_getrandom for AT_RANDOM linux-user/aarch64: Use qemu_getrandom for arm_init_pauth_key linux-user: Remove srand call ui/vnc: Use qemu_getrandom for make_challenge target/arm: Implement ARMv8.5-RNG include/qemu/random.h | 58 ++++++++++++++ include/qom/cpu.h | 1 + target/arm/cpu.h | 5 ++ cpus.c | 9 +++ linux-user/aarch64/cpu_loop.c | 16 +--- linux-user/elfload.c | 8 +- linux-user/main.c | 11 +-- linux-user/syscall.c | 3 + target/arm/cpu64.c | 1 + target/arm/helper.c | 32 ++++++++ ui/vnc.c | 8 +- util/random.c | 140 ++++++++++++++++++++++++++++++++++ vl.c | 4 + configure | 18 ++++- qemu-options.hx | 10 +++ util/Makefile.objs | 1 + 16 files changed, 290 insertions(+), 35 deletions(-) create mode 100644 include/qemu/random.h create mode 100644 util/random.c -- 2.17.1