[Qemu-devel] [PATCH v3] aspeed_scu: Implement RNG register

[Qemu-devel] [PATCH v3] aspeed_scu: Implement RNG register

[Joel Stanley]

The ASPEED SoCs contain a single register that returns random data when
read. This models that register so that guests can use it.

The random number data register has a corresponding control register,
however it returns data regardless of the state of the enabled bit, so
the model follows this behaviour.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
v2:
 - Remove call to qcrypto_random_init as this is done in main()
v3:
 - Add Cédric's reviewed-by
 - Add a comment about why we don't check for the rng enable bit
---
 hw/misc/aspeed_scu.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index 5e6d5744eeca..96db052389cc 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -16,6 +16,7 @@
 #include "qapi/visitor.h"
 #include "qemu/bitops.h"
 #include "qemu/log.h"
+#include "crypto/random.h"
 #include "trace.h"
 
 #define TO_REG(offset) ((offset) >> 2)
@@ -154,6 +155,18 @@ static const uint32_t ast2500_a1_resets[ASPEED_SCU_NR_REGS] = {
      [BMC_DEV_ID]      = 0x00002402U
 };
 
+static uint32_t aspeed_scu_get_random(void)
+{
+    Error *err = NULL;
+    uint32_t num;
+
+    if (qcrypto_random_bytes((uint8_t *)&num, sizeof(num), &err)) {
+        error_report_err(err);
+    }
+
+    return num;
+}
+
 static uint64_t aspeed_scu_read(void *opaque, hwaddr offset, unsigned size)
 {
     AspeedSCUState *s = ASPEED_SCU(opaque);
@@ -167,6 +180,12 @@ static uint64_t aspeed_scu_read(void *opaque, hwaddr offset, unsigned size)
     }
 
     switch (reg) {
+    case RNG_DATA:
+        /* On hardware, RNG_DATA works regardless of
+         * the state of the enable bit in RNG_CTRL
+         */
+        s->regs[RNG_DATA] = aspeed_scu_get_random();
+        break;
     case WAKEUP_EN:
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: Read of write-only offset 0x%" HWADDR_PRIx "\n",
-- 
2.17.0

Re: [Qemu-devel] [Qemu-arm] [PATCH v3] aspeed_scu: Implement RNG register

[Philippe Mathieu-Daudé]

On 05/29/2018 01:19 AM, Joel Stanley wrote:
> The ASPEED SoCs contain a single register that returns random data when
> read. This models that register so that guests can use it.
> 
> The random number data register has a corresponding control register,
> however it returns data regardless of the state of the enabled bit, so
> the model follows this behaviour.
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Joel Stanley <joel@jms.id.au>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
> v2:
>  - Remove call to qcrypto_random_init as this is done in main()
> v3:
>  - Add Cédric's reviewed-by
>  - Add a comment about why we don't check for the rng enable bit
> ---
>  hw/misc/aspeed_scu.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index 5e6d5744eeca..96db052389cc 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -16,6 +16,7 @@
>  #include "qapi/visitor.h"
>  #include "qemu/bitops.h"
>  #include "qemu/log.h"
> +#include "crypto/random.h"
>  #include "trace.h"
>  
>  #define TO_REG(offset) ((offset) >> 2)
> @@ -154,6 +155,18 @@ static const uint32_t ast2500_a1_resets[ASPEED_SCU_NR_REGS] = {
>       [BMC_DEV_ID]      = 0x00002402U
>  };
>  
> +static uint32_t aspeed_scu_get_random(void)
> +{
> +    Error *err = NULL;
> +    uint32_t num;
> +
> +    if (qcrypto_random_bytes((uint8_t *)&num, sizeof(num), &err)) {
> +        error_report_err(err);
> +    }
> +
> +    return num;
> +}
> +
>  static uint64_t aspeed_scu_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      AspeedSCUState *s = ASPEED_SCU(opaque);
> @@ -167,6 +180,12 @@ static uint64_t aspeed_scu_read(void *opaque, hwaddr offset, unsigned size)
>      }
>  
>      switch (reg) {
> +    case RNG_DATA:
> +        /* On hardware, RNG_DATA works regardless of
> +         * the state of the enable bit in RNG_CTRL
> +         */
> +        s->regs[RNG_DATA] = aspeed_scu_get_random();
> +        break;
>      case WAKEUP_EN:
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "%s: Read of write-only offset 0x%" HWADDR_PRIx "\n",
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH v3] aspeed_scu: Implement RNG register

