Re: [PATCH] crypto: ccree: limit build to plausible archs

[Geert Uytterhoeven]

Hi Gilad,

On Mon, Apr 23, 2018 at 3:22 PM, Gilad Ben-Yossef  wrote:
> On Mon, Apr 23, 2018 at 3:13 PM, Geert Uytterhoeven
>  wrote:
>> On Mon, Apr 23, 2018 at 1:48 PM, Gilad Ben-Yossef  
>> wrote:
>>> Limit option to compile ccree to plausible architectures.
>>
>> Thanks for your patch!
>>
>>> Suggested-by: Geert Uytterhoeven 
>>> Signed-off-by: Gilad Ben-Yossef 
>>> ---
>>>  drivers/crypto/Kconfig | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
>>> index d1ea1a0..7302785 100644
>>> --- a/drivers/crypto/Kconfig
>>> +++ b/drivers/crypto/Kconfig
>>> @@ -726,6 +726,7 @@ config CRYPTO_DEV_ARTPEC6
>>>  config CRYPTO_DEV_CCREE
>>> tristate "Support for ARM TrustZone CryptoCell family of security 
>>> processors"
>>> depends on CRYPTO && CRYPTO_HW && OF && HAS_DMA
>>> +   depends on (XTENSA || X86 || UNICORE32 || SUPERH || RISCV || PPC32 
>>> || OPENRISC || NIOS2 || NDS32 || MIPS || MICROBLAZE || HEXAGON || H8300 || 
>>> ARM || ARM64 || ARC || COMPILE_TEST)
>>
>> That list looks a bit excessive to me...
>
> I'm sorry, but as an Arm employee I'm not in liberty to identify which
> customer licensed or might license CryptoCell for which platform, now
> or in the future.
>
> I'm sure you understand.

IC, a clever marketing scheme to make everyone think that everybody else
is already a licensee ;-)

What about using "depends on  || COMPILE_TEST", with  the
platforms for which the DTS (incl. "arm,cryptocell-*") will be submitted
for v4.18? The list can easily be extended when needed.

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: GCM and XTS: kcapi result not matching with NIST vectors

[Stephan Mueller]

Am Montag, 23. April 2018, 07:51:35 CEST schrieb Jitendra Lulla:

Hi Jitendra,

> 
> TEST 2:
> 
> Similarly for XTS also we have one mismatch:
> https://csrc.nist.gov/CSRC/media/Projects/Cryptographic-Algorithm-Validation
> -Program/documents/aes/XTSTestVectors.zip
> 
> /XTSTestVectors/format tweak value input - 128 hex str/XTSGenAES256.rsp
> 
> kcapi -x 1 -e -c "xts(aes)" -k
> 31c8152b5eddc3b8c3a005a3bbc4c005bb57058ae4a6454c166a620389eaecaea0515433574b
> 0dd6a89496acd475ef78dcf012a47a48c319f89e931404018e15 -p
> 31761b6dece3e962030c01f481c5ca681386176d2ef8034c5db5aa04b613ec00 -i
> 6957d297dc9c9b30f6d016b016d913c5
> 
> Result from tool :
> 1e16b5a44274f8791508cf3dec971aa975e16c702d66f11bc1f00ede540ef82c
> 
> NIST Expected Result :
> ae13222810bc66997bf8b57737990e481e16b5a44274f8791508cf3dec971a80

The mentioned test vector is decryption and not encryption as you applied it. 
Then, the data unit length in the test vector is 250 and not a multiple of 128 
which is the one to be used for XTS here.

Bottom line, the test vector is not applicable.

Ciao
Stephan

[PATCH 0/2] Fix stm32-rng for default state and suspend

[Lionel Debieve]

This series are fixing the default build state for stm32-rng that
activate the driver with arm multi_v7_defconfig.
Second patch is fixing the power suspend/resume behavior which was
not working.

Lionel Debieve (2):
  hwrng: stm32 - define default state for rng driver
  hwrng: stm32-rng: Fix pm_suspend issue

 drivers/char/hw_random/Kconfig | 1 +
 drivers/char/hw_random/stm32-rng.c | 9 +++--
 2 files changed, 8 insertions(+), 2 deletions(-)

-- 
2.15.1

[PATCH 1/2] hwrng: stm32 - define default state for rng driver

