[PATCH] fscrypt: log the crypto algorithm implementations

[Eric Biggers]

Log the crypto algorithm driver name for each fscrypt encryption mode on
its first use, also showing a friendly name for the mode.

This will help people determine whether the expected implementations are
being used.  In some cases we've seen people do benchmarks and reject
using encryption for performance reasons, when in fact they used a much
slower implementation of AES-XTS than was possible on the hardware.  It
can make an enormous difference; e.g., AES-XTS on ARM is about 10x
faster with the crypto extensions (AES instructions) than without.

This also makes it more obvious which modes are being used, now that
fscrypt supports multiple combinations of modes.

Example messages (with default modes, on x86_64):

[   35.492057] fscrypt: AES-256-CTS-CBC using implementation 
"cts(cbc-aes-aesni)"
[   35.492171] fscrypt: AES-256-XTS using implementation "xts-aes-aesni"

Note: algorithms can be dynamically added to the crypto API, which can
result in different implementations being used at different times.  But
this is rare; for most users, showing the first will be good enough.

Signed-off-by: Eric Biggers 
---

Note: this patch is on top of the other fscrypt patches I've sent out for 4.18.

 fs/crypto/keyinfo.c | 102 +---
 1 file changed, 68 insertions(+), 34 deletions(-)

diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 41f6025d5d7a..68b5baef5960 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -148,44 +148,64 @@ static int find_and_derive_key(const struct inode *inode,
return err;
 }
 
-static const struct {
+static struct fscrypt_mode {
+   const char *friendly_name;
const char *cipher_str;
int keysize;
+   bool logged_impl_name;
 } available_modes[] = {
-   [FS_ENCRYPTION_MODE_AES_256_XTS]  = { "xts(aes)",   64 },
-   [FS_ENCRYPTION_MODE_AES_256_CTS]  = { "cts(cbc(aes))",  32 },
-   [FS_ENCRYPTION_MODE_AES_128_CBC]  = { "cbc(aes)",   16 },
-   [FS_ENCRYPTION_MODE_AES_128_CTS]  = { "cts(cbc(aes))",  16 },
-   [FS_ENCRYPTION_MODE_SPECK128_256_XTS] = { "xts(speck128)",  64 },
-   [FS_ENCRYPTION_MODE_SPECK128_256_CTS] = { "cts(cbc(speck128))", 32 },
+   [FS_ENCRYPTION_MODE_AES_256_XTS] = {
+   .friendly_name = "AES-256-XTS",
+   .cipher_str = "xts(aes)",
+   .keysize = 64,
+   },
+   [FS_ENCRYPTION_MODE_AES_256_CTS] = {
+   .friendly_name = "AES-256-CTS-CBC",
+   .cipher_str = "cts(cbc(aes))",
+   .keysize = 32,
+   },
+   [FS_ENCRYPTION_MODE_AES_128_CBC] = {
+   .friendly_name = "AES-128-CBC",
+   .cipher_str = "cbc(aes)",
+   .keysize = 16,
+   },
+   [FS_ENCRYPTION_MODE_AES_128_CTS] = {
+   .friendly_name = "AES-128-CTS-CBC",
+   .cipher_str = "cts(cbc(aes))",
+   .keysize = 16,
+   },
+   [FS_ENCRYPTION_MODE_SPECK128_256_XTS] = {
+   .friendly_name = "Speck128/256-XTS",
+   .cipher_str = "xts(speck128)",
+   .keysize = 64,
+   },
+   [FS_ENCRYPTION_MODE_SPECK128_256_CTS] = {
+   .friendly_name = "Speck128/256-CTS-CBC",
+   .cipher_str = "cts(cbc(speck128))",
+   .keysize = 32,
+   },
 };
 
-static int determine_cipher_type(struct fscrypt_info *ci, struct inode *inode,
-const char **cipher_str_ret, int *keysize_ret)
+static struct fscrypt_mode *
+select_encryption_mode(const struct fscrypt_info *ci, const struct inode 
*inode)
 {
-   u32 mode;
-
if (!fscrypt_valid_enc_modes(ci->ci_data_mode, ci->ci_filename_mode)) {
fscrypt_warn(inode->i_sb,
 "inode %lu uses unsupported encryption modes 
(contents mode %d, filenames mode %d)",
 inode->i_ino, ci->ci_data_mode,
 ci->ci_filename_mode);
-   return -EINVAL;
+   return ERR_PTR(-EINVAL);
}
 
-   if (S_ISREG(inode->i_mode)) {
-   mode = ci->ci_data_mode;
-   } else if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) {
-   mode = ci->ci_filename_mode;
-   } else {
-   WARN_ONCE(1, "fscrypt: filesystem tried to load encryption info 
for inode %lu, which is not encryptable (file type %d)\n",
- inode->i_ino, (inode->i_mode & S_IFMT));
-   return -EINVAL;
-   }
+   if (S_ISREG(inode->i_mode))
+   return _modes[ci->ci_data_mode];
+
+   if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode))
+   return _modes[ci->ci_filename_mode];
 
