[Qemu-devel] [PATCH v6 00/25] Add qemu_getrandom and ARMv8.5-RNG etc

Richard Henderson posted 25 patches 4 years, 10 months ago
Test docker-mingw@fedora passed
Test docker-clang@ubuntu failed
Test checkpatch passed
Test asan failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190510173049.28171-1-richard.henderson@linaro.org
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <rth@twiddle.net>, Peter Maydell <peter.maydell@linaro.org>, Eduardo Habkost <ehabkost@redhat.com>, David Gibson <david@gibson.dropbear.id.au>, Joel Stanley <joel@jms.id.au>
There is a newer version of this series
Makefile                            |  14 ++--
Makefile.objs                       |   8 +--
Makefile.target                     |   4 --
include/crypto/random.h             |   2 +-
include/qemu/guest-random.h         |  68 ++++++++++++++++++
include/qom/cpu.h                   |   1 +
linux-user/aarch64/target_syscall.h |   2 -
target/arm/cpu.h                    |  17 +++--
target/i386/helper.h                |   2 +
cpus.c                              |   9 +++
crypto/random-gcrypt.c              |   2 +-
crypto/random-gnutls.c              |   2 +-
crypto/random-platform.c            | 104 +++++++++++++++++-----------
hw/misc/aspeed_scu.c                |  10 +--
hw/misc/bcm2835_rng.c               |  32 ++++-----
hw/misc/exynos4210_rng.c            |  11 ++-
hw/misc/nrf51_rng.c                 |   4 +-
linux-user/aarch64/cpu_loop.c       |  25 +------
linux-user/elfload.c                |   8 +--
linux-user/main.c                   |  34 +++++----
linux-user/syscall.c                |  34 +++++++--
target/arm/cpu64.c                  |   1 +
target/arm/helper.c                 |  64 ++++++++++++++---
target/arm/pauth_helper.c           |  18 ++---
target/i386/cpu.c                   |   5 +-
target/i386/int_helper.c            |  21 ++++++
target/i386/translate.c             |  62 +++++++++++++----
target/ppc/int_helper.c             |  39 +++++++----
target/ppc/translate.c              |  21 ++++--
ui/vnc.c                            |  53 ++++++--------
util/guest-random.c                 |  93 +++++++++++++++++++++++++
vl.c                                |   4 ++
configure                           |  87 +++++++++++++++--------
crypto/Makefile.objs                |  77 ++++++++++----------
qemu-options.hx                     |  10 +++
tests/Makefile.include              |   3 +-
util/Makefile.objs                  |   1 +
37 files changed, 644 insertions(+), 308 deletions(-)
create mode 100644 include/qemu/guest-random.h
create mode 100644 util/guest-random.c
[Qemu-devel] [PATCH v6 00/25] Add qemu_getrandom and ARMv8.5-RNG etc
Posted by Richard Henderson 4 years, 10 months ago
Only the new patch 24 lacks review.

Changes since v5:
  * Merge crypto-obj-y into util-obj-y (patch 2).
  * Fix leftover crypto-obj-aes-y reference (patch 2).
  * Add ARM_CP_IO to the RNG registers (patch 22).
  * Issue gen_io_start/end around ppc DARN (new patch 24).
  * Issue gen_io_start/end around x86 rdrand (patch 25).

Changes since v4:
  * Do not autoenable nettle or gcrypt if linking is broken.
    Fixes --static on fedora 30.
  * Delay removal of srand() for -seed.
  * Do not loop for -1 result for ppc64 DARN.

Changes since v3:
  * Do not autoenable gnutls if linking is broken.
    Fixes --static on ubuntu 18.04.

Changes since v2:
  * Changes from review.
    - getrandom is not exclusive of /dev/urandom fallback.
    - vnc fails gracefully on crypto failure.
    - a great renaming.
  * Drop the "nonblock" argument, as it's not deliverable from the backend.
  * Propagate Error back through qemu_guest_getrandom.
  * Add qemu_guest_getrandom_nofail to centralize "Argh! Death!".
  * Convert hw/misc/
  * Implement ppc darn.
  * Implement x86 rdrand.

Changes since v1:
  * Build crypto-obj-y for linux-user as well.
  * Several patches to tidy crypto/random-platform.c.
  * Use getrandom(2) in crypto/random-platform.c.
  * Use qcrypto_random_bytes in ui/vnc.c.
  * In qemu_getrandom:
    - Use g_rand_int instead of srand48.
    - Use qcrypto_random_bytes instead of getrandom directly.


