Re: [RFC] crypto: Remove mcryptd
On Fri, 2018-07-20 at 11:53 +0800, Herbert Xu wrote:
> On Fri, May 11, 2018 at 06:44:13PM -0700, Megha Dey wrote:
> >
> > +static struct ahash_alg *simd_ahash_create_compat(const char *algname,
> > + const char *drvname,
> > + const char *basename)
> > +{
> > + struct ahash_alg *alg;
> > + struct ahash_alg *ialg;
> > + int err;
>
> I think there has been a misunderstsanding. You're not actually
> using the simd wrapper here. All you're doing is creating a function
> with the word simd in its name. In all other respects this is just
> exposing the underlying algorithm to users directly, which cannot
> work because the underlying algorithm requires SIMD.
Hi Herbert,
Thanks for the feedback.
I still have some questions though:
1. On the existing algorithms covered in aesni_intel-glue.c (eg:
__cbc-aes-aesni), 3 algorithms are registered in /proc/crypto:
__cbc(aes)
cryptd(__cbc-aes-aesni)--> registered via cryptd_create_skcipher
cbc(aes)
cbc-aes-aesni --> registered via simd_skcipher_create_compat
__cbc(aes)
__cbc-aes-aesni--> registered as the internal algorithm
I would want to know why do we need the cryptd(__cbc-aes-aesni)
algorithm at all. I do not see any of the associated setkey, encrypt or
decrypt functions getting called during the selftest or while running
tcrypt. I just see the simd_(setkey, encrypt, decrypt) functions
directly called the inner algorithms. However, if I remove the cryptd
algorithm, none of the algorithms are registered.
>
> What you need to do is create an actual simd wrapper with cryptd
This simd wrapper is already present for skcipher right(in simd.c)?
Assuming we only have ciphers and no hash algorithms, are any changes
required in these wrappers?
Pseudo code:
1. Register inner algorithm (cbc-aes-aesni-mb) in aes_cbc_mb_mod_init()
2. Register outer algorithm with the mcryptd- prefix for the driver name
using the simd_skcipher_create_compat(mcryptd-cbc-aes-aesni-mb)
3. tcrypt/testmanager calls the
crypto_skcipher_encrypt->simd_skcipher_encrypt->mb_cbc_aes_encrypt
4. Shift helper functions which help flush outstanding jobs to glue
layer.
5. Delete mcryptd.c
6. All similar simd wrapper for hash algorithms.
> and all the functions that may do SIMD work needs to invoke cryptd
> if may_use_simd() (and other conditions) is false.
>
> This wrapper should live in crypto/simd.c.
>
> Cheers,
Re: [PATCH] crypto: ccp: Check for NULL PSP pointer at module unload
On 07/26/2018 09:37 AM, Tom Lendacky wrote:
Should the PSP initialization fail, the PSP data structure will be
freed and the value contained in the sp_device struct set to NULL.
At module unload, psp_dev_destroy() does not check if the pointer
value is NULL and will end up dereferencing a NULL pointer.
Add a pointer check of the psp_data field in the sp_device struct
in psp_dev_destroy() and return immediately if it is NULL.
Cc: # 4.16.x-
Fixes: 2a6170dfe755 ("crypto: ccp: Add Platform Security Processor (PSP) device
support")
Signed-off-by: Tom Lendacky
Acked-by: Gary R Hook
---
drivers/crypto/ccp/psp-dev.c |3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index 9b59638..218739b 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -858,6 +858,9 @@ void psp_dev_destroy(struct sp_device *sp)
{
struct psp_device *psp = sp->psp_data;
+ if (!psp)
+ return;
+
if (psp->sev_misc)
kref_put(&misc_dev->refcount, sev_exit);
[PATCH] crypto: ccp: Check for NULL PSP pointer at module unload
Should the PSP initialization fail, the PSP data structure will be
freed and the value contained in the sp_device struct set to NULL.
At module unload, psp_dev_destroy() does not check if the pointer
value is NULL and will end up dereferencing a NULL pointer.
Add a pointer check of the psp_data field in the sp_device struct
in psp_dev_destroy() and return immediately if it is NULL.
Cc: # 4.16.x-
Fixes: 2a6170dfe755 ("crypto: ccp: Add Platform Security Processor (PSP) device
support")
Signed-off-by: Tom Lendacky
---
drivers/crypto/ccp/psp-dev.c |3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index 9b59638..218739b 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -858,6 +858,9 @@ void psp_dev_destroy(struct sp_device *sp)
{
struct psp_device *psp = sp->psp_data;
+ if (!psp)
+ return;
+
if (psp->sev_misc)
kref_put(&misc_dev->refcount, sev_exit);
Re: [PATCH 0/4] crypto/arm64: reduce impact of NEON yield checks
On 2018-07-26 09:25:40 [+0200], Ard Biesheuvel wrote: > Thanks a lot. > > So 20 us ~= 20,000 cycles on my 1 GHz Cortex-A53, and if I am > understanding you correctly, you wouldn't mind the quantum of work to > be in the order 16,000 cycles or even substantially more? I have currently that one box and it does not seem to be a problem. So it reports now on idle around 20us max. So if add "only" 20us to NEON / your preempt-disable section then we may end up at 20+20 = 40us. At this point I am not sure how "bad" it is. It works, it does not seem that much and you can disable it if you don't want the extra 20us here. > That is good news, but it is also rather interesting, given that these > algorithms run at ~4 cycles per byte, meaning that you'd manage an > entire 4 KB page without ever yielding. (GCM is used on network > packets, XTS on disk sectors which are all smaller than that) > > Do you remember how you found out NEON use is a problem for -rt on > arm64 in the first place? Which algorithm did you test at the time to > arrive at this conclusion? I *think* that yield got in there by chance. The main problem was back at the time that within the neon begin/end section there was the scatter list walk. That walk may invoke kmap() / kmalloc() / kfree() and is not allowed on RT within a preempt-disable section. This was my main concern. > Note that AES-GCM using ordinary SIMD instructions runs at 29 cpb, and > plain AES at ~20 (on A53), so perhaps it would make sense to > distinguish between algos using crypto instructions and ones using > plain SIMD. I was looking at AES-CE and AES-NEON (aes-neon-blk / aes_ce_blk) with modprobe tcrypt mode=200 sec=1 and mode=403 +404 for the sha1/256 test. Sebastian
Re: [PATCH 0/4][RFC v2] Introduce the in-kernel hibernation encryption
On Thu, Jul 26, 2018 at 09:30:46AM +0200, Oliver Neukum wrote: > On Di, 2018-07-24 at 00:23 +0800, Yu Chen wrote: > > > > Good point, we once tried to generate key in kernel, but people > > suggest to generate key in userspace and provide it to the > > kernel, which is what ecryptfs do currently, so it seems this > > should also be safe for encryption in kernel. > > https://www.spinics.net/lists/linux-crypto/msg33145.html > > Thus Chun-Yi's signature can use EFI key and both the key from > > user space. > > Hi, > > ecryptfs can trust user space. It is supposed to keep data > safe while the system is inoperative. The whole point of Secure > Boot is a cryptographic system of trust that does not include > user space. > > I seriously doubt we want to use trusted computing here. So the > key needs to be generated in kernel space and stored in a safe > manner. As we have a saolution doing that, can we come to ausable > synthesis? > > Regards > Oliver Crurently there have two solutions, they are trusted key and EFI key. Both of them are generated in kernel and are not visible in user space. The trusted key is generated by kernel then sealed by the TPM's SRK. So the trusted key can be stored in anywhere then be enrolled to kernel when we need it. EVM already uses it. The EFI key is Jiri Kosina's idea. It is stored in boot services variable, which means that it can only be access by signed EFI binary (e.g. signed EFI boot stub) when secure boot be enabled. SLE applied this solution a couple of years. I am working on put the EFI key to key retention service. Then EFI key can be a master key of encrypted key. EVM can also use it: https://github.com/joeyli/linux-s4sign/commit/bae39460393ada4c0226dd07cd5e3afcef86b71f https://github.com/joeyli/linux-s4sign/commit/f552f97cc3cca5acd84f424b7f946ffb5fe8e9ec That's why I want to use key retention service in hibernation encryption/authentication. Which means that we can use key API to access trusted key and EFI key. Thanks Joey Lee
Re: [PATCH 0/4][RFC v2] Introduce the in-kernel hibernation encryption
On Di, 2018-07-24 at 00:23 +0800, Yu Chen wrote: > > Good point, we once tried to generate key in kernel, but people > suggest to generate key in userspace and provide it to the > kernel, which is what ecryptfs do currently, so it seems this > should also be safe for encryption in kernel. > https://www.spinics.net/lists/linux-crypto/msg33145.html > Thus Chun-Yi's signature can use EFI key and both the key from > user space. Hi, ecryptfs can trust user space. It is supposed to keep data safe while the system is inoperative. The whole point of Secure Boot is a cryptographic system of trust that does not include user space. I seriously doubt we want to use trusted computing here. So the key needs to be generated in kernel space and stored in a safe manner. As we have a saolution doing that, can we come to ausable synthesis? Regards Oliver
Re: [PATCH 0/4] crypto/arm64: reduce impact of NEON yield checks
On 25 July 2018 at 18:50, bige...@linutronix.de wrote: > On 2018-07-25 11:54:53 [+0200], Ard Biesheuvel wrote: >> Indeed. OTOH, if the -rt people (Sebastian?) turn up and say that a >> 1000 cycle limit to the quantum of work performed with preemption >> disabled is unreasonably low, we can increase the yield block counts >> and approach the optimal numbers a bit closer. But with diminishing >> returns. > > So I tested on SoftIron Overdrive 1000 which has A57 cores. I added this > series and didn't notice any spikes. This means cyclictest reported a > max value of like ~20us (which means the crypto code was not > noticeable). > I played a little with it and tcrypt tests for aes/sha1 and also no huge > spikes. So at this point this looks fantastic. I also setup cryptsetup / > dm-crypt with the usual xts(aes) mode and saw no spikes. > At this point, on this hardware if you want to raise the block count, I > wouldn't mind. > > I remember on x86 the SIMD accelerated ciphers led to ~1ms+ spikes once > dm-crypt started its jobs. > Thanks a lot. So 20 us ~= 20,000 cycles on my 1 GHz Cortex-A53, and if I am understanding you correctly, you wouldn't mind the quantum of work to be in the order 16,000 cycles or even substantially more? That is good news, but it is also rather interesting, given that these algorithms run at ~4 cycles per byte, meaning that you'd manage an entire 4 KB page without ever yielding. (GCM is used on network packets, XTS on disk sectors which are all smaller than that) Do you remember how you found out NEON use is a problem for -rt on arm64 in the first place? Which algorithm did you test at the time to arrive at this conclusion? Note that AES-GCM using ordinary SIMD instructions runs at 29 cpb, and plain AES at ~20 (on A53), so perhaps it would make sense to distinguish between algos using crypto instructions and ones using plain SIMD.