-   *cipher_str_ret = available_modes[mode].cipher_str;
-   *keysize_ret = available_modes[mode].keysize;
-   return 0;
+   WARN_ONCE(1, "fscrypt: filesystem tried to load encryption info 

Re: [PATCH 1/5] random: fix crng_ready() test

[Theodore Y. Ts'o]

On Thu, May 17, 2018 at 08:01:04AM +0200, Christophe LEROY wrote:
> 
> On a powerpc embedded board which has an mpc8xx processor running at 133Mhz,
> I now get the startup done in more than 7 minutes instead of 30 seconds.
> This is due to the webserver blocking on read on /dev/random until we get
> 'random: crng init done':
> 
> [0.00] Linux version 4.17.0-rc4-00415-gd2f75d40072d (root@localhost)
> (gcc version 5.4.0 (GCC)) #203 PREEMPT Wed May 16 16:32:02 CEST 2018
> [0.295453] random: get_random_u32 called from
> bucket_table_alloc+0x84/0x1bc with crng_init=0
> [1.030472] device: 'random': device_add
> [1.031279] device: 'urandom': device_add
> [1.420069] device: 'hw_random': device_add
> [2.156853] random: fast init done
> [  462.007776] random: crng init done
> 
> This has become really critical, is there anything that can be done ?

Figure out why the webserver needs to read /dev/random and is it for a
security critical purpose?

A kernel patch which makes the kernel do a "lalalalala I'm secure"
when it really isn't secure is a simple "solution", but would it
really make you happy?


- Ted

Re: [PATCH 1/5] random: fix crng_ready() test

[Theodore Y. Ts'o]

On Wed, May 16, 2018 at 05:07:08PM -0700, Srivatsa S. Bhat wrote:
> 
> On a Photon OS VM running on VMware ESXi, this patch causes a boot speed
> regression of 5 minutes :-( [ The VM doesn't have haveged or rng-tools
> (rngd) installed. ]
> 
> [1.420246] EXT4-fs (sda2): re-mounted. Opts: barrier,noacl,data=ordered
> [1.469722] tsc: Refined TSC clocksource calibration: 1900.002 MHz
> [1.470707] clocksource: tsc: mask: 0x max_cycles: 
> 0x36c65c1a9e1, max_idle_ns: 881590695311 ns
> [1.474249] clocksource: Switched to clocksource tsc
> [1.584427] systemd-journald[216]: Received request to flush runtime 
> journal from PID 1
> [  346.620718] random: crng init done
> 
> Interestingly, the boot delay is exacerbated on VMs with large amounts
> of RAM. For example, the delay is not so noticeable (< 30 seconds) on a
> VM with 2GB memory, but goes up to 5 minutes on an 8GB VM.
> 
> Also, cloud-init-local.service seems to be the one blocking for entropy
> here.

So the first thing I'd ask you to investigate is what the heck
cloud-init-local.service is doing, and why it really needs
cryptographic grade random numbers?

> It would be great if this CVE can be fixed somehow without causing boot speed
> to spuike from ~20 seconds to 5 minutes, as that makes the system pretty much
> unusable. I can workaround this by installing haveged, but ideally an 
> in-kernel
> fix would be better. If you need any other info about my setup or if you have
> a patch that I can test, please let me know!

So the question is why is strong random numbers needed by
cloud-init-local, and how much do you trust either haveged and/or
RDRAND (which is what you will be depending upon if you install
rng-tools).  If you believe that Intel and/or the NSA hasn't
backdoored RDRAND[1], or you believe that because Intel processor's
internal cache architecture isn't publically documented, and it's
S complicated that no one can figure it out (which is what you
will be depending upon if you if you choose haveged), be my guest.  I
personally consider the latter to be "security via obscu7rity", but
others have different opinions.

[1] As an aside, read the best paper from the 37th IEEE Symposium on
Security and Privacy and weep:

https://www.computerworld.com/article/3079417/security/researchers-built-devious-undetectable-hardware-level-backdoor-in-computer-chips.html

If it turns out that the security if your VM is critically dependent
on what cloud-init-local.service is doing, and you don't like making
those assumptions, then you may need to ask VMWare ESXi to make
virtio-rng available to guest OS's, and then make Photon OS depend on
a secure RNG available to the host OS.

Best regards,

- Ted

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Geert Uytterhoeven]

Hi Gilad,

On Thu, May 17, 2018 at 3:41 PM, Gilad Ben-Yossef  wrote:
> On Thu, May 17, 2018 at 4:35 PM, Geert Uytterhoeven
>  wrote:
>> On Thu, May 17, 2018 at 3:09 PM, Gilad Ben-Yossef  
>> wrote:
>>> On Thu, May 17, 2018 at 1:16 PM, Geert Uytterhoeven
>>>  wrote:
 However, even with your clock patch, the signature checking fails for me,
 on both R-Car H3 ES1.0 and ES2.0.
 Does this need changes to the ARM Trusted Firmware, to allow Linux to
 access the public SCEG module?
>>>
>>> Well, this is actually something different. If you look you will
>>> notice that my patch was part of a 3 part patch series,
>>> the first of which disabled this test.
>>
>> Sorry, I had completely forgotten about the first patch from the series.
>> With that applied, it continues:
>>
>> ccree e6601000.crypto: ARM CryptoCell 630P Driver: HW version
>> 0x, Driver version 4.0
>> ccree e6601000.crypto: Cache params previous: 0x0777
>> ccree e6601000.crypto: Cache params current: 0x
>> (expect: 0x)
>> alg: No test for cts1(cbc(aes)) (cts1-cbc-aes-ccree)
>> alg: No test for authenc(xcbc(aes),cbc(aes))
>> (authenc-xcbc-aes-cbc-aes-ccree)
>> alg: No test for authenc(xcbc(aes),rfc3686(ctr(aes)))
>> (authenc-xcbc-aes-rfc3686-ctr-aes-ccree)
>> ccree e6601000.crypto: ARM ccree device initialized
>>
>> Is HW version 0x expected?
>
> It's related to the problem with reading the wrong register I've
> mentioned before.

OK.

>>> If you take all the 3 patches, it will work.
>>
>> is there an easy way to test proper operation?
>
> The lines of the form "  alg: No test for cts1(cbc(aes))
> (cts1-cbc-aes-ccree)" indicates
> you have the Crypto API testmgr enable (or rather not disabled would
> be more accurate) so every
> cryptographic algorithm except those specified in these messages was
> tested with test
> vectors from crypto/testmgr.c upon registration. If you don't seen
> failure warnings, it works.

OK.

> You can also check /proc/crypto for all the algorithm with ccree
> listed as their driver and check
> that their test passed.

OK, in that case everything works as expected, on both R-Car H3 ES1.0 and
ES2.0.

Thanks!

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Gilad Ben-Yossef]

On Thu, May 17, 2018 at 4:35 PM, Geert Uytterhoeven
 wrote:
> Hi Gilad,
>
> On Thu, May 17, 2018 at 3:09 PM, Gilad Ben-Yossef  wrote:
>> On Thu, May 17, 2018 at 1:16 PM, Geert Uytterhoeven
>>  wrote:
>>> However, even with your clock patch, the signature checking fails for me,
>>> on both R-Car H3 ES1.0 and ES2.0.
>>> Does this need changes to the ARM Trusted Firmware, to allow Linux to
>>> access the public SCEG module?
>>
>> Well, this is actually something different. If you look you will
>> notice that my patch was part of a 3 part patch series,
>> the first of which disabled this test.
>
> Sorry, I had completely forgotten about the first patch from the series.
> With that applied, it continues:
>
> ccree e6601000.crypto: ARM CryptoCell 630P Driver: HW version
> 0x, Driver version 4.0
> ccree e6601000.crypto: Cache params previous: 0x0777
> ccree e6601000.crypto: Cache params current: 0x
> (expect: 0x)
> alg: No test for cts1(cbc(aes)) (cts1-cbc-aes-ccree)
> alg: No test for authenc(xcbc(aes),cbc(aes))
> (authenc-xcbc-aes-cbc-aes-ccree)
> alg: No test for authenc(xcbc(aes),rfc3686(ctr(aes)))
> (authenc-xcbc-aes-rfc3686-ctr-aes-ccree)
> ccree e6601000.crypto: ARM ccree device initialized
>
> Is HW version 0x expected?

It's related to the problem with reading the wrong register I've
mentioned before.

>
>> If you take all the 3 patches, it will work.
>
> is there an easy way to test proper operation?

The lines of the form "  alg: No test for cts1(cbc(aes))
(cts1-cbc-aes-ccree)" indicates
you have the Crypto API testmgr enable (or rather not disabled would
be more accurate) so every
cryptographic algorithm except those specified in these messages was
tested with test
vectors from crypto/testmgr.c upon registration. If you don't seen
failure warnings, it works.

You can also check /proc/crypto for all the algorithm with ccree
listed as their driver and check
that their test passed.


> I enabled CONFIG_CRYPT_TEST, and did insmod tcrypt.ko, but I mostly see
> "Failed to load transform" messages.
>

tcrypt.ko is a rather crude developer tool. It has hard coded lists of
test for different algorithms that does
not take into account if some crypto algs are enagled in the build or
not. It's more of a stress test.


Gilad


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Geert Uytterhoeven]

Hi Gilad,

On Thu, May 17, 2018 at 3:09 PM, Gilad Ben-Yossef  wrote:
> On Thu, May 17, 2018 at 1:16 PM, Geert Uytterhoeven
>  wrote:
>> However, even with your clock patch, the signature checking fails for me,
>> on both R-Car H3 ES1.0 and ES2.0.
>> Does this need changes to the ARM Trusted Firmware, to allow Linux to
>> access the public SCEG module?
>
> Well, this is actually something different. If you look you will
> notice that my patch was part of a 3 part patch series,
> the first of which disabled this test.

Sorry, I had completely forgotten about the first patch from the series.
With that applied, it continues:

ccree e6601000.crypto: ARM CryptoCell 630P Driver: HW version
0x, Driver version 4.0
ccree e6601000.crypto: Cache params previous: 0x0777
ccree e6601000.crypto: Cache params current: 0x
(expect: 0x)
alg: No test for cts1(cbc(aes)) (cts1-cbc-aes-ccree)
alg: No test for authenc(xcbc(aes),cbc(aes))
(authenc-xcbc-aes-cbc-aes-ccree)
alg: No test for authenc(xcbc(aes),rfc3686(ctr(aes)))
(authenc-xcbc-aes-rfc3686-ctr-aes-ccree)
ccree e6601000.crypto: ARM ccree device initialized

Is HW version 0x expected?

> If you take all the 3 patches, it will work.

is there an easy way to test proper operation?
I enabled CONFIG_CRYPT_TEST, and did insmod tcrypt.ko, but I mostly see
"Failed to load transform" messages.

Thanks!

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

[PATCH] crypto: inside-secure - do not use memset on MMIO

[Antoine Tenart]

This patch fixes the Inside Secure driver which uses a memtset() call to
set an MMIO area from the cryptographic engine to 0. This is wrong as
memset() isn't guaranteed to work on MMIO for many reasons. This led to
kernel paging request panics in certain cases. Use memset_io() instead.

Fixes: 1b44c5a60c13 ("crypto: inside-secure - add SafeXcel EIP197 crypto engine 
driver")
Reported-by: Ofer Heifetz 
Signed-off-by: Antoine Tenart 
---
 drivers/crypto/inside-secure/safexcel.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/inside-secure/safexcel.c 
b/drivers/crypto/inside-secure/safexcel.c
index 46ab2d0eb3fd..4e86f864a952 100644
--- a/drivers/crypto/inside-secure/safexcel.c
+++ b/drivers/crypto/inside-secure/safexcel.c
@@ -153,8 +153,8 @@ static int eip197_load_firmwares(struct 
safexcel_crypto_priv *priv)
   EIP197_PE_ICE_SCRATCH_CTRL_CHANGE_ACCESS;
writel(val, EIP197_PE(priv) + EIP197_PE_ICE_SCRATCH_CTRL);
 
-   memset(EIP197_PE(priv) + EIP197_PE_ICE_SCRATCH_RAM, 0,
-  EIP197_NUM_OF_SCRATCH_BLOCKS * sizeof(u32));
+   memset_io(EIP197_PE(priv) + EIP197_PE_ICE_SCRATCH_RAM, 0,
+ EIP197_NUM_OF_SCRATCH_BLOCKS * sizeof(u32));
 
eip197_write_firmware(priv, fw[FW_IFPP], EIP197_PE_ICE_FPP_CTRL,
  EIP197_PE_ICE_RAM_CTRL_FPP_PROG_EN);
-- 
2.17.0

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Geert Uytterhoeven]

On Thu, May 17, 2018 at 12:16 PM, Geert Uytterhoeven
 wrote:
> On Thu, May 17, 2018 at 10:01 AM, Gilad Ben-Yossef  
> wrote:
>> On Wed, May 16, 2018 at 10:43 AM, Simon Horman  wrote:
>>> On Tue, May 15, 2018 at 04:50:44PM +0200, Geert Uytterhoeven wrote:
 On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef  
 wrote:
 > Add bindings for CryptoCell instance in the SoC.
 >
 > Signed-off-by: Gilad Ben-Yossef 

 Thanks for your patch!

 > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
 > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
 > @@ -528,6 +528,14 @@
 > status = "disabled";
 > };
 >
 > +   arm_cc630p: crypto@e6601000 {
 > +   compatible = "arm,cryptocell-630p-ree";
 > +   interrupts = ;
 > +   #interrupt-cells = <2>;

 I believe the #interrupt-cells property is not needed.

 > +   reg = <0x0 0xe6601000 0 0x1000>;
 > +   clocks = < CPG_MOD 229>;
>
> Missing "power-domains = < R8A7795_PD_ALWAYS_ON>;", as
> the Secure Engine is part of the CPG/MSSR clock domain (see below [*]).

And missing "resets = < 229>;", as the module is tied to the CPG/MSSR
reset controller.

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Gilad Ben-Yossef]

