These patch series introduce a security random number generator
which provides a generic interface to get hardware rnd from Secure
state. The Secure state can be Arm Trusted Firmware(ATF), Trusted
Execution Environment(TEE), or even EL2 hypervisor.
Patch #1..2 adds sec-rng kernel driver for Trustzone based SoCs.
For security awareness SoCs on ARMv8 with TrustZone enabled,
peripherals like entropy sources is not accessible from normal world
(linux) and rather accessible from secure world (HYP/ATF/TEE) only.
This driver aims to provide a generic interface to Arm Trusted
Firmware or Hypervisor rng service.
changes since v1:
- rename mt67xx-rng to mtk-sec-rng since all MediaTek ARMv8 SoCs can reuse
this driver.
- refine coding style and unnecessary check.
changes since v2:
- remove unused comments.
- remove redundant variable.
changes since v3:
- add dt-bindings for MediaTek rng with TrustZone enabled.
- revise HWRNG SMC call fid.
changes since v4:
- move bindings to the arm/firmware directory.
- revise driver init flow to check more property.
changes since v5:
- refactor to more generic security rng driver which
is not platform specific.
*** BLURB HERE ***
Neal Liu (2):
dt-bindings: rng: add bindings for sec-rng
hwrng: add sec-rng driver
.../devicetree/bindings/rng/sec-rng.yaml | 53 ++++++
drivers/char/hw_random/Kconfig | 13 ++
drivers/char/hw_random/Makefile | 1 +
drivers/char/hw_random/sec-rng.c | 155 ++++++++++++++++++
4 files changed, 222 insertions(+)
create mode 100644 Documentation/devicetree/bindings/rng/sec-rng.yaml
create mode 100644 drivers/char/hw_random/sec-rng.c
--
2.18.0
For security awareness SoCs on ARMv8 with TrustZone enabled,
peripherals like entropy sources is not accessible from normal world
(linux) and rather accessible from secure world (HYP/ATF/TEE) only.
This driver aims to provide a generic interface to Arm Trusted
Firmware or Hypervisor rng service.
Signed-off-by: Neal Liu <[email protected]>
---
drivers/char/hw_random/Kconfig | 13 ++++
drivers/char/hw_random/Makefile | 1 +
drivers/char/hw_random/sec-rng.c | 155 ++++++++++++++++++++++++++++++++++++++
3 files changed, 169 insertions(+)
create mode 100644 drivers/char/hw_random/sec-rng.c
diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 9bc46da..cb9c8a9 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -474,6 +474,19 @@ config HW_RANDOM_KEYSTONE
help
This option enables Keystone's hardware random generator.
+config HW_RANDOM_SECURE
+ tristate "Arm Security Random Number Generator support"
+ depends on HAVE_ARM_SMCCC || COMPILE_TEST
+ default HW_RANDOM
+ help
+ This driver provides kernel-side support for the Arm Security
+ Random Number Generator.
+
+ To compile this driver as a module, choose M here. the
+ module will be called sec-rng.
+
+ If unsure, say Y.
+
endif # HW_RANDOM
config UML_RANDOM
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index a7801b4..04533d1 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -41,3 +41,4 @@ obj-$(CONFIG_HW_RANDOM_S390) += s390-trng.o
obj-$(CONFIG_HW_RANDOM_KEYSTONE) += ks-sa-rng.o
obj-$(CONFIG_HW_RANDOM_OPTEE) += optee-rng.o
obj-$(CONFIG_HW_RANDOM_NPCM) += npcm-rng.o
+obj-$(CONFIG_HW_RANDOM_SECURE) += sec-rng.o
diff --git a/drivers/char/hw_random/sec-rng.c b/drivers/char/hw_random/sec-rng.c
new file mode 100644
index 0000000..c6d3872
--- /dev/null
+++ b/drivers/char/hw_random/sec-rng.c
@@ -0,0 +1,155 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 MediaTek Inc.
+ */
+
+#include <linux/arm-smccc.h>
+#include <linux/hw_random.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#define SMC_RET_NUM 4
+#define SEC_RND_SIZE (sizeof(u32) * SMC_RET_NUM)
+
+#define HWRNG_SMC_FAST_CALL_VAL(func_num) \
+ ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_32, \
+ ARM_SMCCC_OWNER_SIP, (func_num))
+
+#define to_sec_rng(p) container_of(p, struct sec_rng_priv, rng)
+
+typedef void (sec_rng_fn)(unsigned long, unsigned long, unsigned long,
+ unsigned long, unsigned long, unsigned long,
+ unsigned long, unsigned long,
+ struct arm_smccc_res *);
+
+struct sec_rng_priv {
+ u16 func_num;
+ sec_rng_fn *rng_fn;
+ struct hwrng rng;
+};
+
+/* Simple wrapper functions to be able to use a function pointer */
+static void sec_rng_smc(unsigned long a0, unsigned long a1,
+ unsigned long a2, unsigned long a3,
+ unsigned long a4, unsigned long a5,
+ unsigned long a6, unsigned long a7,
+ struct arm_smccc_res *res)
+{
+ arm_smccc_smc(a0, a1, a2, a3, a4, a5, a6, a7, res);
+}
+
+static void sec_rng_hvc(unsigned long a0, unsigned long a1,
+ unsigned long a2, unsigned long a3,
+ unsigned long a4, unsigned long a5,
+ unsigned long a6, unsigned long a7,
+ struct arm_smccc_res *res)
+{
+ arm_smccc_hvc(a0, a1, a2, a3, a4, a5, a6, a7, res);
+}
+
+static bool __sec_get_rnd(struct sec_rng_priv *priv, uint32_t *val)
+{
+ struct arm_smccc_res res;
+
+ priv->rng_fn(HWRNG_SMC_FAST_CALL_VAL(priv->func_num),
+ 0, 0, 0, 0, 0, 0, 0, &res);
+
+ if (!res.a0 && !res.a1 && !res.a2 && !res.a3)
+ return false;
+
+ val[0] = res.a0;
+ val[1] = res.a1;
+ val[2] = res.a2;
+ val[3] = res.a3;
+
+ return true;
+}
+
+static int sec_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
+{
+ struct sec_rng_priv *priv = to_sec_rng(rng);
+ u32 val[4] = {0};
+ int retval = 0;
+ int i;
+
+ while (max >= SEC_RND_SIZE) {
+ if (!__sec_get_rnd(priv, val))
+ return retval;
+
+ for (i = 0; i < SMC_RET_NUM; i++) {
+ *(u32 *)buf = val[i];
+ buf += sizeof(u32);
+ }
+
+ retval += SEC_RND_SIZE;
+ max -= SEC_RND_SIZE;
+ }
+
+ return retval;
+}
+
+static int sec_rng_probe(struct platform_device *pdev)
+{
+ struct sec_rng_priv *priv;
+ const char *method;
+ int ret;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ if (of_property_read_string(pdev->dev.of_node, "method", &method))
+ return -ENXIO;
+
+ if (!strncmp("smc", method, strlen("smc")))
+ priv->rng_fn = sec_rng_smc;
+ else if (!strncmp("hvc", method, strlen("hvc")))
+ priv->rng_fn = sec_rng_hvc;
+
+ if (IS_ERR(priv->rng_fn)) {
+ dev_err(&pdev->dev, "method %s is not supported\n", method);
+ return -EINVAL;
+ }
+
+ if (of_property_read_u16(pdev->dev.of_node, "method-fid",
+ &priv->func_num))
+ return -ENXIO;
+
+ if (of_property_read_u16(pdev->dev.of_node, "quality",
+ &priv->rng.quality))
+ return -ENXIO;
+
+ priv->rng.name = pdev->name;
+ priv->rng.read = sec_rng_read;
+ priv->rng.priv = (unsigned long)&pdev->dev;
+
+ ret = devm_hwrng_register(&pdev->dev, &priv->rng);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to register rng device: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct of_device_id sec_rng_match[] = {
+ { .compatible = "arm,sec-rng", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, sec_rng_match);
+
+static struct platform_driver sec_rng_driver = {
+ .probe = sec_rng_probe,
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .owner = THIS_MODULE,
+ .of_match_table = sec_rng_match,
+ },
+};
+
+module_platform_driver(sec_rng_driver);
+
+MODULE_DESCRIPTION("Security Random Number Generator Driver");
+MODULE_AUTHOR("Neal Liu <[email protected]>");
+MODULE_LICENSE("GPL");
--
1.7.9.5
On Tue, Jun 02, 2020 at 04:14:38PM +0800, Neal Liu wrote:
> For security awareness SoCs on ARMv8 with TrustZone enabled,
> peripherals like entropy sources is not accessible from normal world
> (linux) and rather accessible from secure world (HYP/ATF/TEE) only.
> This driver aims to provide a generic interface to Arm Trusted
> Firmware or Hypervisor rng service.
>
> Signed-off-by: Neal Liu <[email protected]>
> ---
> drivers/char/hw_random/Kconfig | 13 ++++
> drivers/char/hw_random/Makefile | 1 +
> drivers/char/hw_random/sec-rng.c | 155 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 169 insertions(+)
> create mode 100644 drivers/char/hw_random/sec-rng.c
>
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 9bc46da..cb9c8a9 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -474,6 +474,19 @@ config HW_RANDOM_KEYSTONE
> help
> This option enables Keystone's hardware random generator.
>
> +config HW_RANDOM_SECURE
> + tristate "Arm Security Random Number Generator support"
> + depends on HAVE_ARM_SMCCC || COMPILE_TEST
> + default HW_RANDOM
> + help
> + This driver provides kernel-side support for the Arm Security
> + Random Number Generator.
> +
> + To compile this driver as a module, choose M here. the
> + module will be called sec-rng.
> +
> + If unsure, say Y.
Why Y?
> +
> endif # HW_RANDOM
>
> config UML_RANDOM
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index a7801b4..04533d1 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -41,3 +41,4 @@ obj-$(CONFIG_HW_RANDOM_S390) += s390-trng.o
> obj-$(CONFIG_HW_RANDOM_KEYSTONE) += ks-sa-rng.o
> obj-$(CONFIG_HW_RANDOM_OPTEE) += optee-rng.o
> obj-$(CONFIG_HW_RANDOM_NPCM) += npcm-rng.o
> +obj-$(CONFIG_HW_RANDOM_SECURE) += sec-rng.o
> diff --git a/drivers/char/hw_random/sec-rng.c b/drivers/char/hw_random/sec-rng.c
> new file mode 100644
> index 0000000..c6d3872
> --- /dev/null
> +++ b/drivers/char/hw_random/sec-rng.c
> @@ -0,0 +1,155 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 MediaTek Inc.
> + */
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/hw_random.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#define SMC_RET_NUM 4
> +#define SEC_RND_SIZE (sizeof(u32) * SMC_RET_NUM)
> +
> +#define HWRNG_SMC_FAST_CALL_VAL(func_num) \
> + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_32, \
> + ARM_SMCCC_OWNER_SIP, (func_num))
> +
> +#define to_sec_rng(p) container_of(p, struct sec_rng_priv, rng)
> +
> +typedef void (sec_rng_fn)(unsigned long, unsigned long, unsigned long,
> + unsigned long, unsigned long, unsigned long,
> + unsigned long, unsigned long,
> + struct arm_smccc_res *);
Why not throw some more unsigned longs in there? :)
Seriously, no variable names for these? Why not?
And given that you only use the first parameter, why have 7 of them that
are not used at all? That feels pointless and needlessly complex.
> +
> +struct sec_rng_priv {
> + u16 func_num;
> + sec_rng_fn *rng_fn;
> + struct hwrng rng;
> +};
Nit, if you put 'struct hwrng' at the top of the structure, your
"to_sec_rng()" macro resolves to a simple cast, no math at all.
> +
> +/* Simple wrapper functions to be able to use a function pointer */
> +static void sec_rng_smc(unsigned long a0, unsigned long a1,
> + unsigned long a2, unsigned long a3,
> + unsigned long a4, unsigned long a5,
> + unsigned long a6, unsigned long a7,
> + struct arm_smccc_res *res)
> +{
> + arm_smccc_smc(a0, a1, a2, a3, a4, a5, a6, a7, res);
> +}
> +
> +static void sec_rng_hvc(unsigned long a0, unsigned long a1,
> + unsigned long a2, unsigned long a3,
> + unsigned long a4, unsigned long a5,
> + unsigned long a6, unsigned long a7,
> + struct arm_smccc_res *res)
> +{
> + arm_smccc_hvc(a0, a1, a2, a3, a4, a5, a6, a7, res);
> +}
> +
> +static bool __sec_get_rnd(struct sec_rng_priv *priv, uint32_t *val)
> +{
> + struct arm_smccc_res res;
> +
> + priv->rng_fn(HWRNG_SMC_FAST_CALL_VAL(priv->func_num),
> + 0, 0, 0, 0, 0, 0, 0, &res);
See, all 0's :(
You could hard-code them in the functions above instead.
But, all of this pointer indirection is really odd, why is it needed at
all? Why not just call one or the other depending on the "type" at
runtime? Wouldn't that actually be faster (hint, it is...), if you
cared about speed here (hint, I doubt it matters).
> +
> + if (!res.a0 && !res.a1 && !res.a2 && !res.a3)
> + return false;
> +
> + val[0] = res.a0;
> + val[1] = res.a1;
> + val[2] = res.a2;
> + val[3] = res.a3;
So no values out of the random number generator can be 0? Feels like an
odd thing for a random number not to be allowed to do, why this
restriction?
> +
> + return true;
> +}
> +
> +static int sec_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
> +{
> + struct sec_rng_priv *priv = to_sec_rng(rng);
> + u32 val[4] = {0};
> + int retval = 0;
> + int i;
> +
> + while (max >= SEC_RND_SIZE) {
> + if (!__sec_get_rnd(priv, val))
> + return retval;
> +
> + for (i = 0; i < SMC_RET_NUM; i++) {
> + *(u32 *)buf = val[i];
> + buf += sizeof(u32);
Wait, what happens if buf is not a multiple of 4? Didn't you just
overwrite some memory above with the previous line?
> + }
> +
> + retval += SEC_RND_SIZE;
> + max -= SEC_RND_SIZE;
> + }
> +
> + return retval;
> +}
> +
> +static int sec_rng_probe(struct platform_device *pdev)
> +{
> + struct sec_rng_priv *priv;
> + const char *method;
> + int ret;
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + if (of_property_read_string(pdev->dev.of_node, "method", &method))
> + return -ENXIO;
> +
> + if (!strncmp("smc", method, strlen("smc")))
> + priv->rng_fn = sec_rng_smc;
> + else if (!strncmp("hvc", method, strlen("hvc")))
> + priv->rng_fn = sec_rng_hvc;
> +
> + if (IS_ERR(priv->rng_fn)) {
How can this ever be true?
Just put another else on the above list and you should be fine.
> + dev_err(&pdev->dev, "method %s is not supported\n", method);
> + return -EINVAL;
> + }
> +
> + if (of_property_read_u16(pdev->dev.of_node, "method-fid",
> + &priv->func_num))
> + return -ENXIO;
> +
> + if (of_property_read_u16(pdev->dev.of_node, "quality",
> + &priv->rng.quality))
> + return -ENXIO;
> +
> + priv->rng.name = pdev->name;
> + priv->rng.read = sec_rng_read;
> + priv->rng.priv = (unsigned long)&pdev->dev;
> +
> + ret = devm_hwrng_register(&pdev->dev, &priv->rng);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to register rng device: %d\n", ret);
Doesn't the caller print out something if this fails?
thanks,
greg k-h
On Tue, 2 Jun 2020 at 10:15, Neal Liu <[email protected]> wrote:
>
> These patch series introduce a security random number generator
> which provides a generic interface to get hardware rnd from Secure
> state. The Secure state can be Arm Trusted Firmware(ATF), Trusted
> Execution Environment(TEE), or even EL2 hypervisor.
>
> Patch #1..2 adds sec-rng kernel driver for Trustzone based SoCs.
> For security awareness SoCs on ARMv8 with TrustZone enabled,
> peripherals like entropy sources is not accessible from normal world
> (linux) and rather accessible from secure world (HYP/ATF/TEE) only.
> This driver aims to provide a generic interface to Arm Trusted
> Firmware or Hypervisor rng service.
>
>
> changes since v1:
> - rename mt67xx-rng to mtk-sec-rng since all MediaTek ARMv8 SoCs can reuse
> this driver.
> - refine coding style and unnecessary check.
>
> changes since v2:
> - remove unused comments.
> - remove redundant variable.
>
> changes since v3:
> - add dt-bindings for MediaTek rng with TrustZone enabled.
> - revise HWRNG SMC call fid.
>
> changes since v4:
> - move bindings to the arm/firmware directory.
> - revise driver init flow to check more property.
>
> changes since v5:
> - refactor to more generic security rng driver which
> is not platform specific.
>
> *** BLURB HERE ***
>
> Neal Liu (2):
> dt-bindings: rng: add bindings for sec-rng
> hwrng: add sec-rng driver
>
There is no reason to model a SMC call as a driver, and represent it
via a DT node like this.
It would be much better if this SMC interface is made truly generic,
and wired into the arch_get_random() interface, which can be used much
earlier.
> .../devicetree/bindings/rng/sec-rng.yaml | 53 ++++++
> drivers/char/hw_random/Kconfig | 13 ++
> drivers/char/hw_random/Makefile | 1 +
> drivers/char/hw_random/sec-rng.c | 155 ++++++++++++++++++
> 4 files changed, 222 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/rng/sec-rng.yaml
> create mode 100644 drivers/char/hw_random/sec-rng.c
>
> --
> 2.18.0
On Tue, 2020-06-02 at 21:02 +0800, Marc Zyngier wrote:
> On 2020-06-02 13:14, Ard Biesheuvel wrote:
> > On Tue, 2 Jun 2020 at 10:15, Neal Liu <[email protected]> wrote:
> >>
> >> These patch series introduce a security random number generator
> >> which provides a generic interface to get hardware rnd from Secure
> >> state. The Secure state can be Arm Trusted Firmware(ATF), Trusted
> >> Execution Environment(TEE), or even EL2 hypervisor.
> >>
> >> Patch #1..2 adds sec-rng kernel driver for Trustzone based SoCs.
> >> For security awareness SoCs on ARMv8 with TrustZone enabled,
> >> peripherals like entropy sources is not accessible from normal world
> >> (linux) and rather accessible from secure world (HYP/ATF/TEE) only.
> >> This driver aims to provide a generic interface to Arm Trusted
> >> Firmware or Hypervisor rng service.
> >>
> >>
> >> changes since v1:
> >> - rename mt67xx-rng to mtk-sec-rng since all MediaTek ARMv8 SoCs can
> >> reuse
> >> this driver.
> >> - refine coding style and unnecessary check.
> >>
> >> changes since v2:
> >> - remove unused comments.
> >> - remove redundant variable.
> >>
> >> changes since v3:
> >> - add dt-bindings for MediaTek rng with TrustZone enabled.
> >> - revise HWRNG SMC call fid.
> >>
> >> changes since v4:
> >> - move bindings to the arm/firmware directory.
> >> - revise driver init flow to check more property.
> >>
> >> changes since v5:
> >> - refactor to more generic security rng driver which
> >> is not platform specific.
> >>
> >> *** BLURB HERE ***
> >>
> >> Neal Liu (2):
> >> dt-bindings: rng: add bindings for sec-rng
> >> hwrng: add sec-rng driver
> >>
> >
> > There is no reason to model a SMC call as a driver, and represent it
> > via a DT node like this.
>
> +1.
>
> > It would be much better if this SMC interface is made truly generic,
> > and wired into the arch_get_random() interface, which can be used much
> > earlier.
>
> Wasn't there a plan to standardize a SMC call to rule them all?
>
> M.
Could you give us a hint how to make this SMC interface more generic in
addition to my approach?
There is no (easy) way to get platform-independent SMC function ID,
which is why we encode it into device tree, and provide a generic
driver. In this way, different devices can be mapped and then get
different function ID internally.
On Wed, 2020-06-03 at 08:40 +0100, Marc Zyngier wrote:
> On 2020-06-03 08:29, Neal Liu wrote:
> > On Tue, 2020-06-02 at 21:02 +0800, Marc Zyngier wrote:
> >> On 2020-06-02 13:14, Ard Biesheuvel wrote:
> >> > On Tue, 2 Jun 2020 at 10:15, Neal Liu <[email protected]> wrote:
> >> >>
> >> >> These patch series introduce a security random number generator
> >> >> which provides a generic interface to get hardware rnd from Secure
> >> >> state. The Secure state can be Arm Trusted Firmware(ATF), Trusted
> >> >> Execution Environment(TEE), or even EL2 hypervisor.
> >> >>
> >> >> Patch #1..2 adds sec-rng kernel driver for Trustzone based SoCs.
> >> >> For security awareness SoCs on ARMv8 with TrustZone enabled,
> >> >> peripherals like entropy sources is not accessible from normal world
> >> >> (linux) and rather accessible from secure world (HYP/ATF/TEE) only.
> >> >> This driver aims to provide a generic interface to Arm Trusted
> >> >> Firmware or Hypervisor rng service.
> >> >>
> >> >>
> >> >> changes since v1:
> >> >> - rename mt67xx-rng to mtk-sec-rng since all MediaTek ARMv8 SoCs can
> >> >> reuse
> >> >> this driver.
> >> >> - refine coding style and unnecessary check.
> >> >>
> >> >> changes since v2:
> >> >> - remove unused comments.
> >> >> - remove redundant variable.
> >> >>
> >> >> changes since v3:
> >> >> - add dt-bindings for MediaTek rng with TrustZone enabled.
> >> >> - revise HWRNG SMC call fid.
> >> >>
> >> >> changes since v4:
> >> >> - move bindings to the arm/firmware directory.
> >> >> - revise driver init flow to check more property.
> >> >>
> >> >> changes since v5:
> >> >> - refactor to more generic security rng driver which
> >> >> is not platform specific.
> >> >>
> >> >> *** BLURB HERE ***
> >> >>
> >> >> Neal Liu (2):
> >> >> dt-bindings: rng: add bindings for sec-rng
> >> >> hwrng: add sec-rng driver
> >> >>
> >> >
> >> > There is no reason to model a SMC call as a driver, and represent it
> >> > via a DT node like this.
> >>
> >> +1.
> >>
> >> > It would be much better if this SMC interface is made truly generic,
> >> > and wired into the arch_get_random() interface, which can be used much
> >> > earlier.
> >>
> >> Wasn't there a plan to standardize a SMC call to rule them all?
> >>
> >> M.
> >
> > Could you give us a hint how to make this SMC interface more generic in
> > addition to my approach?
> > There is no (easy) way to get platform-independent SMC function ID,
> > which is why we encode it into device tree, and provide a generic
> > driver. In this way, different devices can be mapped and then get
> > different function ID internally.
>
> The idea is simply to have *one* single ID that caters for all
> implementations, just like we did for PSCI at the time. This
> requires ARM to edict a standard, which is what I was referring
> to above.
>
> There is zero benefit in having a platform-dependent ID. It just
> pointlessly increases complexity, and means we cannot use the RNG
> before the firmware tables are available (yes, we need it that
> early).
>
> M.
Do you know which ARM expert could edict this standard?
Or is there any chance that we can make one? And be reviewed by
maintainers?
On Wed, Jun 03, 2020 at 08:40:58AM +0100, Marc Zyngier wrote:
> On 2020-06-03 08:29, Neal Liu wrote:
> > On Tue, 2020-06-02 at 21:02 +0800, Marc Zyngier wrote:
> > > On 2020-06-02 13:14, Ard Biesheuvel wrote:
> > > > On Tue, 2 Jun 2020 at 10:15, Neal Liu <[email protected]> wrote:
> > > >>
> > > >> These patch series introduce a security random number generator
> > > >> which provides a generic interface to get hardware rnd from Secure
> > > >> state. The Secure state can be Arm Trusted Firmware(ATF), Trusted
> > > >> Execution Environment(TEE), or even EL2 hypervisor.
> > > >>
> > > >> Patch #1..2 adds sec-rng kernel driver for Trustzone based SoCs.
> > > >> For security awareness SoCs on ARMv8 with TrustZone enabled,
> > > >> peripherals like entropy sources is not accessible from normal world
> > > >> (linux) and rather accessible from secure world (HYP/ATF/TEE) only.
> > > >> This driver aims to provide a generic interface to Arm Trusted
> > > >> Firmware or Hypervisor rng service.
> > > >>
> > > >>
> > > >> changes since v1:
> > > >> - rename mt67xx-rng to mtk-sec-rng since all MediaTek ARMv8 SoCs can
> > > >> reuse
> > > >> this driver.
> > > >> - refine coding style and unnecessary check.
> > > >>
> > > >> changes since v2:
> > > >> - remove unused comments.
> > > >> - remove redundant variable.
> > > >>
> > > >> changes since v3:
> > > >> - add dt-bindings for MediaTek rng with TrustZone enabled.
> > > >> - revise HWRNG SMC call fid.
> > > >>
> > > >> changes since v4:
> > > >> - move bindings to the arm/firmware directory.
> > > >> - revise driver init flow to check more property.
> > > >>
> > > >> changes since v5:
> > > >> - refactor to more generic security rng driver which
> > > >> is not platform specific.
> > > >>
> > > >> *** BLURB HERE ***
> > > >>
> > > >> Neal Liu (2):
> > > >> dt-bindings: rng: add bindings for sec-rng
> > > >> hwrng: add sec-rng driver
> > > >>
> > > >
> > > > There is no reason to model a SMC call as a driver, and represent it
> > > > via a DT node like this.
> > >
> > > +1.
> > >
> > > > It would be much better if this SMC interface is made truly generic,
> > > > and wired into the arch_get_random() interface, which can be used much
> > > > earlier.
> > >
> > > Wasn't there a plan to standardize a SMC call to rule them all?
> > >
> > > M.
> >
> > Could you give us a hint how to make this SMC interface more generic in
> > addition to my approach?
> > There is no (easy) way to get platform-independent SMC function ID,
> > which is why we encode it into device tree, and provide a generic
> > driver. In this way, different devices can be mapped and then get
> > different function ID internally.
>
> The idea is simply to have *one* single ID that caters for all
> implementations, just like we did for PSCI at the time. This
> requires ARM to edict a standard, which is what I was referring
> to above.
This sounds all too familiar.
This kind of thing is something that ARM have seems to shy away from
doing - it's a point I brought up many years ago when the whole
trustzone thing first appeared with its SMC call. Those around the
conference table were not interested - ARM seemed to prefer every
vendor to do off and do their own thing with the SMC interface.
Then OMAP came along with its SMC interfaces, and so did the pain of
not having a standardised way to configure the L2C when Linux was
running in the non-secure world, resulting in stuff like l2c_configure
etc, where each and every implementation has to supply a function to
call its platform specific SMC interfaces to configure a piece of
hardware common across many different platforms.
ARM have seemed reluctant to standardise on stuff like this, so
unless someone pushes hard for it from inside ARM, I doubt it will
ever happen.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up
+ Jose
On Wed, Jun 03, 2020 at 03:54:17PM +0800, Neal Liu wrote:
> On Wed, 2020-06-03 at 08:40 +0100, Marc Zyngier wrote:
[...]
> > The idea is simply to have *one* single ID that caters for all
> > implementations, just like we did for PSCI at the time. This
> > requires ARM to edict a standard, which is what I was referring
> > to above.
> >
> > There is zero benefit in having a platform-dependent ID. It just
> > pointlessly increases complexity, and means we cannot use the RNG
> > before the firmware tables are available (yes, we need it that
> > early).
> >
>
> Do you know which ARM expert could edict this standard?
> Or is there any chance that we can make one? And be reviewed by
> maintainers?
>
Jose Marinho is working on the spec, may be he has more updates on the
timeline.
--
Regards,
Sudeep
On 2020-06-03 08:54, Neal Liu wrote:
> On Wed, 2020-06-03 at 08:40 +0100, Marc Zyngier wrote:
>> On 2020-06-03 08:29, Neal Liu wrote:
[...]
>> > Could you give us a hint how to make this SMC interface more generic in
>> > addition to my approach?
>> > There is no (easy) way to get platform-independent SMC function ID,
>> > which is why we encode it into device tree, and provide a generic
>> > driver. In this way, different devices can be mapped and then get
>> > different function ID internally.
>>
>> The idea is simply to have *one* single ID that caters for all
>> implementations, just like we did for PSCI at the time. This
>> requires ARM to edict a standard, which is what I was referring
>> to above.
>>
>> There is zero benefit in having a platform-dependent ID. It just
>> pointlessly increases complexity, and means we cannot use the RNG
>> before the firmware tables are available (yes, we need it that
>> early).
>>
>> M.
>
> Do you know which ARM expert could edict this standard?
> Or is there any chance that we can make one? And be reviewed by
> maintainers?
Sudeep already mentioned Jose's effort to offer a standard.
Hopefully he will *soon* be able to give us something that can be
implemented everywhere (firmware, kernel, but also hypervisors),
as the need exists across the whole stack.
M.
--
Jazz is not dead. It just smells funny...
On Wed, 2020-06-03 at 17:34 +0800, Russell King - ARM Linux admin wrote:
> On Wed, Jun 03, 2020 at 08:40:58AM +0100, Marc Zyngier wrote:
> > On 2020-06-03 08:29, Neal Liu wrote:
> > > On Tue, 2020-06-02 at 21:02 +0800, Marc Zyngier wrote:
> > > > On 2020-06-02 13:14, Ard Biesheuvel wrote:
> > > > > On Tue, 2 Jun 2020 at 10:15, Neal Liu <[email protected]> wrote:
> > > > >>
> > > > >> These patch series introduce a security random number generator
> > > > >> which provides a generic interface to get hardware rnd from Secure
> > > > >> state. The Secure state can be Arm Trusted Firmware(ATF), Trusted
> > > > >> Execution Environment(TEE), or even EL2 hypervisor.
> > > > >>
> > > > >> Patch #1..2 adds sec-rng kernel driver for Trustzone based SoCs.
> > > > >> For security awareness SoCs on ARMv8 with TrustZone enabled,
> > > > >> peripherals like entropy sources is not accessible from normal world
> > > > >> (linux) and rather accessible from secure world (HYP/ATF/TEE) only.
> > > > >> This driver aims to provide a generic interface to Arm Trusted
> > > > >> Firmware or Hypervisor rng service.
> > > > >>
> > > > >>
> > > > >> changes since v1:
> > > > >> - rename mt67xx-rng to mtk-sec-rng since all MediaTek ARMv8 SoCs can
> > > > >> reuse
> > > > >> this driver.
> > > > >> - refine coding style and unnecessary check.
> > > > >>
> > > > >> changes since v2:
> > > > >> - remove unused comments.
> > > > >> - remove redundant variable.
> > > > >>
> > > > >> changes since v3:
> > > > >> - add dt-bindings for MediaTek rng with TrustZone enabled.
> > > > >> - revise HWRNG SMC call fid.
> > > > >>
> > > > >> changes since v4:
> > > > >> - move bindings to the arm/firmware directory.
> > > > >> - revise driver init flow to check more property.
> > > > >>
> > > > >> changes since v5:
> > > > >> - refactor to more generic security rng driver which
> > > > >> is not platform specific.
> > > > >>
> > > > >> *** BLURB HERE ***
> > > > >>
> > > > >> Neal Liu (2):
> > > > >> dt-bindings: rng: add bindings for sec-rng
> > > > >> hwrng: add sec-rng driver
> > > > >>
> > > > >
> > > > > There is no reason to model a SMC call as a driver, and represent it
> > > > > via a DT node like this.
> > > >
> > > > +1.
> > > >
> > > > > It would be much better if this SMC interface is made truly generic,
> > > > > and wired into the arch_get_random() interface, which can be used much
> > > > > earlier.
> > > >
> > > > Wasn't there a plan to standardize a SMC call to rule them all?
> > > >
> > > > M.
> > >
> > > Could you give us a hint how to make this SMC interface more generic in
> > > addition to my approach?
> > > There is no (easy) way to get platform-independent SMC function ID,
> > > which is why we encode it into device tree, and provide a generic
> > > driver. In this way, different devices can be mapped and then get
> > > different function ID internally.
> >
> > The idea is simply to have *one* single ID that caters for all
> > implementations, just like we did for PSCI at the time. This
> > requires ARM to edict a standard, which is what I was referring
> > to above.
>
> This sounds all too familiar.
>
> This kind of thing is something that ARM have seems to shy away from
> doing - it's a point I brought up many years ago when the whole
> trustzone thing first appeared with its SMC call. Those around the
> conference table were not interested - ARM seemed to prefer every
> vendor to do off and do their own thing with the SMC interface.
Does that mean it make sense to model a sec-rng driver, and get each
vendor's SMC function id by DT node?
>
> Then OMAP came along with its SMC interfaces, and so did the pain of
> not having a standardised way to configure the L2C when Linux was
> running in the non-secure world, resulting in stuff like l2c_configure
> etc, where each and every implementation has to supply a function to
> call its platform specific SMC interfaces to configure a piece of
> hardware common across many different platforms.
>
> ARM have seemed reluctant to standardise on stuff like this, so
> unless someone pushes hard for it from inside ARM, I doubt it will
> ever happen.
>
On Fri, Jun 05, 2020 at 04:59:42PM +0800, Neal Liu wrote:
> On Fri, 2020-06-05 at 09:09 +0100, Russell King - ARM Linux admin wrote:
> > On Fri, Jun 05, 2020 at 03:19:03PM +0800, Neal Liu wrote:
> > > On Wed, 2020-06-03 at 17:34 +0800, Russell King - ARM Linux admin wrote:
> > > > This kind of thing is something that ARM have seems to shy away from
> > > > doing - it's a point I brought up many years ago when the whole
> > > > trustzone thing first appeared with its SMC call. Those around the
> > > > conference table were not interested - ARM seemed to prefer every
> > > > vendor to do off and do their own thing with the SMC interface.
> > >
> > > Does that mean it make sense to model a sec-rng driver, and get each
> > > vendor's SMC function id by DT node?
> >
> > _If_ vendors have already gone off and decided to use different SMC
> > function IDs for this, while keeping the rest of the SMC interface
> > the same, then the choice has already been made.
> >
> > I know on 32-bit that some of the secure world implementations can't
> > be changed; they're burnt into the ROM. I believe on 64-bit that isn't
> > the case, which makes it easier to standardise.
> >
> > Do you have visibility of how this SMC is implemented in the secure
> > side? Is it in ATF, and is it done as a vendor hack or is there an
> > element of generic implementation to it? Has it been submitted
> > upstream to the main ATF repository?
> >
>
> Take MediaTek as an example, some SoCs are implemented in ATF, some of
> them are implemented in TEE. We have no plan to make generic
> implementation in "secure world".
I think you have your answer right there - by _not_ making the API
generic and giving no motivation to use it, different vendors are
going to do different things (maybe even with a different API as well)
so there's no point the kernel driver pretending to be a generic
driver. If the driver isn't going to be generic, I see little point in
the SMC function number being in DT.
I think that as a _whole_ is a big mistake - there should be a generic
kernel driver for this, and there should be a standardised interface to
it through firmware. So, I would encourage you to try to get it
accepted one way or another amongst vendors as a standardised
interface.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up
Hi Neal,
On Fri, 5 Jun 2020 at 14:40, Neal Liu <[email protected]> wrote:
>
> On Fri, 2020-06-05 at 09:09 +0100, Russell King - ARM Linux admin wrote:
> > On Fri, Jun 05, 2020 at 03:19:03PM +0800, Neal Liu wrote:
> > > On Wed, 2020-06-03 at 17:34 +0800, Russell King - ARM Linux admin wrote:
> > > > This kind of thing is something that ARM have seems to shy away from
> > > > doing - it's a point I brought up many years ago when the whole
> > > > trustzone thing first appeared with its SMC call. Those around the
> > > > conference table were not interested - ARM seemed to prefer every
> > > > vendor to do off and do their own thing with the SMC interface.
> > >
> > > Does that mean it make sense to model a sec-rng driver, and get each
> > > vendor's SMC function id by DT node?
> >
> > _If_ vendors have already gone off and decided to use different SMC
> > function IDs for this, while keeping the rest of the SMC interface
> > the same, then the choice has already been made.
> >
> > I know on 32-bit that some of the secure world implementations can't
> > be changed; they're burnt into the ROM. I believe on 64-bit that isn't
> > the case, which makes it easier to standardise.
> >
> > Do you have visibility of how this SMC is implemented in the secure
> > side? Is it in ATF, and is it done as a vendor hack or is there an
> > element of generic implementation to it? Has it been submitted
> > upstream to the main ATF repository?
> >
>
> Take MediaTek as an example, some SoCs are implemented in ATF, some of
> them are implemented in TEE.
In case your TEE implementation is derived from OP-TEE, then I will
suggest you to re-use OP-TEE based RNG driver [1]. With that, you just
need to implement an OP-TEE based pseudo trusted application (similar
to this [2]) specific to your platform and need to extend driver UUID
config table [3] with UUID of your platform specific pseudo TA. This
way you can avoid using hardcoded DT based SMC approach and rather use
auto RNG device detection provided by TEE bus.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/char/hw_random/optee-rng.c
[2] https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/plat-synquacer/rng_pta.c
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/char/hw_random/optee-rng.c#n273
-Sumit
> We have no plan to make generic
> implementation in "secure world".
>
> Due to there must have different implementation in secure world for
> vendors, we plan to provide a generic SMC interface in secure rng kernel
> driver for more flexibility.
>
> Vendors can decide which "secure world" they want (HYP/ATF/TEE) by
> different smc/hvc and different SMC function IDs in DT node.
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 2020-06-03 08:54, Neal Liu wrote:
Hi Neal,
> Do you know which ARM expert could edict this standard?
> Or is there any chance that we can make one? And be reviewed by
> maintainers?
It appears that ARM just released a beta version of the spec at [1].
I'd encourage you (and anyone else) to have a look at it and provide
feedback to ARM.
Thanks,
M.
[1]
https://developer.arm.com/-/media/Files/pdf/DEN0098-True_Random_Number_Generator_Firmware_Interface-1.0BET2.pdf
--
Jazz is not dead. It just smells funny...
Hi Marc,
On Thu, 2020-06-18 at 10:50 +0100, Marc Zyngier wrote:
> On 2020-06-03 08:54, Neal Liu wrote:
>
> Hi Neal,
>
> > Do you know which ARM expert could edict this standard?
> > Or is there any chance that we can make one? And be reviewed by
> > maintainers?
>
> It appears that ARM just released a beta version of the spec at [1].
>
> I'd encourage you (and anyone else) to have a look at it and provide
> feedback to ARM.
>
> Thanks,
>
> M.
>
> [1]
> https://developer.arm.com/-/media/Files/pdf/DEN0098-True_Random_Number_Generator_Firmware_Interface-1.0BET2.pdf
I also received this spec from ARM. I'll take a look and see if it meets
our needs.
Thanks for your sharing.