2024-03-29 07:56:07

by Gowthami Thiagarajan

[permalink] [raw]
Subject: Re: [PATCH REVIEW] hwrng: add exynos Secure World RNG device driver

On 2024-03-28 at 18:20:56, Alexey Klimov ([email protected]) wrote:
> The Exynos TRNG device is controlled by firmware and shared between
> non-secure world and secure world. Access to it is exposed via SMC
> interface which is implemented here. The firmware code does
> additional security checks, start-up test and some checks on resume.
>
> This device is found, for instance, in Google Tensor GS101-family
> of devices.
>
> Signed-off-by: Alexey Klimov <[email protected]>
> ---
Hi Alexey Klimov,

Please find few comments inline.

> drivers/char/hw_random/Kconfig | 12 +
> drivers/char/hw_random/Makefile | 1 +
> drivers/char/hw_random/exynos-swd-trng.c | 423 +++++++++++++++++++++++
> 3 files changed, 436 insertions(+)
> create mode 100644 drivers/char/hw_random/exynos-swd-trng.c
>
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 442c40efb200..bff7c3ec129a 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -479,6 +479,18 @@ config HW_RANDOM_EXYNOS
>
> If unsure, say Y.
>
> +config HW_RANDOM_EXYNOS_SWD
> + tristate "Exynos SWD HW random number generator support"
> + default n
> + help
> + This driver provides kernel-side support for accessing Samsung
> + TRNG hardware located in secure world using smc calls.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called exynos-swd-trng.
> +
> + If unsure, say N.
> +
> config HW_RANDOM_OPTEE
> tristate "OP-TEE based Random Number Generator support"
> depends on OPTEE
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index 32549a1186dc..ce64929d461a 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_HW_RANDOM_N2RNG) += n2-rng.o
> n2-rng-y := n2-drv.o n2-asm.o
> obj-$(CONFIG_HW_RANDOM_VIA) += via-rng.o
> obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-trng.o
> +obj-$(CONFIG_HW_RANDOM_EXYNOS_SWD) += exynos-swd-trng.o
> obj-$(CONFIG_HW_RANDOM_IXP4XX) += ixp4xx-rng.o
> obj-$(CONFIG_HW_RANDOM_OMAP) += omap-rng.o
> obj-$(CONFIG_HW_RANDOM_OMAP3_ROM) += omap3-rom-rng.o
> diff --git a/drivers/char/hw_random/exynos-swd-trng.c b/drivers/char/hw_random/exynos-swd-trng.c
> new file mode 100644
> index 000000000000..29def8e6d0b7
> --- /dev/null
> +++ b/drivers/char/hw_random/exynos-swd-trng.c
> @@ -0,0 +1,423 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * exynos-swd-trng.c - Random Number Generator driver for the exynos TRNG
> + * located in secure world
> + * Copyright (C) Linaro Ltd 2024 Alexey Klimov <[email protected]>
> + *
> + * Based on downstream driver:
> + * Copyright (C) 2018 Samsung Electronics
> + * Sehee Kim <[email protected]>
> + */
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/delay.h>
> +#include <linux/hw_random.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +/* DTRNG smc */
> +#define SMC_CMD_RANDOM (0x82001012)
> +
> +/* DTRNG smc commands */
> +#define HWRNG_INIT (0x0)
> +#define HWRNG_EXIT (0x1)
> +#define HWRNG_GET_DATA (0x2)
> +#define HWRNG_RESUME (0x3)
> +
> +#define HWRNG_RET_OK 0
> +#define HWRNG_RET_INVALID_ERROR 1
> +#define HWRNG_RET_RETRY_ERROR 2
> +#define HWRNG_RET_INVALID_FLAG_ERROR 3
> +#define HWRNG_RET_TEST_ERROR 4
> +#define HWRNG_RET_START_UP_TEST_DONE 5
> +#define HWRNG_RET_TEST_KAT_ERROR 0xC
> +
> +#define EXYRNG_START_UP_SIZE (4096 + 1)
> +#define EXYRNG_RETRY_MAX_COUNT 1000000
> +#define EXYRNG_START_UP_TEST_MAX_RETRY 2
> +
> +#define DRVNAME "exynos_swd_trng"
> +
> +enum state {
> + INACTIVE = 0,
> + ACTIVE,
> +};
> +
> +struct exyswd_rng {
> + struct hwrng rng;
> + enum state state;
> + struct device *dev;
> + /* to track and protect state of the device */
> + struct mutex lock;
> +};
> +
> +static int __exynos_cm_smc(u64 *arg0, u64 *arg1,
> + u64 *arg2, u64 *arg3)
> +{
> + struct arm_smccc_res res;
> +
> + arm_smccc_smc(*arg0, *arg1, *arg2, *arg3, 0, 0, 0, 0, &res);
> +
> + *arg0 = res.a0;
> + *arg1 = res.a1;
> + *arg2 = res.a2;
> + *arg3 = res.a3;
> +
> + return *arg0;
> +}
> +
> +static int exynos_cm_cmd(int cmd)
> +{
> + u64 reg0, reg1, reg2, reg3;
> +
> + reg0 = SMC_CMD_RANDOM;
> + reg1 = cmd;
> + reg3 = reg2 = 0;
> +
> + return __exynos_cm_smc(&reg0, &reg1, &reg2, &reg3);
> +}
> +
> +static int exynos_cm_get_data(u64 *arg0, u64 *arg1,
> + u64 *arg2, u64 *arg3)
> +{
> + *arg0 = SMC_CMD_RANDOM;
> + *arg1 = HWRNG_GET_DATA;
> + *arg3 = 0;
> +
> + return __exynos_cm_smc(arg0, arg1, arg2, arg3);
> +}