[Lionel Debieve]

Define default state for stm32_rng driver. It will
be default selected with multi_v7_defconfig

Signed-off-by: Lionel Debieve 
---
 drivers/char/hw_random/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index d53541e96bee..c34b257d852d 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -347,6 +347,7 @@ config HW_RANDOM_STM32
tristate "STMicroelectronics STM32 random number generator"
depends on HW_RANDOM && (ARCH_STM32 || COMPILE_TEST)
depends on HAS_IOMEM
+   default HW_RANDOM
help
  This driver provides kernel-side support for the Random Number
  Generator hardware found on STM32 microcontrollers.
-- 
2.15.1

[PATCH 2/2] hwrng: stm32-rng - fix pm_suspend issue

[Lionel Debieve]

When suspend is called after pm_runtime_suspend,
same callback is used and access to rng register is
freezing system. By calling the pm_runtime_force_suspend,
it first checks that runtime has been already done.

Signed-off-by: Lionel Debieve 
---
 drivers/char/hw_random/stm32-rng.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/char/hw_random/stm32-rng.c 
b/drivers/char/hw_random/stm32-rng.c
index 0d2328da3b76..042860d97b15 100644
--- a/drivers/char/hw_random/stm32-rng.c
+++ b/drivers/char/hw_random/stm32-rng.c
@@ -187,8 +187,13 @@ static int stm32_rng_runtime_resume(struct device *dev)
 }
 #endif
 