On Thu, May 17, 2018 at 12:04 PM, Simon Horman  wrote:
> On Thu, May 17, 2018 at 11:01:57AM +0300, Gilad Ben-Yossef wrote:
>> On Wed, May 16, 2018 at 10:43 AM, Simon Horman  wrote:
>> > On Tue, May 15, 2018 at 04:50:44PM +0200, Geert Uytterhoeven wrote:
>> >> Hi Gilad,
>> >>
>> >> On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef  
>> >> wrote:
>> >> > Add bindings for CryptoCell instance in the SoC.
>> >> >
>> >> > Signed-off-by: Gilad Ben-Yossef 
>> >>
>> >> Thanks for your patch!
>> >>
>> >> > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>> >> > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>> >> > @@ -528,6 +528,14 @@
>> >> > status = "disabled";
>> >> > };
>> >> >
>> >> > +   arm_cc630p: crypto@e6601000 {
>> >> > +   compatible = "arm,cryptocell-630p-ree";
>> >> > +   interrupts = ;
>> >> > +   #interrupt-cells = <2>;
>> >>
>> >> I believe the #interrupt-cells property is not needed.
>> >>
>> >> > +   reg = <0x0 0xe6601000 0 0x1000>;
>> >> > +   clocks = < CPG_MOD 229>;
>> >> > +   };
>> >>
>> >> The rest looks good, but I cannot verify the register block.
>> >>
>> >> > +
>> >> > i2c3: i2c@e66d {
>> >> > #address-cells = <1>;
>> >> > #size-cells = <0>;
>> >
>> > Thanks, I have applied this after dropping the #interrupt-cells property.
>>
>> Thanks you!
>>
>> Alas, it will not work without the clk patch (the previous one in the
>> series) so they need to be
>> taken or dropped together.
>
> I think its fine if it does not yet work.
> But not if its causes things that previously worked to stop working.

Based on the further discussion with Geert my recommendation is to
drop my patch for now,
take Geert CR clock  patch and I will follow up next week with a v2
that fixes the clock
handing as discussed with Geert.

Many thanks,
Gilad



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Gilad Ben-Yossef]