Can you avoid implementing specific SMC calls in this driver?
Instead, is it possible to use arm_smccc_1_1_invoke passing
corresponding arguements?

> +
> +static int exynos_swd_get_data(u64 *arg0, u64 *arg1, u64 *arg2, u64 *arg3,
> + struct exyswd_rng *exyswd_rng)
> +{
> + u32 retry_cnt = 0;
> + int ret;
> +
> + while (retry_cnt++ < EXYRNG_RETRY_MAX_COUNT) {
> + ret = exynos_cm_get_data(arg0, arg1, arg2, arg3);
> +
> + if (ret == HWRNG_RET_OK)
> + return 0;
> +
> + if (ret == HWRNG_RET_RETRY_ERROR) {
> + usleep_range(50, 100);
> + continue;
> + }
> +
> + if (ret == HWRNG_RET_TEST_ERROR) {
> + dev_dbg(exyswd_rng->dev, "error while testing\n");
> + return -EAGAIN;
> + }
> +
> + return -EFAULT;
> + }
> +
> + ret = -EFAULT;
> + dev_warn(exyswd_rng->dev, "retry counter is reached\n");
> + return ret;
> +}
> +
> +static int exynos_swd_init(void)
> +{
> + u32 retry_cnt = 0;
> + int ret;
> +
> + do {
> + ret = exynos_cm_cmd(HWRNG_INIT);
> + if (ret == HWRNG_RET_RETRY_ERROR) {
> + if (retry_cnt++ > EXYRNG_RETRY_MAX_COUNT)
> + break;
> +
> + usleep_range(50, 100);
> + }
> + } while (ret == HWRNG_RET_RETRY_ERROR);
> +
> + return ret;
> +}
> +
> +static void exynos_swd_exit(void)
> +{
> + u32 retry_cnt = 0;
> +
> + while (retry_cnt++ < EXYRNG_RETRY_MAX_COUNT) {
> + if (!exynos_cm_cmd(HWRNG_EXIT))
> + break;
> + usleep_range(50, 100);
> + }
> +}
> +
> +static int exynos_swd_startup_test(struct exyswd_rng *exyswd_rng)
> +{
> + u64 reg0, reg1, reg2, reg3;
> + int start_up_size = EXYRNG_START_UP_SIZE;
> + u32 retry_cnt = 0;
> + int ret;
> +
> + ret = exynos_swd_init();
> + if (ret != HWRNG_RET_OK) {
> + if (ret == HWRNG_RET_TEST_ERROR) {
> + ret = -EAGAIN;
> + goto out;
> + } else
> + return -EFAULT;
> + }
> +
> + while (start_up_size > 0) {
> + /* For start-up test the 3-rd argument has to be set to 1 */
> + reg2 = 1;

Can this be changed to a #define constant for better clarity?

> + ret = exynos_cm_get_data(&reg0, &reg1, &reg2, &reg3);
> + if (ret == HWRNG_RET_RETRY_ERROR) {
> + if (retry_cnt++ > EXYRNG_RETRY_MAX_COUNT) {
> + dev_warn(exyswd_rng->dev,
> + "exceeded retry in start-up test\n");
> + break;
> + }
> + usleep_range(50, 100);
> + continue;
> + }
> +
> + if (ret == HWRNG_RET_TEST_ERROR ||
> + ret == HWRNG_RET_TEST_KAT_ERROR) {
> + dev_err(exyswd_rng->dev,
> + "malfunction of TRNG(HW) is detected\n");
> + return -EFAULT;
> + }
> +
> + if (ret == HWRNG_RET_START_UP_TEST_DONE) {
> + dev_dbg(exyswd_rng->dev,
> + "start-up test is already done\n");
> + ret = 0;
> + break;
> + }
> +
> + if (ret != HWRNG_RET_OK) {
> + dev_dbg(exyswd_rng->dev, "failed to get random data\n");
> + return -EFAULT;
> + }
> +
> + start_up_size -= 32;
Similar to the above, please change this. And why 32 bytes?

> + retry_cnt = 0;
> + }
> +
> +out:
> + exynos_swd_exit();
> + return ret;
> +}
> +
> +static int exynos_swd_read(struct hwrng *rng, void *data, size_t max, bool wait)
> +{
> + struct exyswd_rng *exyswd_rng =
> + container_of(rng, struct exyswd_rng, rng);
> + u64 reg0, reg1, reg2, reg3;
> + u32 *read_buf = data;
> + u32 read_size = max;
> + u32 retry_cnt;
> + int ret = HWRNG_RET_OK;
> +
> + mutex_lock(&exyswd_rng->lock);
> + ret = exynos_swd_init();
> + if (ret != HWRNG_RET_OK) {
> + if (ret == HWRNG_RET_TEST_ERROR) {
> + ret = -EAGAIN;
> + goto out_locked;
> + } else {
> + mutex_unlock(&exyswd_rng->lock);
> + return -EFAULT;
> + }
> + }
> +
> + exyswd_rng->state = ACTIVE;
> + mutex_unlock(&exyswd_rng->lock);
> +
> + retry_cnt = 0;
> + while (read_size >= 8) {
> + reg2 = 0;
> + ret = exynos_swd_get_data(&reg0, &reg1, &reg2, &reg3, exyswd_rng);
> + if (ret)
> + goto out;
> +
> + *(u32 *)(read_buf++) = (u32)reg2;
> + *(u32 *)(read_buf++) = (u32)reg3;
> +
> + read_size -= 8;
> + retry_cnt = 0;
> + }
> +
> + /*
> + * rng_buf_size is 32 bytes or cache line usually, it is unlikely
> + * we will have remaining bytes to read here.
> + */
> + if (unlikely(read_size > 0)) {
> + reg2 = 0;
> + ret = exynos_swd_get_data(&reg0, &reg1, &reg2, &reg3, exyswd_rng);
> + if (ret)
> + goto out;
> + if (read_size >= 4) {
> + *(u32 *)(read_buf++) = (u32)reg2;
> + read_size -= 4;
> + }
> +
> + if (read_size) {
> + memcpy(read_buf, &reg3, read_size);
> + read_size = 0;
> + }
> + }
> +
> + ret = max;
> +out:
> + mutex_lock(&exyswd_rng->lock);
> +out_locked:
> + exynos_swd_exit();
> + exyswd_rng->state = INACTIVE;
> + mutex_unlock(&exyswd_rng->lock);
> +
> + return ret;
> +}
> +
> +static int exyswd_rng_probe(struct platform_device *pdev)
> +{
> + struct exyswd_rng *exyswd_rng;
> + int ret;
> +
> + exyswd_rng = devm_kzalloc(&pdev->dev, sizeof(*exyswd_rng), GFP_KERNEL);
> + if (!exyswd_rng)
> + return -ENOMEM;
> +
> + exyswd_rng->rng.name = DRVNAME;
> + exyswd_rng->rng.read = exynos_swd_read;
> + exyswd_rng->rng.quality = 500;
> + exyswd_rng->dev = &pdev->dev;
> + exyswd_rng->state = INACTIVE;
> + mutex_init(&exyswd_rng->lock);
> +
> + /*
> + * Do the startup test first. If it works we've got the device
> + * and can finish probe().
> + */
> + ret = exynos_swd_startup_test(exyswd_rng);
> + if (ret) {
> + dev_dbg(&pdev->dev, "start-up test failed\n");
> + return -ENODEV;
> + }
> +
> + ret = devm_hwrng_register(&pdev->dev, &exyswd_rng->rng);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "failed to register hwrng\n");
> +
> + platform_set_drvdata(pdev, exyswd_rng);
> + dev_set_drvdata(&pdev->dev, exyswd_rng);
> +
> + dev_info(&pdev->dev, "hwrng registered\n");
> +
> + return 0;
> +}
> +
> +static int exyswd_rng_remove(struct platform_device *pdev)
> +{
> + struct exyswd_rng *exyswd_rng = platform_get_drvdata(pdev);
> +
> + devm_hwrng_unregister(&pdev->dev, &exyswd_rng->rng);
> +
> + mutex_lock(&exyswd_rng->lock);
> + if (exyswd_rng->state == ACTIVE) {
> + exynos_swd_exit();
> + exyswd_rng->state = INACTIVE;
> + }
> + mutex_unlock(&exyswd_rng->lock);
> +
> + return 0;
> +}
> +
> +#if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM_RUNTIME)
> +static int exyswd_rng_suspend(struct device *dev)
> +{
> + struct exyswd_rng *exyswd_rng = dev_get_drvdata(dev);
> + int ret = 0;
> +
> + mutex_lock(&exyswd_rng->lock);
> + if (exyswd_rng->state) {
> + ret = exynos_cm_cmd(HWRNG_EXIT);
> + if (ret != HWRNG_RET_OK)
> + dev_warn(dev,
> + "failed to enter suspend, error %d\n", ret);
> + }
> + mutex_unlock(&exyswd_rng->lock);
> +
> + return ret;
> +}
> +
> +static int exyswd_rng_resume(struct device *dev)
> +{
> + struct exyswd_rng *exyswd_rng = dev_get_drvdata(dev);
> + int ret;
> +
> + mutex_lock(&exyswd_rng->lock);
> + ret = exynos_cm_cmd(HWRNG_RESUME);
> + if (ret != HWRNG_RET_OK)
> + dev_warn(dev, "failed to resume, error %d\n", ret);
> + if (exyswd_rng->state) {
> + ret = exynos_cm_cmd(HWRNG_INIT);
> + if (ret != HWRNG_RET_OK)
> + dev_warn(dev,
> + "failed to init hwrng on resume, error %d\n",
> + ret);
> + }
> + mutex_unlock(&exyswd_rng->lock);
> +
> + return ret;
> +}
> +#endif
> +
> +static UNIVERSAL_DEV_PM_OPS(exyswd_rng_pm_ops, exyswd_rng_suspend,
> + exyswd_rng_resume, NULL);
> +
> +static struct platform_driver exyswd_rng_driver = {
> + .probe = exyswd_rng_probe,
> + .remove = exyswd_rng_remove,
> + .driver = {
> + .name = DRVNAME,
> + .owner = THIS_MODULE,
> + .pm = &exyswd_rng_pm_ops,
> + },
> +};
> +
> +static struct platform_device *exyswd_rng_pdev;
> +
> +static int __init exyswd_rng_init(void)
> +{
> + int ret;
> +
> + exyswd_rng_pdev = platform_device_register_simple(DRVNAME, -1, NULL, 0);
> + if (IS_ERR(exyswd_rng_pdev))
> + pr_err(DRVNAME ": could not register device: %ld\n",
> + PTR_ERR(exyswd_rng_pdev));
> +
> + ret = platform_driver_register(&exyswd_rng_driver);
> + if (ret) {
> + platform_device_unregister(exyswd_rng_pdev);
> + return ret;
> + }
> +
> + pr_info("ExyRNG driver, (c) 2014 Samsung Electronics\n");
> +
> + return 0;
> +}
> +
> +static void __exit exyswd_rng_exit(void)
> +{
> + platform_driver_unregister(&exyswd_rng_driver);
> + platform_device_unregister(exyswd_rng_pdev);
> +}
> +
> +module_init(exyswd_rng_init);
> +module_exit(exyswd_rng_exit);
> +
> +MODULE_DESCRIPTION("Exynos SWD H/W Random Number Generator driver");
> +MODULE_AUTHOR("Alexey Klimov <[email protected]>");
> +MODULE_AUTHOR("Sehee Kim <[email protected]>");
> +MODULE_LICENSE("GPL");
> --
> 2.43.0
>