-static UNIVERSAL_DEV_PM_OPS(stm32_rng_pm_ops, stm32_rng_runtime_suspend,
-   stm32_rng_runtime_resume, NULL);
+static const struct dev_pm_ops stm32_rng_pm_ops = {
+   SET_RUNTIME_PM_OPS(stm32_rng_runtime_suspend,
+  stm32_rng_runtime_resume, NULL)
+   SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+   pm_runtime_force_resume)
+};
+
 
 static const struct of_device_id stm32_rng_match[] = {
{
-- 
2.15.1

Re: [PATCH] crypto: ccree: limit build to plausible archs

[Gilad Ben-Yossef]

On Mon, Apr 23, 2018 at 3:13 PM, Geert Uytterhoeven
 wrote:
> Hi Gilad,
>
> On Mon, Apr 23, 2018 at 1:48 PM, Gilad Ben-Yossef  wrote:
>> Limit option to compile ccree to plausible architectures.
>
> Thanks for your patch!
>
>> Suggested-by: Geert Uytterhoeven 
>> Signed-off-by: Gilad Ben-Yossef 
>> ---
>>  drivers/crypto/Kconfig | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
>> index d1ea1a0..7302785 100644
>> --- a/drivers/crypto/Kconfig
>> +++ b/drivers/crypto/Kconfig
>> @@ -726,6 +726,7 @@ config CRYPTO_DEV_ARTPEC6
>>  config CRYPTO_DEV_CCREE
>> tristate "Support for ARM TrustZone CryptoCell family of security 
>> processors"
>> depends on CRYPTO && CRYPTO_HW && OF && HAS_DMA
>> +   depends on (XTENSA || X86 || UNICORE32 || SUPERH || RISCV || PPC32 
>> || OPENRISC || NIOS2 || NDS32 || MIPS || MICROBLAZE || HEXAGON || H8300 || 
>> ARM || ARM64 || ARC || COMPILE_TEST)
>
> That list looks a bit excessive to me...

I'm sorry, but as an Arm employee I'm not in liberty to identify which
customer licensed or might license CryptoCell for which platform, now
or in the future.

I'm sure you understand.

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] crypto: ccree: limit build to plausible archs

[Geert Uytterhoeven]

Hi Gilad,

On Mon, Apr 23, 2018 at 1:48 PM, Gilad Ben-Yossef  wrote:
> Limit option to compile ccree to plausible architectures.

Thanks for your patch!

> Suggested-by: Geert Uytterhoeven 
> Signed-off-by: Gilad Ben-Yossef 
> ---
>  drivers/crypto/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index d1ea1a0..7302785 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -726,6 +726,7 @@ config CRYPTO_DEV_ARTPEC6
>  config CRYPTO_DEV_CCREE
> tristate "Support for ARM TrustZone CryptoCell family of security 
> processors"
> depends on CRYPTO && CRYPTO_HW && OF && HAS_DMA
> +   depends on (XTENSA || X86 || UNICORE32 || SUPERH || RISCV || PPC32 || 
> OPENRISC || NIOS2 || NDS32 || MIPS || MICROBLAZE || HEXAGON || H8300 || ARM 
> || ARM64 || ARC || COMPILE_TEST)

That list looks a bit excessive to me...

> default n
> select CRYPTO_HASH
> select CRYPTO_BLKCIPHER

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: ccree: limit build to plausible archs

[Gilad Ben-Yossef]

Limit option to compile ccree to plausible architectures.

Suggested-by: Geert Uytterhoeven 
Signed-off-by: Gilad Ben-Yossef 
---
 drivers/crypto/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index d1ea1a0..7302785 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -726,6 +726,7 @@ config CRYPTO_DEV_ARTPEC6
 config CRYPTO_DEV_CCREE
tristate "Support for ARM TrustZone CryptoCell family of security 
processors"
depends on CRYPTO && CRYPTO_HW && OF && HAS_DMA
+   depends on (XTENSA || X86 || UNICORE32 || SUPERH || RISCV || PPC32 || 
OPENRISC || NIOS2 || NDS32 || MIPS || MICROBLAZE || HEXAGON || H8300 || ARM || 
ARM64 || ARC || COMPILE_TEST)
default n
select CRYPTO_HASH
select CRYPTO_BLKCIPHER
-- 
2.7.4

Re: WARNING: kernel stack regs at (ptrval) in syzkaller has bad 'bp' value (ptrval)

[Dmitry Vyukov]

On Mon, Apr 23, 2018 at 12:10 PM, syzbot
 wrote:
> Hello,
>
> syzbot hit the following crash on upstream commit
> 5ec83b22a2dd13180762c89698e4e2c2881a423c (Sun Apr 22 19:13:04 2018 +)
> Merge tag '4.17-rc1-SMB3-CIFS' of git://git.samba.org/sfrench/cifs-2.6
> syzbot dashboard link:
> https://syzkaller.appspot.com/bug?extid=e073e4740cfbb3ae200b
>
> C reproducer: https://syzkaller.appspot.com/x/repro.c?id=6509307940044800
> syzkaller reproducer:
> https://syzkaller.appspot.com/x/repro.syz?id=6027105183727616
> Raw console output:
> https://syzkaller.appspot.com/x/log.txt?id=4763013630394368
> Kernel config:
> https://syzkaller.appspot.com/x/.config?id=1808800213120130118
> compiler: gcc (GCC) 8.0.1 20180413 (experimental)

There are 2 similar bugs reported:

https://syzkaller.appspot.com/bug?id=4fcc07e4e85fc6c1d5472fcc8ab5a35db01972dc
https://syzkaller.appspot.com/bug?id=bed7a7acabe08fe69a89a4225572911a32598651

They all seem to point to crypto/SHA3/K512_4.
+crypto maintainers

> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+e073e4740cfbb3ae2...@syzkaller.appspotmail.com
> It will help syzbot understand when the bug is fixed. See footer for
> details.
> If you forward the report, please keep this part and the footer.
>
> (ptrval): 00445649 (0x445649)
> (ptrval): 0033 (0x33)
> (ptrval): 0216 (0x216)
> (ptrval): 7f155536eda8 (0x7f155536eda8)
> (ptrval): 002b (0x2b)
> WARNING: kernel stack regs at (ptrval) in syzkaller547737:4493 has
> bad 'bp' value (ptrval)
> WARNING: kernel stack frame pointer at (ptrval) in
> syzkaller547737:4496 has bad value (ptrval)
>
>
> ---
> This bug is generated by a dumb bot. It may contain errors.
> See https://goo.gl/tpsmEJ for details.
> Direct all questions to syzkal...@googlegroups.com.
>
> syzbot will keep track of this bug report.
> If you forgot to add the Reported-by tag, once the fix for this bug is
> merged
> into any tree, please reply to this email with:
> #syz fix: exact-commit-title
> If you want to test a patch for this bug, please reply with:
> #syz test: git://repo/address.git branch
> and provide the patch inline or as an attachment.
> To mark this as a duplicate of another syzbot report, please reply with:
> #syz dup: exact-subject-of-another-report
> If it's a one-off invalid bug report, please reply with:
> #syz invalid
> Note: if the crash happens again, it will cause creation of a new bug
> report.
> Note: all commands must start from beginning of the line in the email body.
>
> --
> You received this message because you are subscribed to the Google Groups
> "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to syzkaller-bugs+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/syzkaller-bugs/af09f3056a813d2d%40google.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH/RFC] crypto: Add platform dependencies for CRYPTO_DEV_CCREE

[Geert Uytterhoeven]

Hi Gilad,

On Mon, Apr 23, 2018 at 9:45 AM, Gilad Ben-Yossef  wrote:
> On Wed, Apr 18, 2018 at 10:47 AM, Geert Uytterhoeven
>  wrote:
>> On Wed, Apr 18, 2018 at 6:32 AM, Gilad Ben-Yossef  
>> wrote:
>>> On Tue, Apr 17, 2018 at 9:14 PM, Geert Uytterhoeven
>>>  wrote:
 The ARM TrustZone CryptoCell is found on ARM SoCs only.  Hence make it
 depend on ARM or ARM64, unless compile-testing.
>>>
>>> Actually it is not. Despite what the name suggest, CryptoCell is
>>> designed by Arm but is
>>> not in fact limited to Arm cores. I think the only requirement is
>>> ability to provide an AMBA bus
>>> interface. Kudos to our marketing department to make that so clear and
>>> so on... :-)
>>
>> Good to know, I couldn't find any users of the compatible value in DT 
>> sources,
>> so I had to guess... and missed ;-)
>
> Yes, the original driver that went through staging was for CC 712,
> which so new it doesn't yet
> have a commercially available silicon yet :-)
>
> I've added the older 710 and 613 support just recently and will be
> working with CC hardware implementors
> to add the relevant DT bindings for their respective SoCs
>
>> Do you have a good suggestion for a platform dependency?
>> Based on the above, I'd say "depends on ARM_AMBA || COMPILE_TEST",
>> but (currently) ARM_AMBA is selected on ARM or ARM64 only?
>
> So AMBA *as a system bus* is not strictly needed AFAIK in the sense
> that you just need
> an AMBA to whatever bus interface, so not all system implementing this
> actually define ARM_AMBA.

