linux-kernel - Re: [PATCH v3 5/6] hwrng: exynos: Add SMC based TRNG operation

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]

Date: Sat, 22 Jun 2024 00:10:57 +0200
From: Lukasz Stelmach <l.stelmach@...sung.com>
To: Sam Protsenko <semen.protsenko@...aro.org>
Cc: Krzysztof Kozlowski <krzk+dt@...nel.org>,  Rob Herring
	<robh@...nel.org>,  Conor Dooley <conor+dt@...nel.org>,  Anand Moon
	<linux.amoon@...il.com>,  Olivia Mackall <olivia@...enic.com>,  Herbert Xu
	<herbert@...dor.apana.org.au>,  Alim Akhtar <alim.akhtar@...sung.com>,
	linux-samsung-soc@...r.kernel.org,  linux-crypto@...r.kernel.org,
	devicetree@...r.kernel.org,  linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org,  Marek Szyprowski <m.szyprowski@...sung.com>
Subject: Re: [PATCH v3 5/6] hwrng: exynos: Add SMC based TRNG operation

It was <2024-06-21 pią 14:40>, when Sam Protsenko wrote:
> On Fri, Jun 21, 2024 at 2:00 PM Lukasz Stelmach <l.stelmach@...sung.com> wrote:
>>
>> It was <2024-06-20 czw 18:13>, when Sam Protsenko wrote:
>> > On some Exynos chips like Exynos850 the access to Security Sub System
>> > (SSS) registers is protected with TrustZone, and therefore only possible
>> > from EL3 monitor software. The Linux kernel is running in EL1, so the
>> > only way for the driver to obtain TRNG data is via SMC calls to EL3
>> > monitor. Implement such SMC operation and use it when EXYNOS_SMC flag is
>> > set in the corresponding chip driver data.
>> >
>> > Signed-off-by: Sam Protsenko <semen.protsenko@...aro.org>
>> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
>> > ---
>> > Changes in v3:
>> >   - Added appropriate error messages for the case when init SMC command fails
>> >
>> > Changes in v2:
>> >   - Used the "reversed Christmas tree" style in the variable declaration
>> >     block in exynos_trng_do_read_smc()
>> >   - Renamed .quirks to .flags in the driver structure
>> >   - Added Krzysztof's R-b tag
>> >
>> >  drivers/char/hw_random/exynos-trng.c | 140 +++++++++++++++++++++++++--
>> >  1 file changed, 130 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/drivers/char/hw_random/exynos-trng.c b/drivers/char/hw_random/exynos-trng.c
>> > index 6ef2ee6c9804..9fa30583cc86 100644
>> > --- a/drivers/char/hw_random/exynos-trng.c
>> > +++ b/drivers/char/hw_random/exynos-trng.c
>>
>> [...]
>>
>>
>> > @@ -103,6 +163,24 @@ static int exynos_trng_init(struct hwrng *rng)
>> >       return 0;
>> >  }
>> >
>> > +static int exynos_trng_init_smc(struct hwrng *rng)
>> > +{
>> > +     struct exynos_trng_dev *trng = (struct exynos_trng_dev *)rng->priv;
>> > +     struct arm_smccc_res res;
>> > +     int ret = 0;
>> > +
>> > +     arm_smccc_smc(SMC_CMD_RANDOM, HWRNG_INIT, 0, 0, 0, 0, 0, 0, &res);
>> > +     if (res.a0 != HWRNG_RET_OK) {
>> > +             dev_err(trng->dev, "SMC command for TRNG init failed (%d)\n",
>> > +                     (int)res.a0);
>> > +             ret = -EIO;
>> > +     }
>> > +     if ((int)res.a0 == -1)
>> > +             dev_info(trng->dev, "Make sure LDFW is loaded by your BL\n");
>>
>> This is good, thank you for adding it. It can be even better though, if
>> you don't skimp on message length (-; I mean, I know what BL is, I can
>> fingure what LDFW is because you have explained to me and I can see the
>> source code, but somewone who sees it for the first time will be only
>> slightly less surprised than with v2 error message only. Come on, you
>> can make this message twice as long and it will still fit in 80 characters (-;
>>
>
> Guess my OCD got in the way and I just didn't want to break the line
> :) But yeah, LDFW = Loadable Firmware, and BL = bootloader. There is
> an "ldfw" partition on eMMC, and I noticed Samsung usually uses LDFW
> term, so I figured it was not a big deal to throw that abbreviation at
> the user. But I totally agree on BL part, it might be confusing. I
> don't have any strong opinion on this one. If you are going to apply
> v3, can I kindly ask you to change that message the way you want it to
> be?

I guess Olivia or Herbert will be applying it. Let me try… How about:

"Check if your bootloader loads the firmware (SMC) part of the driver."

>> Don't change it if v3 is the last. If not, please, make it more verbose.
>>
>> > +
>> > +     return ret;
>> > +}
>> > +
>>
>>
>> [...]
>>
>>
>> Kind regards,
>> --
>> Łukasz Stelmach
>> Samsung R&D Institute Poland
>> Samsung Electronics
>

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

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