r~


Richard Henderson (25):
  configure: Link test before auto-enabling crypto libraries
  crypto: Merge crypto-obj-y into libqemuutil.a
  crypto: Reverse code blocks in random-platform.c
  crypto: Do not fail for EINTR during qcrypto_random_bytes
  crypto: Use O_CLOEXEC in qcrypto_random_init
  crypto: Use getrandom for qcrypto_random_bytes
  crypto: Change the qcrypto_random_bytes buffer type to void*
  ui/vnc: Split out authentication_failed
  ui/vnc: Use gcrypto_random_bytes for start_auth_vnc
  util: Add qemu_guest_getrandom and associated routines
  cpus: Initialize pseudo-random seeds for all guest cpus
  linux-user: Initialize pseudo-random seeds for all guest cpus
  linux-user: Call qcrypto_init if not using -seed
  linux-user: Use qemu_guest_getrandom_nofail for AT_RANDOM
  linux-user/aarch64: Use qemu_guest_getrandom for PAUTH keys
  linux-user: Remove srand call
  aspeed/scu: Use qemu_guest_getrandom_nofail
  hw/misc/nrf51_rng: Use qemu_guest_getrandom_nofail
  hw/misc/bcm2835_rng: Use qemu_guest_getrandom_nofail
  hw/misc/exynos4210_rng: Use qemu_guest_getrandom
  target/arm: Put all PAC keys into a structure
  target/arm: Implement ARMv8.5-RNG
  target/ppc: Use qemu_guest_getrandom for DARN
  target/ppc: Use gen_io_start/end around DARN
  target/i386: Implement CPUID_EXT_RDRAND

 Makefile                            |  14 ++--
 Makefile.objs                       |   8 +--
 Makefile.target                     |   4 --
 include/crypto/random.h             |   2 +-
 include/qemu/guest-random.h         |  68 ++++++++++++++++++
 include/qom/cpu.h                   |   1 +
 linux-user/aarch64/target_syscall.h |   2 -
 target/arm/cpu.h                    |  17 +++--
 target/i386/helper.h                |   2 +
 cpus.c                              |   9 +++
 crypto/random-gcrypt.c              |   2 +-
 crypto/random-gnutls.c              |   2 +-
 crypto/random-platform.c            | 104 +++++++++++++++++-----------
 hw/misc/aspeed_scu.c                |  10 +--
 hw/misc/bcm2835_rng.c               |  32 ++++-----
 hw/misc/exynos4210_rng.c            |  11 ++-
 hw/misc/nrf51_rng.c                 |   4 +-
 linux-user/aarch64/cpu_loop.c       |  25 +------
 linux-user/elfload.c                |   8 +--
 linux-user/main.c                   |  34 +++++----
 linux-user/syscall.c                |  34 +++++++--
 target/arm/cpu64.c                  |   1 +
 target/arm/helper.c                 |  64 ++++++++++++++---
 target/arm/pauth_helper.c           |  18 ++---
 target/i386/cpu.c                   |   5 +-
 target/i386/int_helper.c            |  21 ++++++
 target/i386/translate.c             |  62 +++++++++++++----
 target/ppc/int_helper.c             |  39 +++++++----
 target/ppc/translate.c              |  21 ++++--
 ui/vnc.c                            |  53 ++++++--------
 util/guest-random.c                 |  93 +++++++++++++++++++++++++
 vl.c                                |   4 ++
 configure                           |  87 +++++++++++++++--------
 crypto/Makefile.objs                |  77 ++++++++++----------
 qemu-options.hx                     |  10 +++
 tests/Makefile.include              |   3 +-
 util/Makefile.objs                  |   1 +
 37 files changed, 644 insertions(+), 308 deletions(-)
 create mode 100644 include/qemu/guest-random.h
 create mode 100644 util/guest-random.c

-- 
2.17.1


Re: [Qemu-devel] [PATCH v6 00/25] Add qemu_getrandom and ARMv8.5-RNG etc
Posted by Markus Armbruster 4 years, 10 months ago
"make check-unit" fails for me:

  TEST    check-unit: tests/test-crypto-tlscredsx509
Unexpected error in object_new_with_propv() at /work/armbru/qemu/qom/object.c:674:
invalid object type: tls-creds-x509

and

  TEST    check-unit: tests/test-io-channel-tls