On Thu, May 17, 2018 at 1:16 PM, Geert Uytterhoeven
 wrote:
> Hi Gilad,
>
> On Thu, May 17, 2018 at 10:01 AM, Gilad Ben-Yossef  
> wrote:
>> On Wed, May 16, 2018 at 10:43 AM, Simon Horman  wrote:
>>> On Tue, May 15, 2018 at 04:50:44PM +0200, Geert Uytterhoeven wrote:
 On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef  
 wrote:
 > Add bindings for CryptoCell instance in the SoC.
 >
 > Signed-off-by: Gilad Ben-Yossef 

 Thanks for your patch!

 > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
 > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
 > @@ -528,6 +528,14 @@
 > status = "disabled";
 > };
 >
 > +   arm_cc630p: crypto@e6601000 {
 > +   compatible = "arm,cryptocell-630p-ree";
 > +   interrupts = ;
 > +   #interrupt-cells = <2>;

 I believe the #interrupt-cells property is not needed.

 > +   reg = <0x0 0xe6601000 0 0x1000>;
 > +   clocks = < CPG_MOD 229>;
>
> Missing "power-domains = < R8A7795_PD_ALWAYS_ON>;", as
> the Secure Engine is part of the CPG/MSSR clock domain (see below [*]).

Thank you. I didn't get this information from Renesas :-)

>
 > +   };

 The rest looks good, but I cannot verify the register block.

 > +
 > i2c3: i2c@e66d {
 > #address-cells = <1>;
 > #size-cells = <0>;
>>>
>>> Thanks, I have applied this after dropping the #interrupt-cells property.
>>
>> Thanks you!
>>
>> Alas, it will not work without the clk patch (the previous one in the
>> series) so they need to be
>> taken or dropped together.
>
> Indeed. From a quick glance, it looks like drivers/crypto/ccree/cc_driver.c
> does not distinguish between the absence of the clock property, and an
> actual error in getting the clock, and never considers any error a failure
> (incl. -PROBE_DEFER).
>
> As of_clk_get() returns -ENOENT for both a missing clock property and a
> missing clock, you should use (devm_)clk_get() instead, and distinguish
> between NULL (no clock property) and IS_ERR() (actual failure -> abort).
>

Thank you, this is very valuable. I will do as you suggested.


> Hence in the absence of the clock patch, the driver accesses the crypto
> engine while its module clock is turned off, leading to:
>
> ccree e6601000.crypto: Invalid CC signature: SIGNATURE=0x
> != expected=0xDCC63000
>
> You must be lucky, though, usually you get an imprecise external abort
> later, crashing the whole system ;-)
>
> So I think this patch should be dropped for now.
>
> However, even with your clock patch, the signature checking fails for me,
> on both R-Car H3 ES1.0 and ES2.0.
> Does this need changes to the ARM Trusted Firmware, to allow Linux to
> access the public SCEG module?