IC.

> It's actually safer for me to rule out certain architectures rather
> than point to which are used.
> I'd say ruling out s390, um, alpha, ia64 and m68k is a safe bet.
>
> Do you want to send a patch or shall I?

Please send a patch, thanks!

(I don't want to be the one adding more negative architecture dependencies ;-)

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/RFC] crypto: Add platform dependencies for CRYPTO_DEV_CCREE

[Gilad Ben-Yossef]

On Wed, Apr 18, 2018 at 10:47 AM, Geert Uytterhoeven
 wrote:
> Hi Gilad,
>
> On Wed, Apr 18, 2018 at 6:32 AM, Gilad Ben-Yossef  wrote:
>> On Tue, Apr 17, 2018 at 9:14 PM, Geert Uytterhoeven
>>  wrote:
>>> The ARM TrustZone CryptoCell is found on ARM SoCs only.  Hence make it
>>> depend on ARM or ARM64, unless compile-testing.
>>
>> Actually it is not. Despite what the name suggest, CryptoCell is
>> designed by Arm but is
>> not in fact limited to Arm cores. I think the only requirement is
>> ability to provide an AMBA bus
>> interface. Kudos to our marketing department to make that so clear and
>> so on... :-)
>
> Good to know, I couldn't find any users of the compatible value in DT sources,
> so I had to guess... and missed ;-)

Yes, the original driver that went through staging was for CC 712,
which so new it doesn't yet
have a commercially available silicon yet :-)

I've added the older 710 and 613 support just recently and will be
working with CC hardware implementors
to add the relevant DT bindings for their respective SoCs

>
> Do you have a good suggestion for a platform dependency?
> Based on the above, I'd say "depends on ARM_AMBA || COMPILE_TEST",
> but (currently) ARM_AMBA is selected on ARM or ARM64 only?

So AMBA *as a system bus* is not strictly needed AFAIK in the sense
that you just need
an AMBA to whatever bus interface, so not all system implementing this
actually define ARM_AMBA.

It's actually safer for me to rule out certain architectures rather
than point to which are used.
I'd say ruling out s390, um, alpha, ia64 and m68k is a safe bet.