Unexpected error in object_new_with_propv() at /work/armbru/qemu/qom/object.c:674:
invalid object type: tls-creds-x509

I haven't looked further.

Re: [Qemu-devel] [PATCH v6 00/25] Add qemu_getrandom and ARMv8.5-RNG etc
Posted by Daniel P. Berrangé 4 years, 10 months ago
On Tue, May 14, 2019 at 05:15:31PM +0200, Markus Armbruster wrote:
> "make check-unit" fails for me:
> 
>   TEST    check-unit: tests/test-crypto-tlscredsx509
> Unexpected error in object_new_with_propv() at /work/armbru/qemu/qom/object.c:674:
> invalid object type: tls-creds-x509
> 
> and
> 
>   TEST    check-unit: tests/test-io-channel-tls
> Unexpected error in object_new_with_propv() at /work/armbru/qemu/qom/object.c:674:
> invalid object type: tls-creds-x509
> 
> I haven't looked further.

I have a nasty feeling it is caused by

  Subject: [PATCH v6 02/25] crypto: Merge crypto-obj-y into libqemuutil.a

The QOM objects are not directly used by most of the code. We rely on
the constructor registering the QOM object and then we request an
instance of it via the type name. So there's no direct function calls
from any code into the crypto object impls.

When we put the crypto objects into libqemuutil.a the linker is not
intelligent enough to see the constructor and so thinks all these
QOM object impls are unused and discards them when linking the final
binary.

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 :|

Re: [Qemu-devel] [PATCH v6 00/25] Add qemu_getrandom and ARMv8.5-RNG etc
Posted by Richard Henderson 4 years, 10 months ago
On 5/14/19 8:23 AM, Daniel P. Berrangé wrote:
> On Tue, May 14, 2019 at 05:15:31PM +0200, Markus Armbruster wrote:
>> "make check-unit" fails for me:
>>
>>   TEST    check-unit: tests/test-crypto-tlscredsx509
>> Unexpected error in object_new_with_propv() at /work/armbru/qemu/qom/object.c:674:
>> invalid object type: tls-creds-x509
>>
>> and
>>
>>   TEST    check-unit: tests/test-io-channel-tls
>> Unexpected error in object_new_with_propv() at /work/armbru/qemu/qom/object.c:674:
>> invalid object type: tls-creds-x509
>>
>> I haven't looked further.
> 
> I have a nasty feeling it is caused by
> 
>   Subject: [PATCH v6 02/25] crypto: Merge crypto-obj-y into libqemuutil.a
> 
> The QOM objects are not directly used by most of the code. We rely on
> the constructor registering the QOM object and then we request an
> instance of it via the type name. So there's no direct function calls
> from any code into the crypto object impls.
> 
> When we put the crypto objects into libqemuutil.a the linker is not
> intelligent enough to see the constructor and so thinks all these
> QOM object impls are unused and discards them when linking the final
> binary.

Yes, that would do it.  We would need something in the test that forces the
objects into the link.  Without having yet looked at the test cases, any ideas?


r~


Re: [Qemu-devel] [PATCH v6 00/25] Add qemu_getrandom and ARMv8.5-RNG etc
Posted by Daniel P. Berrangé 4 years, 10 months ago
On Tue, May 14, 2019 at 09:14:57AM -0700, Richard Henderson wrote:
> On 5/14/19 8:23 AM, Daniel P. Berrangé wrote:
> > On Tue, May 14, 2019 at 05:15:31PM +0200, Markus Armbruster wrote:
> >> "make check-unit" fails for me:
> >>
> >>   TEST    check-unit: tests/test-crypto-tlscredsx509
> >> Unexpected error in object_new_with_propv() at /work/armbru/qemu/qom/object.c:674:
> >> invalid object type: tls-creds-x509
> >>
> >> and
> >>
> >>   TEST    check-unit: tests/test-io-channel-tls
> >> Unexpected error in object_new_with_propv() at /work/armbru/qemu/qom/object.c:674:
> >> invalid object type: tls-creds-x509
> >>
> >> I haven't looked further.
> > 
> > I have a nasty feeling it is caused by
> > 
> >   Subject: [PATCH v6 02/25] crypto: Merge crypto-obj-y into libqemuutil.a
> > 
> > The QOM objects are not directly used by most of the code. We rely on
> > the constructor registering the QOM object and then we request an
> > instance of it via the type name. So there's no direct function calls
> > from any code into the crypto object impls.
> > 
> > When we put the crypto objects into libqemuutil.a the linker is not
> > intelligent enough to see the constructor and so thinks all these
> > QOM object impls are unused and discards them when linking the final
> > binary.
> 
> Yes, that would do it.  We would need something in the test that forces the
> objects into the link.  Without having yet looked at the test cases, any ideas?