Well, this is actually something different. If you look you will
notice that my patch was part of a 3 part patch series,
the first of which disabled this test.

If you take all the 3 patches, it will work.

To make things more interesting, I have since sending the patch
learned WHY the test does not work, so disabling
it is not needed - to make a long story short, I was reading the wrong
register that just happens to have the right value
in our FPGA based tests systems but does not in the real silicon
implementations.

But you are right - if the clock is not enabled and you are try to
read from the register the system does freeze.

I will send a fixed v2. based on your patch enabling the CR clock.

>
> [*] More on the subject of clock control:
> At least for Renesas SoCs, where the module is part of a clock domain, and
> can be controlled automatically by Runtime PM, you could drop the explicit
> clock control, and use Runtime PM instead
> (pm_runtime_{enable,get_sync,put,disable}()).  That would allow the driver
> to work on systems with any kind of PM Domains, too.
> Depending on the other platforms that include a CryptoCell and their
> (non)reliance on PM Domains, you may have to keep the explicit clock
> handling, in addition to Runtime PM.
>



> To decrease power consumption, I suggest to move the clock and/or Runtime
> PM handling to the routines that actually use the hardware, instead of
> powering the module in the probe routine.
>

This is very interesting and I will give it a try.

Thanks again!
Gilad



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH 1/3] crypto: ccree: drop signature register check

