Re: [PATCH v2 5/5] KVM: arm64: implement the TRNG hypervisor call
On Thu, 5 Nov 2020 at 15:13, Marc Zyngier wrote:
>
> On 2020-11-05 12:56, Andre Przywara wrote:
> > From: Ard Biesheuvel
> >
> > Provide a hypervisor implementation of the ARM architected TRNG
> > firmware
> > interface described in ARM spec DEN0098. All function IDs are
> > implemented,
> > including both 32-bit and 64-bit versions of the TRNG_RND service,
> > which
> > is the centerpiece of the API.
> >
> > The API is backed by arch_get_unsigned_seed_long(), which is
> > implemented
> > in terms of RNDRRS currently, and will be alternatively backed by a SMC
> > call to the secure firmware using same interface after a future patch.
> > If neither are available, the kernel's entropy pool is used instead.
> >
> > Signed-off-by: Ard Biesheuvel
> > Signed-off-by: Andre Przywara
> > ---
> > arch/arm64/include/asm/kvm_host.h | 2 +
> > arch/arm64/kvm/Makefile | 2 +-
> > arch/arm64/kvm/hypercalls.c | 6 ++
> > arch/arm64/kvm/trng.c | 91 +++
> > 4 files changed, 100 insertions(+), 1 deletion(-)
> > create mode 100644 arch/arm64/kvm/trng.c
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h
> > b/arch/arm64/include/asm/kvm_host.h
> > index 781d029b8aa8..615932bacf76 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -652,4 +652,6 @@ bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu
> > *vcpu);
> > #define kvm_arm_vcpu_sve_finalized(vcpu) \
> > ((vcpu)->arch.flags & KVM_ARM64_VCPU_SVE_FINALIZED)
> >
> > +int kvm_trng_call(struct kvm_vcpu *vcpu);
> > +
> > #endif /* __ARM64_KVM_HOST_H__ */
> > diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> > index 1504c81fbf5d..a510037e3270 100644
> > --- a/arch/arm64/kvm/Makefile
> > +++ b/arch/arm64/kvm/Makefile
> > @@ -16,7 +16,7 @@ kvm-y := $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o
> > $(KVM)/eventfd.o \
> >inject_fault.o regmap.o va_layout.o handle_exit.o \
> >guest.o debug.o reset.o sys_regs.o \
> >vgic-sys-reg-v3.o fpsimd.o pmu.o \
> > - aarch32.o arch_timer.o \
> > + aarch32.o arch_timer.o trng.o \
> >vgic/vgic.o vgic/vgic-init.o \
> >vgic/vgic-irqfd.o vgic/vgic-v2.o \
> >vgic/vgic-v3.o vgic/vgic-v4.o \
> > diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
> > index 25ea4ecb6449..ead21b98b620 100644
> > --- a/arch/arm64/kvm/hypercalls.c
> > +++ b/arch/arm64/kvm/hypercalls.c
> > @@ -71,6 +71,12 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
> > if (gpa != GPA_INVALID)
> > val = gpa;
> > break;
> > + case ARM_SMCCC_TRNG_VERSION:
> > + case ARM_SMCCC_TRNG_FEATURES:
> > + case ARM_SMCCC_TRNG_GET_UUID:
> > + case ARM_SMCCC_TRNG_RND32:
> > + case ARM_SMCCC_TRNG_RND64:
> > + return kvm_trng_call(vcpu);
> > default:
> > return kvm_psci_call(vcpu);
> > }
> > diff --git a/arch/arm64/kvm/trng.c b/arch/arm64/kvm/trng.c
> > new file mode 100644
> > index ..5a27b2d99977
> > --- /dev/null
> > +++ b/arch/arm64/kvm/trng.c
> > @@ -0,0 +1,91 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (C) 2020 Arm Ltd.
> > +
> > +#include
> > +#include
> > +
> > +#include
> > +
> > +#include
> > +
> > +#define ARM_SMCCC_TRNG_VERSION_1_0 0x1UL
> > +
> > +#define TRNG_SUCCESS 0UL
>
> SMCCC_RET_SUCCESS
>
> > +#define TRNG_NOT_SUPPORTED ((unsigned long)-1)
>
> SMCCC_RET_NOT_SUPPORTED
>
> > +#define TRNG_INVALID_PARAMETER ((unsigned long)-2)
>
> *crap*. Why isn't that the same value as SMCCC_RET_INVALID_PARAMETER?
> Is it too late to fix the spec?
>
The SMCC_RET_ and TRNG_ ID spaces are deliberately disjoint,
so that we can add TRNG result codes that are only relevant in the
contex of this ABI without having to modify the SMCCC spec.
> > +#define TRNG_NO_ENTROPY ((unsigned long)-3)
> > +
> > +#define MAX_BITS32 96
> > +#define MAX_BITS64 192
>
> Nothing seems to be using these definitions.
>
Indeed.
> > +
> > +static const uuid_t arm_smc_trng_uuid __aligned(4) = UUID_INIT(
> > + 0x023534a2, 0xe0bc, 0x45ec, 0x95, 0xdd, 0x33, 0x34, 0xc1, 0xcc, 0x31,
> > 0x89);
>
> I object to the lack of Easter egg in this UUID. Or at least one I can
> understand. ;-)
>
> > +
> > +static int kvm_trng_do_rnd(struct kvm_vcpu *vcpu, int size)
> > +{
> > + u32 num_bits = smccc_get_arg1(vcpu);
> > + u64 bits[3];
> > + int i;
> > +
> > + if (num_bits > 3 * size) {
> > + smccc_set_retval(vcpu, TRNG_INVALID_PARAMETER, 0, 0, 0);
> > + return 1;
> > + }
> > +
> > + /* get as many bits as we need to fulfil the request */
> > + for (i = 0; i < DIV_ROUND_UP(num_bits, 64); i++)
> > + /* use the arm64 specific backend directly if one exists */
> > + if (!arch_get_random_seed_long((unsigned long
Re: [PATCH v2 5/5] KVM: arm64: implement the TRNG hypervisor call
On 2020-11-05 12:56, Andre Przywara wrote:
From: Ard Biesheuvel
Provide a hypervisor implementation of the ARM architected TRNG
firmware
interface described in ARM spec DEN0098. All function IDs are
implemented,
including both 32-bit and 64-bit versions of the TRNG_RND service,
which
is the centerpiece of the API.
The API is backed by arch_get_unsigned_seed_long(), which is
implemented
in terms of RNDRRS currently, and will be alternatively backed by a SMC
call to the secure firmware using same interface after a future patch.
If neither are available, the kernel's entropy pool is used instead.
Signed-off-by: Ard Biesheuvel
Signed-off-by: Andre Przywara
---
arch/arm64/include/asm/kvm_host.h | 2 +
arch/arm64/kvm/Makefile | 2 +-
arch/arm64/kvm/hypercalls.c | 6 ++
arch/arm64/kvm/trng.c | 91 +++
4 files changed, 100 insertions(+), 1 deletion(-)
create mode 100644 arch/arm64/kvm/trng.c
diff --git a/arch/arm64/include/asm/kvm_host.h
b/arch/arm64/include/asm/kvm_host.h
index 781d029b8aa8..615932bacf76 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -652,4 +652,6 @@ bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu
*vcpu);
#define kvm_arm_vcpu_sve_finalized(vcpu) \
((vcpu)->arch.flags & KVM_ARM64_VCPU_SVE_FINALIZED)
+int kvm_trng_call(struct kvm_vcpu *vcpu);
+
#endif /* __ARM64_KVM_HOST_H__ */
diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
index 1504c81fbf5d..a510037e3270 100644
--- a/arch/arm64/kvm/Makefile
+++ b/arch/arm64/kvm/Makefile
@@ -16,7 +16,7 @@ kvm-y := $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o
$(KVM)/eventfd.o \
inject_fault.o regmap.o va_layout.o handle_exit.o \
guest.o debug.o reset.o sys_regs.o \
vgic-sys-reg-v3.o fpsimd.o pmu.o \
-aarch32.o arch_timer.o \
+aarch32.o arch_timer.o trng.o \
vgic/vgic.o vgic/vgic-init.o \
vgic/vgic-irqfd.o vgic/vgic-v2.o \
vgic/vgic-v3.o vgic/vgic-v4.o \
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
index 25ea4ecb6449..ead21b98b620 100644
--- a/arch/arm64/kvm/hypercalls.c
+++ b/arch/arm64/kvm/hypercalls.c
@@ -71,6 +71,12 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
if (gpa != GPA_INVALID)
val = gpa;
break;
+ case ARM_SMCCC_TRNG_VERSION:
+ case ARM_SMCCC_TRNG_FEATURES:
+ case ARM_SMCCC_TRNG_GET_UUID:
+ case ARM_SMCCC_TRNG_RND32:
+ case ARM_SMCCC_TRNG_RND64:
+ return kvm_trng_call(vcpu);
default:
return kvm_psci_call(vcpu);
}
diff --git a/arch/arm64/kvm/trng.c b/arch/arm64/kvm/trng.c
new file mode 100644
index ..5a27b2d99977
--- /dev/null
+++ b/arch/arm64/kvm/trng.c
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2020 Arm Ltd.
+
+#include
+#include
+
+#include
+
+#include
+
+#define ARM_SMCCC_TRNG_VERSION_1_0 0x1UL
+
+#define TRNG_SUCCESS 0UL
SMCCC_RET_SUCCESS
+#define TRNG_NOT_SUPPORTED ((unsigned long)-1)
SMCCC_RET_NOT_SUPPORTED
+#define TRNG_INVALID_PARAMETER ((unsigned long)-2)
*crap*. Why isn't that the same value as SMCCC_RET_INVALID_PARAMETER?
Is it too late to fix the spec?
+#define TRNG_NO_ENTROPY((unsigned long)-3)
+
+#define MAX_BITS32 96
+#define MAX_BITS64 192
Nothing seems to be using these definitions.
+
+static const uuid_t arm_smc_trng_uuid __aligned(4) = UUID_INIT(
+ 0x023534a2, 0xe0bc, 0x45ec, 0x95, 0xdd, 0x33, 0x34, 0xc1, 0xcc, 0x31,
0x89);
I object to the lack of Easter egg in this UUID. Or at least one I can
understand. ;-)
+
+static int kvm_trng_do_rnd(struct kvm_vcpu *vcpu, int size)
+{
+ u32 num_bits = smccc_get_arg1(vcpu);
+ u64 bits[3];
+ int i;
+
+ if (num_bits > 3 * size) {
+ smccc_set_retval(vcpu, TRNG_INVALID_PARAMETER, 0, 0, 0);
+ return 1;
+ }
+
+ /* get as many bits as we need to fulfil the request */
+ for (i = 0; i < DIV_ROUND_UP(num_bits, 64); i++)
+ /* use the arm64 specific backend directly if one exists */
+ if (!arch_get_random_seed_long((unsigned long *)[i]))
+ bits[i] = get_random_long();
+
+ if (num_bits % 64)
+ bits[i - 1] &= U64_MAX >> (64 - (num_bits % 64));
+
+ while (i < ARRAY_SIZE(bits))
+ bits[i++] = 0;
I just wasted 3 minutes trying to understand what this was doing, only
to
realise this is clearing the [MAX_BITS:num_bits] range.
How about using a bitmap instead? It would result in the exact same data
structure, only much more readable (and no u64->unsigned long casts).
+
+ if (size == 32)
+ smccc_set_retval(vcpu, TRNG_SUCCESS, lower_32_bits(bits[1]),
+
[PATCH v2 5/5] KVM: arm64: implement the TRNG hypervisor call
From: Ard Biesheuvel
Provide a hypervisor implementation of the ARM architected TRNG firmware
interface described in ARM spec DEN0098. All function IDs are implemented,
including both 32-bit and 64-bit versions of the TRNG_RND service, which
is the centerpiece of the API.
The API is backed by arch_get_unsigned_seed_long(), which is implemented
in terms of RNDRRS currently, and will be alternatively backed by a SMC
call to the secure firmware using same interface after a future patch.
If neither are available, the kernel's entropy pool is used instead.
Signed-off-by: Ard Biesheuvel
Signed-off-by: Andre Przywara
---
arch/arm64/include/asm/kvm_host.h | 2 +
arch/arm64/kvm/Makefile | 2 +-
arch/arm64/kvm/hypercalls.c | 6 ++
arch/arm64/kvm/trng.c | 91 +++
4 files changed, 100 insertions(+), 1 deletion(-)
create mode 100644 arch/arm64/kvm/trng.c
diff --git a/arch/arm64/include/asm/kvm_host.h
b/arch/arm64/include/asm/kvm_host.h
index 781d029b8aa8..615932bacf76 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -652,4 +652,6 @@ bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu);
#define kvm_arm_vcpu_sve_finalized(vcpu) \
((vcpu)->arch.flags & KVM_ARM64_VCPU_SVE_FINALIZED)
+int kvm_trng_call(struct kvm_vcpu *vcpu);
+
#endif /* __ARM64_KVM_HOST_H__ */
diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
index 1504c81fbf5d..a510037e3270 100644
--- a/arch/arm64/kvm/Makefile
+++ b/arch/arm64/kvm/Makefile
@@ -16,7 +16,7 @@ kvm-y := $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o
$(KVM)/eventfd.o \
inject_fault.o regmap.o va_layout.o handle_exit.o \
guest.o debug.o reset.o sys_regs.o \
vgic-sys-reg-v3.o fpsimd.o pmu.o \
-aarch32.o arch_timer.o \
+aarch32.o arch_timer.o trng.o \
vgic/vgic.o vgic/vgic-init.o \
vgic/vgic-irqfd.o vgic/vgic-v2.o \
vgic/vgic-v3.o vgic/vgic-v4.o \
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
index 25ea4ecb6449..ead21b98b620 100644
--- a/arch/arm64/kvm/hypercalls.c
+++ b/arch/arm64/kvm/hypercalls.c
@@ -71,6 +71,12 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
if (gpa != GPA_INVALID)
val = gpa;
break;
+ case ARM_SMCCC_TRNG_VERSION:
+ case ARM_SMCCC_TRNG_FEATURES:
+ case ARM_SMCCC_TRNG_GET_UUID:
+ case ARM_SMCCC_TRNG_RND32:
+ case ARM_SMCCC_TRNG_RND64:
+ return kvm_trng_call(vcpu);
default:
return kvm_psci_call(vcpu);
}
diff --git a/arch/arm64/kvm/trng.c b/arch/arm64/kvm/trng.c
new file mode 100644
index ..5a27b2d99977
--- /dev/null
+++ b/arch/arm64/kvm/trng.c
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2020 Arm Ltd.
+
+#include
+#include
+
+#include
+
+#include
+
+#define ARM_SMCCC_TRNG_VERSION_1_0 0x1UL
+
+#define TRNG_SUCCESS 0UL
+#define TRNG_NOT_SUPPORTED ((unsigned long)-1)
+#define TRNG_INVALID_PARAMETER ((unsigned long)-2)
+#define TRNG_NO_ENTROPY((unsigned long)-3)
+
+#define MAX_BITS32 96
+#define MAX_BITS64 192
+
+static const uuid_t arm_smc_trng_uuid __aligned(4) = UUID_INIT(
+ 0x023534a2, 0xe0bc, 0x45ec, 0x95, 0xdd, 0x33, 0x34, 0xc1, 0xcc, 0x31,
0x89);
+
+static int kvm_trng_do_rnd(struct kvm_vcpu *vcpu, int size)
+{
+ u32 num_bits = smccc_get_arg1(vcpu);
+ u64 bits[3];
+ int i;
+
+ if (num_bits > 3 * size) {
+ smccc_set_retval(vcpu, TRNG_INVALID_PARAMETER, 0, 0, 0);
+ return 1;
+ }
+
+ /* get as many bits as we need to fulfil the request */
+ for (i = 0; i < DIV_ROUND_UP(num_bits, 64); i++)
+ /* use the arm64 specific backend directly if one exists */
+ if (!arch_get_random_seed_long((unsigned long *)[i]))
+ bits[i] = get_random_long();
+
+ if (num_bits % 64)
+ bits[i - 1] &= U64_MAX >> (64 - (num_bits % 64));
+
+ while (i < ARRAY_SIZE(bits))
+ bits[i++] = 0;
+
+ if (size == 32)
+ smccc_set_retval(vcpu, TRNG_SUCCESS, lower_32_bits(bits[1]),
+upper_32_bits(bits[0]),
lower_32_bits(bits[0]));
+ else
+ smccc_set_retval(vcpu, TRNG_SUCCESS, bits[2], bits[1], bits[0]);
+
+ memzero_explicit(bits, sizeof(bits));
+ return 1;
+}
+
+int kvm_trng_call(struct kvm_vcpu *vcpu)
+{
+ const __le32 *u = (__le32 *)arm_smc_trng_uuid.b;
+ u32 func_id = smccc_get_function(vcpu);
+ unsigned long val = TRNG_NOT_SUPPORTED;
+ int size = 64;
+
+ switch (func_id) {
+ case ARM_SMCCC_TRNG_VERSION:
+ val = ARM_SMCCC_TRNG_VERSION_1_0;
+ break;
+ case ARM_SMCCC_TRNG_FEATURES:
+