I don't think this is only the test suite. I think it will affect all the
binaries we build

The only way you can force it is to use -Wl,--whole-archive arg to tell ld
to include everything from libqemuutil.la, but that will break the way
the stubs work, as we want make of the stubs to be discarded.

The only other option is to not build $(crypto-obj-y) into libqemuutil.la,
but list that variable explicitly everywhere that we list libqemuutil.la

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 :|

Re: [Qemu-devel] [PATCH v6 00/25] Add qemu_getrandom and ARMv8.5-RNG etc
Posted by Richard Henderson 4 years, 10 months ago
On 5/14/19 9:50 AM, Daniel P. Berrangé wrote:
> On Tue, May 14, 2019 at 09:14:57AM -0700, Richard Henderson wrote:
>> Yes, that would do it.  We would need something in the test that forces the
>> objects into the link.  Without having yet looked at the test cases, any ideas?
> 
> I don't think this is only the test suite. I think it will affect all the
> binaries we build

You're right, it does.

$ nm aarch64-softmmu/qemu-system-aarch64  \
  | grep qcrypto_tls_creds_x509_register_types

comes up empty.

It didn't occur to me that there was nothing in the object files for the
reference.  I'll have to drop the crypto-obj-y patch and come up with a
different solution.


r~

Re: [Qemu-devel] [PATCH v6 00/25] Add qemu_getrandom and ARMv8.5-RNG etc
Posted by Eric Blake 4 years, 10 months ago
On 5/14/19 12:46 PM, Richard Henderson wrote:
> On 5/14/19 9:50 AM, Daniel P. Berrangé wrote:
>> On Tue, May 14, 2019 at 09:14:57AM -0700, Richard Henderson wrote:
>>> Yes, that would do it.  We would need something in the test that forces the
>>> objects into the link.  Without having yet looked at the test cases, any ideas?
>>
>> I don't think this is only the test suite. I think it will affect all the
>> binaries we build
> 
> You're right, it does.
> 
> $ nm aarch64-softmmu/qemu-system-aarch64  \
>   | grep qcrypto_tls_creds_x509_register_types
> 
> comes up empty.
> 
> It didn't occur to me that there was nothing in the object files for the
> reference.  I'll have to drop the crypto-obj-y patch and come up with a
> different solution.

Isn't there a gcc annotation for marking a simple as mandatorily
included during link?

/me goes looking...

__attribute__((externally_visible)) sounds promising (it nullifies the
effects of -fwhole-program, so that a function remains visible even if
the linker would have otherwise suppressed it)

__attribute__((used)) also sounds useful (the function must be emitted
even if it does not appear to be referenced, which may be enough for the
linker to infer that it is used)

There may be other tricks, although I didn't go searching very hard.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v6 00/25] Add qemu_getrandom and ARMv8.5-RNG etc
Posted by Richard Henderson 4 years, 10 months ago
On 5/14/19 2:43 PM, Eric Blake wrote:
>> It didn't occur to me that there was nothing in the object files for the
>> reference.  I'll have to drop the crypto-obj-y patch and come up with a
>> different solution.
> 
> Isn't there a gcc annotation for marking a simple as mandatorily
> included during link?

No.

There's stuff you can mark a single function within an object file that you can
use to avoid the function being elided...

> __attribute__((externally_visible)) sounds promising (it nullifies the
> effects of -fwhole-program, so that a function remains visible even if
> the linker would have otherwise suppressed it)
> 
> __attribute__((used)) also sounds useful (the function must be emitted
> even if it does not appear to be referenced, which may be enough for the
> linker to infer that it is used)

... and you found those.  But those do not affect the linker's behaviour with
.a files at all.

You can force a symbol reference from the ld command-line: -u sym, which can
cause the .o containing sym to be included from the .a file.  But that doesn't
work if there's no global symbol in the .o to reference.

You can force all .o from a .a file to be included, with --whole-archive.  That
is useful when you're using .a files a shorthand for lots and lots of .o files.
 But in our case that would break the use of stubs.

Anyway, see v7 now.


r~