[Gilad Ben-Yossef]

Herbert,

On Tue, May 15, 2018 at 3:29 PM, Gilad Ben-Yossef  wrote:
> We were using the content of the signature register as a sanity
> check for the hardware functioning but it turns out not all
> implementers use the same values so the check is giving false
> negative on certain SoCs and so we drop it.
>

Please drop this patch. I have found a better fix and will send a v2 soon.

Thanks,
Gilad


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Geert Uytterhoeven]

Hi Gilad,

On Thu, May 17, 2018 at 10:01 AM, Gilad Ben-Yossef  wrote:
> On Wed, May 16, 2018 at 10:43 AM, Simon Horman  wrote:
>> On Tue, May 15, 2018 at 04:50:44PM +0200, Geert Uytterhoeven wrote:
>>> On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef  
>>> wrote:
>>> > Add bindings for CryptoCell instance in the SoC.
>>> >
>>> > Signed-off-by: Gilad Ben-Yossef 
>>>
>>> Thanks for your patch!
>>>
>>> > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>>> > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>>> > @@ -528,6 +528,14 @@
>>> > status = "disabled";
>>> > };
>>> >
>>> > +   arm_cc630p: crypto@e6601000 {
>>> > +   compatible = "arm,cryptocell-630p-ree";
>>> > +   interrupts = ;
>>> > +   #interrupt-cells = <2>;
>>>
>>> I believe the #interrupt-cells property is not needed.
>>>
>>> > +   reg = <0x0 0xe6601000 0 0x1000>;
>>> > +   clocks = < CPG_MOD 229>;

Missing "power-domains = < R8A7795_PD_ALWAYS_ON>;", as
the Secure Engine is part of the CPG/MSSR clock domain (see below [*]).

>>> > +   };
>>>
>>> The rest looks good, but I cannot verify the register block.
>>>
>>> > +
>>> > i2c3: i2c@e66d {
>>> > #address-cells = <1>;
>>> > #size-cells = <0>;
>>
>> Thanks, I have applied this after dropping the #interrupt-cells property.
>
> Thanks you!
>
> Alas, it will not work without the clk patch (the previous one in the
> series) so they need to be
> taken or dropped together.

Indeed. From a quick glance, it looks like drivers/crypto/ccree/cc_driver.c
does not distinguish between the absence of the clock property, and an
actual error in getting the clock, and never considers any error a failure
(incl. -PROBE_DEFER).

As of_clk_get() returns -ENOENT for both a missing clock property and a
missing clock, you should use (devm_)clk_get() instead, and distinguish
between NULL (no clock property) and IS_ERR() (actual failure -> abort).

Hence in the absence of the clock patch, the driver accesses the crypto
engine while its module clock is turned off, leading to:

ccree e6601000.crypto: Invalid CC signature: SIGNATURE=0x
!= expected=0xDCC63000

You must be lucky, though, usually you get an imprecise external abort
later, crashing the whole system ;-)

So I think this patch should be dropped for now.

However, even with your clock patch, the signature checking fails for me,
on both R-Car H3 ES1.0 and ES2.0.
Does this need changes to the ARM Trusted Firmware, to allow Linux to
access the public SCEG module?

[*] More on the subject of clock control:
At least for Renesas SoCs, where the module is part of a clock domain, and
can be controlled automatically by Runtime PM, you could drop the explicit
clock control, and use Runtime PM instead
(pm_runtime_{enable,get_sync,put,disable}()).  That would allow the driver
to work on systems with any kind of PM Domains, too.
Depending on the other platforms that include a CryptoCell and their
(non)reliance on PM Domains, you may have to keep the explicit clock
handling, in addition to Runtime PM.

To decrease power consumption, I suggest to move the clock and/or Runtime
PM handling to the routines that actually use the hardware, instead of
powering the module in the probe routine.

Thanks!

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Simon Horman]

