Re: [PATCH v3 1/3] hw/misc: Introduce AMD/Xilix Versal TRNG device

[Peter Maydell]

On Wed, 4 Oct 2023 at 15:27, Tong Ho <tong.ho@amd.com> wrote:
>
> This adds a non-cryptographic grade implementation of the
> model for the True Random Number Generator (TRNG) component
> in AMD/Xilinx Versal device family.
>
> This implements all 3 modes defined by the actual hardware
> specs, all of which selectable by guest software at will
> at anytime:
> 1) PRNG mode, in which the generated sequence is required to
>    be reproducible after reseeded by the same 384-bit value
>    as supplied by guest software.
> 2) Test mode, in which the generated sequence is required to
>    be reproducible ater reseeded by the same 128-bit test
>    seed supplied by guest software.
> 3) TRNG mode, in which non-reproducible sequence is generated
>    based on periodic reseed by a suitable entropy source.
>
> This model is only intended for non-real world testing of
> guest software, where cryptographically strong PRNG and TRNG
> is not needed.
>
> This model supports versions 1 & 2 of the device, with
> default to be version 2; the 'hw-version' uint32 property
> can be set to 0x0100 to override the default.
>
> Other implemented properties:
> - 'forced-prng', uint64
>   When set to non-zero, mode 3's entropy source is implemented
>   as a deterministic sequence based on the given value and other
>   deterministic parameters.
>   This option allows the emulation to test guest software using
>   mode 3 and to reproduce data-dependent defects.
>
> - 'fips-fault-events', uint32, bit-mask
>   bit 3: Triggers the SP800-90B entropy health test fault irq
>   bit 1: Triggers the FIPS 140-2 continuous test fault irq
>
> Signed-off-by: Tong Ho <tong.ho@amd.com>
> ---

> +    enum {
> +        U384_U32 = 384 / 32,
> +    };
> +
> +    /*
> +     * Maximum seed length is len(personalized string) + len(ext seed).
> +     *
> +     * Use little-endian to ensure guest sequence being indepedent of
> +     * host endian.
> +     */
> +    struct {
> +        guint32 ps[U384_U32];
> +        union {
> +            uint64_t int_seed[2];
> +            guint32 ext_seed[U384_U32];
> +        };
> +    } gs;

This struct-and-union seems unnecessarily complicated.
It also means you will be seeding the RNG with different
values on little and big endian, because the two halves
of the uint64_t values will be the opposite way around
when g_rand_set_seed_array() views them as a set of guint32s.
Probably better to use a simple array of guint32.

> +    /*
> +     * A disabled personalized string is the same as
> +     * a string with all zeros.
> +     *
> +     * The device's hardware spec defines 3 modes (all selectable
> +     * by guest at will and at anytime):
> +     * 1) External seeding
> +     *    This is a PRNG mode, in which the produced sequence shall
> +     *    be reproducible if reseeded by the same 384-bit seed, as
> +     *    supplied by guest software.
> +     * 2) Test seeding
> +     *    This is a PRNG mode, in which the produced sequence shall
> +     *    be reproducible if reseeded by a 128-bit test seed, as
> +     *    supplied by guest software.
> +     * 3) Truly-random seeding
> +     *    This is the TRNG mode, in which the produced sequence is
> +     *    periodically reseeded by a crypto-strength entropy source.
> +     *
> +     * To assist debugging of certain classes of software defects,
> +     * this QEMU model implements a 4th mode,
> +     * 4) Forced PRNG
> +     *    When in this mode, a reproducible sequence is generated
> +     *    if software has selected the TRNG mode (mode 2).
> +     *
> +     *    This emulation-only mode can only be selected by setting
> +     *    the uint64 property 'forced-prng' to a non-zero value.
> +     *    Guest software cannot select this mode.
> +     */
> +    memset(&gs, 0, sizeof(gs));
> +    gs.ext_seed[ARRAY_SIZE(gs.ext_seed) - 1] = cpu_to_be32(1);
> +
> +    if (!pers_disabled) {
> +        trng_le384(gs.ps, &s->regs[R_PER_STRNG_0]);
> +    }
> +
> +    if (ext_seed) {
> +        trng_le384(gs.ext_seed, &s->regs[R_EXT_SEED_0]);
> +    } else if (trng_test_enabled(s)) {
> +        gs.int_seed[0] = cpu_to_le64(s->tst_seed[0]);
> +        gs.int_seed[1] = cpu_to_le64(s->tst_seed[1]);
> +    } else if (s->forced_prng_seed) {
> +        s->forced_prng_count++;
> +        gs.int_seed[0] = cpu_to_le64(s->forced_prng_count);
> +        gs.int_seed[1] = cpu_to_le64(s->forced_prng_seed);
> +    } else {
> +        gs.int_seed[0] = cpu_to_le64(qemu_clock_get_ns(QEMU_CLOCK_HOST));
> +        gs.int_seed[1] = cpu_to_le64(getpid());

This is not "a crypto strength entropy source". It's also going
to cause problems for record-and-replay, because the value will
not be the same on replay as it was on record.

If you want 128 (or 384) bits of random data, call
qemu_guest_getrandom_nofail().

> +    }
> +
> +    g_rand_set_seed_array(s->prng, gs.ps,
> +                          sizeof(gs) / sizeof(guint32));

> +
> +    s->rand_count = 0;
> +    s->rand_reseed = 1ULL << 48;
> +}
> +
> +static void trng_regen(XlnxVersalTRng *s)
> +{
> +    if (s->rand_reseed == 0) {
> +        TRNG_GUEST_ERROR(s, "Too many generations without a reseed");
> +        trng_reseed(s);
> +    }
> +    s->rand_reseed--;
> +
> +    /*
> +     * In real hardware, each regen creates 256 bits, but QCNT
> +     * reports a max of 4.
> +     */
> +    ARRAY_FIELD_DP32(s->regs, STATUS, QCNT, 4);
> +    s->rand_count = 256 / 32;
> +}
> +
> +static uint32_t trng_rdout(XlnxVersalTRng *s)
> +{
> +    assert(s->rand_count);
> +
> +    s->rand_count--;
> +    if (s->rand_count < 4) {
> +        ARRAY_FIELD_DP32(s->regs, STATUS, QCNT, s->rand_count);
> +    }
> +
> +    /* Reject all 0's and all 1's */

This is weird. Why do we need to do this ?

> +    while (true) {
> +        /* g_rand_int_range() returns gint32, not guint32 */
> +        guint32 nr = g_rand_int(s->prng);
> +
> +        if (nr && (nr != ~0)) {
> +            return nr;
> +        }
> +    }
> +}
> +

thanks
-- PMM