[Peter Maydell]

On 29 May 2018 at 05:19, Joel Stanley <joel@jms.id.au> wrote:
> The ASPEED SoCs contain a single register that returns random data when
> read. This models that register so that guests can use it.
>
> The random number data register has a corresponding control register,
> however it returns data regardless of the state of the enabled bit, so
> the model follows this behaviour.
>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> v2:
>  - Remove call to qcrypto_random_init as this is done in main()
> v3:
>  - Add Cédric's reviewed-by
>  - Add a comment about why we don't check for the rng enable bit
> ---
>  hw/misc/aspeed_scu.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index 5e6d5744eeca..96db052389cc 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -16,6 +16,7 @@
>  #include "qapi/visitor.h"
>  #include "qemu/bitops.h"
>  #include "qemu/log.h"
> +#include "crypto/random.h"
>  #include "trace.h"
>
>  #define TO_REG(offset) ((offset) >> 2)
> @@ -154,6 +155,18 @@ static const uint32_t ast2500_a1_resets[ASPEED_SCU_NR_REGS] = {
>       [BMC_DEV_ID]      = 0x00002402U
>  };
>
> +static uint32_t aspeed_scu_get_random(void)
> +{
> +    Error *err = NULL;
> +    uint32_t num;
> +
> +    if (qcrypto_random_bytes((uint8_t *)&num, sizeof(num), &err)) {
> +        error_report_err(err);
> +    }
> +
> +    return num;
> +}

This will return an uninitialized value if qcrypto_random_bytes()
fails.

Does the guest use this value for cryptographic purposes?
In hw/misc/bcm2835_rng.c we opted to make qcrypto_random_bytes()
failing be fatal (for reasons noted in the comment there),
though that is a bit rough. exynos4210_rng() is more conveniently
able to just never report to the guest that the PRNG is ready.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH v3] aspeed_scu: Implement RNG register

[Joel Stanley]

On 30 May 2018 at 00:52, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 29 May 2018 at 05:19, Joel Stanley <joel@jms.id.au> wrote:
>> The ASPEED SoCs contain a single register that returns random data when
>> read. This models that register so that guests can use it.
>>
>> The random number data register has a corresponding control register,
>> however it returns data regardless of the state of the enabled bit, so
>> the model follows this behaviour.
>>
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> ---
>> v2:
>>  - Remove call to qcrypto_random_init as this is done in main()
>> v3:
>>  - Add Cédric's reviewed-by
>>  - Add a comment about why we don't check for the rng enable bit
>> ---
>>  hw/misc/aspeed_scu.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
>> index 5e6d5744eeca..96db052389cc 100644
>> --- a/hw/misc/aspeed_scu.c
>> +++ b/hw/misc/aspeed_scu.c
>> @@ -16,6 +16,7 @@
>>  #include "qapi/visitor.h"
>>  #include "qemu/bitops.h"
>>  #include "qemu/log.h"
>> +#include "crypto/random.h"
>>  #include "trace.h"
>>
>>  #define TO_REG(offset) ((offset) >> 2)
>> @@ -154,6 +155,18 @@ static const uint32_t ast2500_a1_resets[ASPEED_SCU_NR_REGS] = {
>>       [BMC_DEV_ID]      = 0x00002402U
>>  };
>>
>> +static uint32_t aspeed_scu_get_random(void)
>> +{
>> +    Error *err = NULL;
>> +    uint32_t num;
>> +
>> +    if (qcrypto_random_bytes((uint8_t *)&num, sizeof(num), &err)) {
>> +        error_report_err(err);
>> +    }
>> +
>> +    return num;
>> +}
>
> This will return an uninitialized value if qcrypto_random_bytes()
> fails.

Good catch.

> Does the guest use this value for cryptographic purposes?

The guest runs Linux, and Linux has a driver to expose this as a
hardware random number device.

As I said in the v2 thread, the aspeed model is for smoketesting of
firmware builds. I can't imagine a situation where it would make sense
to run a production BMC workload on qemu.

> In hw/misc/bcm2835_rng.c we opted to make qcrypto_random_bytes()
> failing be fatal (for reasons noted in the comment there),
> though that is a bit rough. exynos4210_rng() is more conveniently
> able to just never report to the guest that the PRNG is ready.

The hardware doesn't have the facility to indicate this to the guest.
If we want to be strict about handling the case where qcrypto has
failed, I will implement the same error handling as bcm2835_rng.