linux-kernel - Re: [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <87o9nsde9r.fsf%l.stelmach@samsung.com>
Date:   Thu, 23 Nov 2017 19:46:24 +0100
From:   Łukasz Stelmach <l.stelmach@...sung.com>
To:     Krzysztof Kozlowski <krzk@...nel.org>
Cc:     Rob Herring <robh+dt@...nel.org>, Matt Mackall <mpm@...enic.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        devicetree@...r.kernel.org, linux-crypto@...r.kernel.org,
        linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
Subject: Re: [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver

It was <2017-11-23 czw 17:31>, when Krzysztof Kozlowski wrote:
> On Thu, Nov 23, 2017 at 4:09 PM, Łukasz Stelmach <l.stelmach@...sung.com> wrote:
>> Add support for True Random Number Generator found in Samsung Exynos
>> 5250+ SoCs.
>>
>> Signed-off-by: Łukasz Stelmach <l.stelmach@...sung.com>
>> ---
>>  MAINTAINERS                          |   7 +
>>  drivers/char/hw_random/Kconfig       |  12 ++
>>  drivers/char/hw_random/Makefile      |   1 +
>>  drivers/char/hw_random/exynos-trng.c | 256 +++++++++++++++++++++++++++++++++++
>>  4 files changed, 276 insertions(+)
>>  create mode 100644 drivers/char/hw_random/exynos-trng.c
>>

[...]

>>  endif # HW_RANDOM
>
> Thanks for working on TRNG.
>
> 1. I was rather thinking about extending existing exynos-rng.c [1] so
> it would be using TRNG as seed for PRNG as this gives you much more
> random data. Instead you developed totally separate driver which has
> its own benefits - one can choose which interface he wants. Although
> it is a little bit duplication.

As far as I can tell, these are two different devices. However, PRNG
shares hardware with the hash engine. Indeed there is a hardware to
connect TRNG and PRNG, but, IMHO, it might be hard to model that
dependency in kernel. To me it seems easier to read TRNG (or
/dev/random) and and write the result to PRNG manually (in software).

> 2. I understand that in your current version of TRNG driver there is
> no conflict with PRNG and they can work both at the same time?

I guess so. I expect no problems as long as TRNG_SEED_START is not set
in the HASH_SEED_CONF register.

> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/crypto/exynos-rng.c
>

>> +
>> +       trng->rng.init = exynos_trng_init;
>> +       trng->rng.read = exynos_trng_do_read;
>> +       trng->rng.priv = (unsigned long) trng;
>> +
>> +       platform_set_drvdata(pdev, trng);
>> +       trng->dev = &pdev->dev;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       trng->mem = devm_ioremap_resource(&pdev->dev, res);
>> +       if (IS_ERR(trng->mem)) {
>> +               dev_err(&pdev->dev, "Couldn't map IO resources.\n");
>> +               ret = PTR_ERR(trng->mem);
>> +               goto err_ioremap;
>> +       }
>> +
>> +       pm_runtime_enable(&pdev->dev);
>> +       ret = pm_runtime_get_sync(&pdev->dev);
>> +       if (ret < 0) {
>> +               dev_err(&pdev->dev, "pm_runtime_get_sync() failed: %d.\n", ret);
>> +               goto err_ioremap;
>
> Missing pm_runtime_disable?

Added a separate label down below.

>> +               dev_err(&pdev->dev, "Couldn't get clock.\n");
>> +               ret = PTR_ERR(trng->clk);
>> +               goto err_clock;
>> +       }
>> +
>> +       ret = clk_prepare_enable(trng->clk);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "Unable to enable the clk: %d.\n", ret);
>> +               goto err_clock;
>> +       }
>> +
>> +       ret = hwrng_register(&trng->rng);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "Couldn't register hwrng device.\n");
>> +               goto err_register;
>> +       }
>> +
>> +       dev_info(&pdev->dev, "Exynos True Random Number Generator.\n");
>> +
>> +       return 0;
>> +
>> +err_register:
>> +       clk_disable_unprepare(trng->clk);
>> +
>> +err_clock:
>> +       trng->mem = NULL;
>
> Why this has to be null-ified?

I found this pattern in omap-rng.c.

Removed.

I'll wait a little bit more before posting v2.

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

Download attachment "signature.asc" of type "application/pgp-signature" (473 bytes)