Do you want to send a patch or shall I?

Thanks,
Gilad


>
> 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: GCM and XTS: kcapi result not matching with NIST vectors

[Stephan Mueller]

Am Montag, 23. April 2018, 07:51:35 CEST schrieb Jitendra Lulla:

Hi Jitendra,

> Hi,
> 
> Consider the following 2 invocations from kcapi and the results we get
> from it. They are not matching with the NIST vectors [links pasted
> below].
> 
> Could somebody please tell why that could be happening?
> 
> thanks
> JItendra
> 
> https://csrc.nist.gov/CSRC/media/Projects/Cryptographic-Algorithm-Validation
> -Program/documents/mac/gcmtestvectors.zip
> 
> /gcmtestvectors/gcmEncryptExtIV192.rsp
> 
> 
> TEST 1:
> GCM
> [Keylen = 192]
> [IVlen = 8 bits]
> [PTlen = 128]
> [AADlen = 0]
> [Taglen = 128]
> 
> NIST vector:
> Key = d4ba70cb3e8d246aa66ebfafd26266b5f08ec3a88000e770
> IV = 13
> PT = 0616236190652619ff51ad2775f2826e
> AAD =
> CT = 52b5f106a01d1cef4c833099ce88a354
> Tag = d8acd529c97efbefb6102a4a9c3dafb2
> 
> attempt1: jlulla@ubuntu:~/libkcapi-1.0.3/bin$ ./kcapi -x 2 -e -c
> "gcm(aes)" -p 0616236190652619ff51ad2775f2826e -k
> d4ba70cb3e8d246aa66ebfafd26266b5f08ec3a88000e770 -i 13 -l 16
> 172e34500211d494ec35171aa488a26e65bc6a61759a974751875ab6fe27caed
> 
> attempt2: jlulla@ubuntu:~/libkcapi-1.0.3/bin$ ./kcapi -x 2 -e -c
> "gcm(aes)" -p 0616236190652619ff51ad2775f2826e -k
> d4ba70cb3e8d246aa66ebfafd26266b5f08ec3a88000e770 -i 13 -a "" -l 16
> 172e34500211d494ec35171aa488a26e65bc6a61759a974751875ab6fe27caed
> 
> attempt3: jlulla@ubuntu:~/libkcapi-1.0.3/bin$ ./kcapi -x 2 -e -c
> "gcm(aes)" -p 0616236190652619ff51ad2775f2826e -k
> d4ba70cb3e8d246aa66ebfafd26266b5f08ec3a88000e770 -i 13 -a 0 -l 16
> 172e34500211d494ec35171aa488a26e65bc6a61759a974751875ab6fe27caed
> 
> 
> SO the tag and the ct both not matching in all 3 attempts above.

IV of 1 byte? See /proc/crypto: IV must be 96 bits.
> 
> 
> TEST 2:
> 
> Similarly for XTS also we have one mismatch:
> https://csrc.nist.gov/CSRC/media/Projects/Cryptographic-Algorithm-Validation
> -Program/documents/aes/XTSTestVectors.zip
> 
> /XTSTestVectors/format tweak value input - 128 hex str/XTSGenAES256.rsp
> 
> kcapi -x 1 -e -c "xts(aes)" -k
> 31c8152b5eddc3b8c3a005a3bbc4c005bb57058ae4a6454c166a620389eaecaea0515433574b
> 0dd6a89496acd475ef78dcf012a47a48c319f89e931404018e15 -p
> 31761b6dece3e962030c01f481c5ca681386176d2ef8034c5db5aa04b613ec00 -i
> 6957d297dc9c9b30f6d016b016d913c5
> 
> Result from tool :
> 1e16b5a44274f8791508cf3dec971aa975e16c702d66f11bc1f00ede540ef82c
> 
> NIST Expected Result :
> ae13222810bc66997bf8b57737990e481e16b5a44274f8791508cf3dec971a80

I need to check that.


Ciao
Stephan

[PATCH v2 2/2] crypto: ccree: use proper printk format

[Gilad Ben-Yossef]

Fix incorrect use of %pad as a printk format string for none dma_addr_t
variable.

Discovered via smatch.

Signed-off-by: Gilad Ben-Yossef 
---
 drivers/crypto/ccree/cc_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/ccree/cc_driver.c b/drivers/crypto/ccree/cc_driver.c