On Thu, May 17, 2018 at 11:01:57AM +0300, Gilad Ben-Yossef wrote:
> On Wed, May 16, 2018 at 10:43 AM, Simon Horman  wrote:
> > On Tue, May 15, 2018 at 04:50:44PM +0200, Geert Uytterhoeven wrote:
> >> Hi Gilad,
> >>
> >> On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef  
> >> wrote:
> >> > Add bindings for CryptoCell instance in the SoC.
> >> >
> >> > Signed-off-by: Gilad Ben-Yossef 
> >>
> >> Thanks for your patch!
> >>
> >> > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> >> > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> >> > @@ -528,6 +528,14 @@
> >> > status = "disabled";
> >> > };
> >> >
> >> > +   arm_cc630p: crypto@e6601000 {
> >> > +   compatible = "arm,cryptocell-630p-ree";
> >> > +   interrupts = ;
> >> > +   #interrupt-cells = <2>;
> >>
> >> I believe the #interrupt-cells property is not needed.
> >>
> >> > +   reg = <0x0 0xe6601000 0 0x1000>;
> >> > +   clocks = < CPG_MOD 229>;
> >> > +   };
> >>
> >> The rest looks good, but I cannot verify the register block.
> >>
> >> > +
> >> > i2c3: i2c@e66d {
> >> > #address-cells = <1>;
> >> > #size-cells = <0>;
> >
> > Thanks, I have applied this after dropping the #interrupt-cells property.
> 
> Thanks you!
> 
> Alas, it will not work without the clk patch (the previous one in the
> series) so they need to be
> taken or dropped together.

I think its fine if it does not yet work.
But not if its causes things that previously worked to stop working.

Re: [PATCH 2/3] clk: renesas: r8a7795: Add ccree clock

[Geert Uytterhoeven]

Hi Gilad,

On Thu, May 17, 2018 at 10:00 AM, Gilad Ben-Yossef  wrote:
> On Tue, May 15, 2018 at 5:47 PM, Geert Uytterhoeven
>  wrote:
>> On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef  
>> wrote:
>>> This patch adds the clock used by the CryptoCell 630p instance in the SoC.
>>>
>>> Signed-off-by: Gilad Ben-Yossef 
>>
>> Thanks for your patch!
>>
>>> --- a/drivers/clk/renesas/r8a7795-cpg-mssr.c
>>> +++ b/drivers/clk/renesas/r8a7795-cpg-mssr.c
>>> @@ -132,6 +132,7 @@ static struct mssr_mod_clk r8a7795_mod_clks[] 
>>> __initdata = {
>>> DEF_MOD("sys-dmac2", 217,   R8A7795_CLK_S0D3),
>>> DEF_MOD("sys-dmac1", 218,   R8A7795_CLK_S0D3),
>>> DEF_MOD("sys-dmac0", 219,   R8A7795_CLK_S0D3),
>>> +   DEF_MOD("ccree", 229,   R8A7795_CLK_S3D2),
>>
>> I don't know if "ccree" is the proper name for this clock, as there
>> may be multiple
>> instances.
>
> I'd be happy to rename it to anything else. Suggestions?

I believe it should be called "sceg-pub".

>> I also can't verify the parent clock.
>
> I'm afraid I can't really help. This is based on code snippet from
> Renesas. I verified it works but
> I am not an expert on the clock settings :-(

As your driver doesn't care about the clock rate, only about
enabling/disabling the clock, the actual parent doesn't matter much.

After some deeper diving into the datasheet, I believe the correct parent is
the CR clock, which is unfortunately not yet supported by the R-Car H3 clock
driver. I'll send a patch...

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH] crypto: reorder paes test lexicographically

[Corentin Labbe]

On Fri, May 11, 2018 at 09:04:06AM +0100, Gilad Ben-Yossef wrote:
> Due to a snafu "paes" testmgr tests were not ordered
> lexicographically, which led to boot time warnings.
> Reorder the tests as needed.
> 
> Fixes: a794d8d ("crypto: ccree - enable support for hardware keys")
> Reported-by: Abdul Haleem 
> Signed-off-by: Gilad Ben-Yossef 

Tested-by: Corentin Labbe 

Thanks

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

[Gilad Ben-Yossef]

On Wed, May 16, 2018 at 10:43 AM, Simon Horman  wrote:
> On Tue, May 15, 2018 at 04:50:44PM +0200, Geert Uytterhoeven wrote:
>> Hi Gilad,
>>
>> On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef  
>> wrote:
>> > Add bindings for CryptoCell instance in the SoC.
>> >
>> > Signed-off-by: Gilad Ben-Yossef 
>>
>> Thanks for your patch!
>>
>> > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>> > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>> > @@ -528,6 +528,14 @@
>> > status = "disabled";
>> > };
>> >
>> > +   arm_cc630p: crypto@e6601000 {
>> > +   compatible = "arm,cryptocell-630p-ree";
>> > +   interrupts = ;
>> > +   #interrupt-cells = <2>;
>>
>> I believe the #interrupt-cells property is not needed.
>>
>> > +   reg = <0x0 0xe6601000 0 0x1000>;
>> > +   clocks = < CPG_MOD 229>;
>> > +   };
>>
>> The rest looks good, but I cannot verify the register block.
>>
>> > +
>> > i2c3: i2c@e66d {
>> > #address-cells = <1>;
>> > #size-cells = <0>;
>
> Thanks, I have applied this after dropping the #interrupt-cells property.

Thanks you!

Alas, it will not work without the clk patch (the previous one in the
series) so they need to be
taken or dropped together.

Gilad



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH 2/3] clk: renesas: r8a7795: Add ccree clock

[Gilad Ben-Yossef]

On Tue, May 15, 2018 at 5:47 PM, Geert Uytterhoeven
 wrote:
> Hi Gilad,
>
> On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef  wrote:
>> This patch adds the clock used by the CryptoCell 630p instance in the SoC.
>>
>> Signed-off-by: Gilad Ben-Yossef 
>
> Thanks for your patch!
>
>> --- a/drivers/clk/renesas/r8a7795-cpg-mssr.c
>> +++ b/drivers/clk/renesas/r8a7795-cpg-mssr.c
>> @@ -132,6 +132,7 @@ static struct mssr_mod_clk r8a7795_mod_clks[] __initdata 
>> = {
>> DEF_MOD("sys-dmac2", 217,   R8A7795_CLK_S0D3),
>> DEF_MOD("sys-dmac1", 218,   R8A7795_CLK_S0D3),
>> DEF_MOD("sys-dmac0", 219,   R8A7795_CLK_S0D3),
>> +   DEF_MOD("ccree", 229,   R8A7795_CLK_S3D2),
>
> I don't know if "ccree" is the proper name for this clock, as there
> may be multiple
> instances.

I'd be happy to rename it to anything else. Suggestions?

> I also can't verify the parent clock.

I'm afraid I can't really help. This is based on code snippet from
Renesas. I verified it works but
I am not an expert on the clock settings :-(

>
>> DEF_MOD("cmt3",  300,   R8A7795_CLK_R),
>> DEF_MOD("cmt2",  301,   R8A7795_CLK_R),
>> DEF_MOD("cmt1",  302,   R8A7795_CLK_R),
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH 1/5] random: fix crng_ready() test

[Christophe LEROY]

Le 13/04/2018 à 19:00, Theodore Y. Ts'o a écrit :

On Fri, Apr 13, 2018 at 03:05:01PM +0200, Stephan Mueller wrote:


What I would like to point out that more and more folks change to
getrandom(2). As this call will now unblock much later in the boot cycle,
these systems see a significant departure from the current system behavior.

E.g. an sshd using getrandom(2) would be ready shortly after the boot finishes
as of now. Now it can be a matter minutes before it responds. Thus, is such
change in the kernel behavior something for stable?


It will have some change on the kernel behavior, but not as much as
you might think.  That's because in older kernels, we were *already*
blocking until crng_init > 2 --- if the getrandom(2) call happened
while crng_init was in state 0.

Even before this patch series, we didn't wake up a process blocked on
crng_init_wait until crng_init state 2 is reached:

static void crng_reseed(struct crng_state *crng, struct entropy_store *r)
{
...
if (crng == _crng && crng_init < 2) {
invalidate_batched_entropy();
crng_init = 2;
process_random_ready_list();
wake_up_interruptible(_init_wait);
pr_notice("random: crng init done\n");
}
}

This is the reason why there are reports like this: "Boot delayed for
about 90 seconds until 'random: crng init done'"[1]

[1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1685794


So we have the problem already.  There will be more cases of this
after this patch series is applied, true.  But what we have already is
an inconsistent state where if you call getrandom(2) while the kernel
is in crng_init state 0, you will block until crng_init state 2, but
if you are in crng_init state 1, you will assume the CRNG is fully
initialized.

Given the documentation of how getrandom(2) works what its documented
guarantees are, I think it does justify making its behavior both more
consistent with itself, and more consistent what the security
guarantees we have promised people.

I was a little worried that on VM's this could end up causing things
to block for a long time, but an experiment on a GCE VM shows that
isn't a problem:

[0.00] Linux version 4.16.0-rc3-ext4-9-gf6b302ebca85 (tytso@cwcc) 
(gcc version 7.3.0 (Debian 7.3.0-15)) #16 SMP Thu Apr 12 16:57:17 EDT 2018
[1.282220] random: fast init done
[3.987092] random: crng init done
[4.376787] EXT4-fs (sda1): re-mounted. Opts: (null)

There are some desktops where the "crng_init done" report doesn't
happen until 45-90 seconds into the boot.  I don't think I've seen
reports where it takes _minutes_ however.  Can you give me some
examples of such cases?



On a powerpc embedded board which has an mpc8xx processor running at 
133Mhz, I now get the startup done in more than 7 minutes instead of 30 
seconds. This is due to the webserver blocking on read on /dev/random 
until we get 'random: crng init done':


[0.00] Linux version 4.17.0-rc4-00415-gd2f75d40072d 
(root@localhost) (gcc version 5.4.0 (GCC)) #203 PREEMPT Wed May 16 
16:32:02 CEST 2018
[0.295453] random: get_random_u32 called from 
bucket_table_alloc+0x84/0x1bc with crng_init=0

[1.030472] device: 'random': device_add
[1.031279] device: 'urandom': device_add
[1.420069] device: 'hw_random': device_add
[2.156853] random: fast init done
[  462.007776] random: crng init done

This has become really critical, is there anything that can be done ?

Christophe



- Ted

P.S.  Of course, in a VM environment, if the host supports virtio-rng,
the boot delay problem is completely not an issue.  You just have to
enable virtio-rng in the guest kernel, which I believe is already the
case for most distro kernels.

BTW, for KVM, it's fairly simple to set it the host-side support for
virtio-rng.  Just add to the kvm command-line options:

 -object rng-random,filename=/dev/urandom,id=rng0 \
-device virtio-rng-pci,rng=rng0