Re: [Qemu-devel] [PATCH v3 14/23] linux-user/aarch64: Use qemu_guest_getrandom for PAUTH keys

[Philippe Mathieu-Daudé]

On 3/15/19 4:26 AM, Richard Henderson wrote:
> Use a better interface for random numbers than rand() * 3.
> 
> Cc: Laurent Vivier <address@hidden>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  linux-user/aarch64/target_syscall.h |  2 --
>  linux-user/aarch64/cpu_loop.c       | 29 ++++++---------------------
>  linux-user/syscall.c                | 31 ++++++++++++++++++++++++-----
>  3 files changed, 32 insertions(+), 30 deletions(-)
> 
> diff --git a/linux-user/aarch64/target_syscall.h 
> b/linux-user/aarch64/target_syscall.h
> index b595e5da82..995e475c73 100644
> --- a/linux-user/aarch64/target_syscall.h
> +++ b/linux-user/aarch64/target_syscall.h
> @@ -29,6 +29,4 @@ struct target_pt_regs {
>  # define TARGET_PR_PAC_APDBKEY   (1 << 3)
>  # define TARGET_PR_PAC_APGAKEY   (1 << 4)
>  
> -void arm_init_pauth_key(ARMPACKey *key);
> -
>  #endif /* AARCH64_TARGET_SYSCALL_H */
> diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c
> index d75fd9d3e2..cedad39ca0 100644
> --- a/linux-user/aarch64/cpu_loop.c
> +++ b/linux-user/aarch64/cpu_loop.c
> @@ -20,6 +20,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu.h"
>  #include "cpu_loop-common.h"
> +#include "qemu/guest-random.h"
>  
>  #define get_user_code_u32(x, gaddr, env)                \
>      ({ abi_long __r = get_user_u32((x), (gaddr));       \
> @@ -147,24 +148,6 @@ void cpu_loop(CPUARMState *env)
>      }
>  }
>  
> -static uint64_t arm_rand64(void)
> -{
> -    int shift = 64 - clz64(RAND_MAX);
> -    int i, n = 64 / shift + (64 % shift != 0);
> -    uint64_t ret = 0;
> -
> -    for (i = 0; i < n; i++) {
> -        ret = (ret << shift) | rand();
> -    }
> -    return ret;
> -}
> -
> -void arm_init_pauth_key(ARMPACKey *key)
> -{
> -    key->lo = arm_rand64();
> -    key->hi = arm_rand64();
> -}
> -
>  void target_cpu_copy_regs(CPUArchState *env, struct target_pt_regs *regs)
>  {
>      ARMCPU *cpu = arm_env_get_cpu(env);
> @@ -192,11 +175,11 @@ void target_cpu_copy_regs(CPUArchState *env, struct 
> target_pt_regs *regs)
>  #endif
>  
>      if (cpu_isar_feature(aa64_pauth, cpu)) {
> -        arm_init_pauth_key(&env->apia_key);
> -        arm_init_pauth_key(&env->apib_key);
> -        arm_init_pauth_key(&env->apda_key);
> -        arm_init_pauth_key(&env->apdb_key);
> -        arm_init_pauth_key(&env->apga_key);
> +        qemu_guest_getrandom_nofail(&env->apia_key, sizeof(ARMPACKey));
> +        qemu_guest_getrandom_nofail(&env->apib_key, sizeof(ARMPACKey));
> +        qemu_guest_getrandom_nofail(&env->apda_key, sizeof(ARMPACKey));
> +        qemu_guest_getrandom_nofail(&env->apdb_key, sizeof(ARMPACKey));
> +        qemu_guest_getrandom_nofail(&env->apga_key, sizeof(ARMPACKey));
>      }
>  
>      ts->stack_base = info->start_stack;
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 8f7125cd67..c2168db1c8 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -111,6 +111,7 @@
>  
>  #include "qemu.h"
>  #include "qemu/guest-random.h"
> +#include "qapi/error.h"
>  #include "fd-trans.h"
>  
>  #ifndef CLONE_IO
> @@ -9731,25 +9732,45 @@ static abi_long do_syscall1(void *cpu_env, int num, 
> abi_long arg1,
>                      int all = (TARGET_PR_PAC_APIAKEY | TARGET_PR_PAC_APIBKEY 
> |
>                                 TARGET_PR_PAC_APDAKEY | TARGET_PR_PAC_APDBKEY 
> |
>                                 TARGET_PR_PAC_APGAKEY);
> +                    int ret = 0;
> +                    Error *err = NULL;
> +
>                      if (arg2 == 0) {
>                          arg2 = all;
>                      } else if (arg2 & ~all) {
>                          return -TARGET_EINVAL;
>                      }
>                      if (arg2 & TARGET_PR_PAC_APIAKEY) {
> -                        arm_init_pauth_key(&env->apia_key);
> +                        ret |= qemu_guest_getrandom(&env->apia_key,
> +                                                    sizeof(ARMPACKey), &err);
>                      }
>                      if (arg2 & TARGET_PR_PAC_APIBKEY) {
> -                        arm_init_pauth_key(&env->apib_key);
> +                        ret |= qemu_guest_getrandom(&env->apib_key,
> +                                                    sizeof(ARMPACKey), &err);
>                      }
>                      if (arg2 & TARGET_PR_PAC_APDAKEY) {
> -                        arm_init_pauth_key(&env->apda_key);
> +                        ret |= qemu_guest_getrandom(&env->apda_key,
> +                                                    sizeof(ARMPACKey), &err);
>                      }
>                      if (arg2 & TARGET_PR_PAC_APDBKEY) {
> -                        arm_init_pauth_key(&env->apdb_key);
> +                        ret |= qemu_guest_getrandom(&env->apdb_key,
> +                                                    sizeof(ARMPACKey), &err);
>                      }
>                      if (arg2 & TARGET_PR_PAC_APGAKEY) {
> -                        arm_init_pauth_key(&env->apga_key);
> +                        ret |= qemu_guest_getrandom(&env->apga_key,
> +                                                    sizeof(ARMPACKey), &err);
> +                    }
> +                    if (ret != 0) {
> +                        /*
> +                         * Some unknown failure in the crypto.  The best
> +                         * we can do is log it and fail the syscall.
> +                         * The real syscall cannot fail this way.
> +                         */
> +                        qemu_log_mask(LOG_UNIMP,
> +                                      "PR_PAC_RESET_KEYS: Crypto failure: 
> %s",
> +                                      error_get_pretty(err));
> +                        error_free(err);
> +                        return -TARGET_EIO;

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

>                      }
>                      return 0;
>                  }
>