index 89ce013..37f2e6e 100644
--- a/drivers/crypto/ccree/cc_driver.c
+++ b/drivers/crypto/ccree/cc_driver.c
@@ -265,7 +265,7 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
}
 
if (rc) {
-   dev_err(dev, "Failed in dma_set_mask, mask=%pad\n", _mask);
+   dev_err(dev, "Failed in dma_set_mask, mask=%llx\n", dma_mask);
return rc;
}
 
-- 
2.7.4

[PATCH v2 1/2] crypto: ccree: enable support for hardware keys

[Gilad Ben-Yossef]

Enable CryptoCell support for hardware keys.

Hardware keys are regular AES keys loaded into CryptoCell internal memory
via firmware, often from secure boot ROM or hardware fuses at boot time.

As such, they can be used for enc/dec purposes like any other key but
cannot (read: extremely hard to) be extracted since since they are not
available anywhere in RAM during runtime.

The mechanism has some similarities to s390 secure keys although the keys
are not wrapped or sealed, but simply loaded offline. The interface was
therefore modeled based on the s390 secure keys support.

Signed-off-by: Gilad Ben-Yossef 
---
 crypto/testmgr.c |  43 +
 drivers/crypto/ccree/cc_cipher.c | 350 ++-
 drivers/crypto/ccree/cc_cipher.h |  30 +---
 3 files changed, 361 insertions(+), 62 deletions(-)

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 397b117..c31da0f 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -2582,6 +2582,13 @@ static const struct alg_test_desc alg_test_descs[] = {
}
}
}, {
+   /* Same as cbc(aes) except the key is stored in
+* hardware secure memory which we reference by index
+*/
+   .alg = "cbc(paes)",
+   .test = alg_test_null,
+   .fips_allowed = 1,
+   }, {
.alg = "cbc(serpent)",
.test = alg_test_skcipher,
.suite = {
@@ -2728,6 +2735,13 @@ static const struct alg_test_desc alg_test_descs[] = {
}
}
}, {
+   /* Same as ctr(aes) except the key is stored in
+* hardware secure memory which we reference by index
+*/
+   .alg = "ctr(paes)",
+   .test = alg_test_null,
+   .fips_allowed = 1,
+   }, {
.alg = "ctr(serpent)",
.test = alg_test_skcipher,
.suite = {
@@ -2998,6 +3012,13 @@ static const struct alg_test_desc alg_test_descs[] = {
}
}
}, {
+   /* Same as ecb(aes) except the key is stored in
+* hardware secure memory which we reference by index
+*/
+   .alg = "ecb(paes)",
+   .test = alg_test_null,
+   .fips_allowed = 1,
+   }, {
.alg = "ecb(khazad)",
.test = alg_test_skcipher,
.suite = {
@@ -3325,6 +3346,13 @@ static const struct alg_test_desc alg_test_descs[] = {
}
}
}, {
+   /* Same as ofb(aes) except the key is stored in
+* hardware secure memory which we reference by index
+*/
+   .alg = "ofb(paes)",
+   .test = alg_test_null,
+   .fips_allowed = 1,
+   }, {
.alg = "pcbc(fcrypt)",
.test = alg_test_skcipher,
.suite = {
@@ -3582,6 +3610,21 @@ static const struct alg_test_desc alg_test_descs[] = {
}
}
}, {
+   /* Same as xts(aes) except the key is stored in
+* hardware secure memory which we reference by index
+*/
+   .alg = "xts(paes)",
+   .test = alg_test_null,
+   .fips_allowed = 1,
+   }, {
+   .alg = "xts4096(paes)",
+   .test = alg_test_null,
+   .fips_allowed = 1,
+   }, {
+   .alg = "xts512(paes)",
+   .test = alg_test_null,
+   .fips_allowed = 1,
+   }, {
.alg = "xts(camellia)",
.test = alg_test_skcipher,
.suite = {
diff --git a/drivers/crypto/ccree/cc_cipher.c b/drivers/crypto/ccree/cc_cipher.c
index df98f7a..d2810c1 100644
--- a/drivers/crypto/ccree/cc_cipher.c
+++ b/drivers/crypto/ccree/cc_cipher.c
@@ -42,6 +42,7 @@ struct cc_cipher_ctx {
int cipher_mode;
int flow_mode;
unsigned int flags;
+   bool hw_key;
struct cc_user_key_info user;
struct cc_hw_key_info hw;
struct crypto_shash *shash_tfm;
@@ -49,6 +50,13 @@ struct cc_cipher_ctx {
 
 static void cc_cipher_complete(struct device *dev, void *cc_req, int err);
 
+static inline bool cc_is_hw_key(struct crypto_tfm *tfm)
+{
+   struct cc_cipher_ctx *ctx_p = crypto_tfm_ctx(tfm);
+
+   return ctx_p->hw_key;
+}
+
 static int validate_keys_sizes(struct cc_cipher_ctx *ctx_p, u32 size)
 {
switch (ctx_p->flow_mode) {
@@ -211,7 +219,7 @@ struct tdes_keys {
u8  key3[DES_KEY_SIZE];
 };
 
-static enum cc_hw_crypto_key hw_key_to_cc_hw_key(int slot_num)
+static enum cc_hw_crypto_key cc_slot_to_hw_key(int slot_num)
 {
switch (slot_num) {
case 0:
@@ -226,69 +234,100 @@ static enum cc_hw_crypto_key 

[PATCH v2 0/2] cleanup and hardware keys

[Gilad Ben-Yossef]

Small cleanup and add support for CryptoCell hardware keys.

Changes from v1:
- The cleanup patch is new, as Herbert took the previous one :-)
- Hardware key API uses the "paes" designator to indicate a protected key,
  same as s390, as per Herbert preference
- Avoid direct casting and reference of key material, as pointed out by
  Herbert

Gilad Ben-Yossef (2):
  crypto: ccree: enable support for hardware keys
  crypto: ccree: use proper printk format

 crypto/testmgr.c |  43 +
 drivers/crypto/ccree/cc_cipher.c | 350 ++-
 drivers/crypto/ccree/cc_cipher.h |  30 +---
 drivers/crypto/ccree/cc_driver.c |   2 +-
 4 files changed, 362 insertions(+), 63 deletions(-)

-- 
2.7.4

Re: [PATCH 2/2] crypto: ccree: enable support for hardware keys

[Gilad Ben-Yossef]

On Thu, Apr 19, 2018 at 6:35 AM, Herbert Xu  wrote:
> On Mon, Apr 09, 2018 at 11:42:31AM +0300, Gilad Ben-Yossef wrote:
>>
>> Please look again. The stub version of cc_is_hw_key() doing that is being
>> replaced in this patch.
>
> The point is that the existing mechanism was unused before and this
> is new code.  So you can't really point to the stubbed-out function
> as a precedent.

hm... I was trying to point to the s390 implementation as a precedent,
not my own stub code.
Sorry if I miscommunicated my intent.

>
>> The s390 key and the cryptocell keys are not the same:
>>
>> Their is, I believe, is an AES key encrypted by some internal key/algorithm.
>>
>> The cryptocell "key" is a token, which is internally comprised of one
>> or two indexes, referencing slots in the internal memory in the
>> hardware, and a key size, that describe the size of the key.
>>
>> I thought it would be confusing to use "paes" to describe both, since
>> they are not interchangeable.
>> You would not be able to feed an paes key that works with the s390
>> version to cryptocell and vice verse and get it work.
>
> Thanks for the info.
>
>> Having said, if you prefer to have "paes" simply designate
>> "implementation specific token for an AES key" I'm perfectly fine with
>> that.
>
> Well by definition none of these hardware keys will be compatible
> with each other so I don't really see the point of using individual
> algorithm names such as paes or haes.  This would make sense only if
> they were somehow compatible with each other.
>
> So instead of using algorithm names, you really want refer to the
> specific driver name, which means that they can all use the same
> algorithm name.

Sounds good to me.

>
>> > As to your patch specifically, there is one issue where you're
>> > directly dereferencing the key as a struct.  This is a no-no because
>> > the key may have come from user-space.  You must treat it as a
>> > binary blob.  The s390 code seems to do this correctly.
>>
>> As noted above, the haes "key" is really a token encoding 3 different
>> pieces of information:
>
> My point is that you should not just cast it but instead do a
> copy to properly aligned kernel memory.


That is a good point I completely missed. Thanks!

A v2 will follow shortly.

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