Re: [PATCH] crypto: testmgr: Allow different compression results
On Wed, Apr 11, 2018 at 08:28:32PM +0200, Jan Glauber wrote:
>
> @@ -1362,7 +1373,17 @@ static int test_comp(struct crypto_comp *tfm,
> goto out;
> }
>
> - if (dlen != ctemplate[i].outlen) {
> + ilen = dlen;
> + dlen = COMP_BUF_SIZE;
> + ret = crypto_comp_decompress(tfm, output,
> + ilen, decomp_output, &dlen);
> + if (ret) {
> + pr_err("alg: comp: compression failed: decompress: on
> test %d for %s failed: ret=%d\n",
> +i + 1, algo, -ret);
> + goto out;
> + }
> +
> + if (dlen != ctemplate[i].inlen) {
> printk(KERN_ERR "alg: comp: Compression test %d "
> "failed for %s: output len = %d\n", i + 1, algo,
> dlen);
Your patch is fine as it is.
But I just thought I'd mention that if anyone wants to we should
really change this to use a different tfm, e.g., always use the
generic algorithm to perform the decompression. This way if there
were multiple implementations we can at least test them against
the generic one.
Otherwise you could end up with a buggy implementation that works
against itself but still generates incorrect output.
Cheers,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 2/2] crypto: ccree: enable support for hardware keys
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. > 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. > > 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. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support
On Thu, Apr 19, 2018 at 12:54:16AM +, Dey, Megha wrote: > > Yeah I think I misunderstood. I think what you mean is to remove mcryptd.c > completely and avoid the extra layer of indirection to call the underlying > algorithm, instead call it directly, correct? > > So currently we have 3 algorithms registered for every multibuffer algorithm: > name : __sha1-mb > driver : mcryptd(__intel_sha1-mb) > > name : sha1 > driver : sha1_mb > > name : __sha1-mb > driver : __intel_sha1-mb > > If we remove mcryptd, then we will have just the 2? It should be down to just one, i.e., the current inner algorithm. It's doing all the scheduling work already so I don't really see why it needs the wrappers around it. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
RE: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support
>-Original Message- >From: Herbert Xu [mailto:herb...@gondor.apana.org.au] >Sent: Wednesday, April 18, 2018 4:01 AM >To: Dey, Megha >Cc: linux-ker...@vger.kernel.org; linux-crypto@vger.kernel.org; >da...@davemloft.net >Subject: Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure >support > >On Tue, Apr 17, 2018 at 06:40:17PM +, Dey, Megha wrote: >> >> >> >-Original Message- >> >From: Herbert Xu [mailto:herb...@gondor.apana.org.au] >> >Sent: Friday, March 16, 2018 7:54 AM >> >To: Dey, Megha >> >Cc: linux-ker...@vger.kernel.org; linux-crypto@vger.kernel.org; >> >da...@davemloft.net >> >Subject: Re: [PATCH V8 1/5] crypto: Multi-buffer encryption >> >infrastructure support >> > >> >I have taken a deeper look and I'm even more convinced now that >> >mcryptd is simply not needed in your current model. >> > >> >The only reason you would need mcryptd is if you need to limit the >> >rate of requests going into the underlying mb algorithm. >> > >> >However, it doesn't do that all. Even though it seems to have a >> >batch size of 10, but because it immediately reschedules itself after >> >the batch runs out, it's essentially just dumping all requests at the >> >underlying algorithm as fast as they're coming in. The underlying >> >algorithm doesn't have need throttling anyway because it'll do the work >when the queue is full synchronously. >> > >> >So why not just get rid of mcryptd completely and expose the >> >underlying algorithm as a proper async skcipher/hash? >> >> Hi Herbert, >> >> Most part of the cryptd.c and mcryptd.c are similar, except the logic >> used to flush out partially completed jobs in the case of multibuffer >algorithms. >> >> I think I will try to merge the cryptd and mcryptd adding necessary quirks >> for >multibuffer where needed. > >I think you didn't quite get my point. From what I'm seeing you don't need >either cryptd or mcryptd. You just need to expose the underlying mb >algorithm directly. Hi Herbert, Yeah I think I misunderstood. I think what you mean is to remove mcryptd.c completely and avoid the extra layer of indirection to call the underlying algorithm, instead call it directly, correct? So currently we have 3 algorithms registered for every multibuffer algorithm: name : __sha1-mb driver : mcryptd(__intel_sha1-mb) name : sha1 driver : sha1_mb name : __sha1-mb driver : __intel_sha1-mb If we remove mcryptd, then we will have just the 2? The outer algorithm:sha1-mb, will > >So I'm not sure what we would gain from merging cryptd and mcryptd. > >Cheers, >-- >Email: Herbert Xu Home Page: >http://gondor.apana.org.au/~herbert/ >PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support
On Tue, Apr 17, 2018 at 06:40:17PM +, Dey, Megha wrote: > > > >-Original Message- > >From: Herbert Xu [mailto:herb...@gondor.apana.org.au] > >Sent: Friday, March 16, 2018 7:54 AM > >To: Dey, Megha > >Cc: linux-ker...@vger.kernel.org; linux-crypto@vger.kernel.org; > >da...@davemloft.net > >Subject: Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure > >support > > > >I have taken a deeper look and I'm even more convinced now that mcryptd is > >simply not needed in your current model. > > > >The only reason you would need mcryptd is if you need to limit the rate of > >requests going into the underlying mb algorithm. > > > >However, it doesn't do that all. Even though it seems to have a batch size > >of > >10, but because it immediately reschedules itself after the batch runs out, > >it's essentially just dumping all requests at the underlying algorithm as > >fast > >as they're coming in. The underlying algorithm doesn't have need throttling > >anyway because it'll do the work when the queue is full synchronously. > > > >So why not just get rid of mcryptd completely and expose the underlying > >algorithm as a proper async skcipher/hash? > > Hi Herbert, > > Most part of the cryptd.c and mcryptd.c are similar, except the logic used to > flush out partially completed jobs > in the case of multibuffer algorithms. > > I think I will try to merge the cryptd and mcryptd adding necessary quirks > for multibuffer where needed. I think you didn't quite get my point. From what I'm seeing you don't need either cryptd or mcryptd. You just need to expose the underlying mb algorithm directly. So I'm not sure what we would gain from merging cryptd and mcryptd. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH/RFC] crypto: Add platform dependencies for CRYPTO_DEV_CCREE
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 ;-)
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?
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
Hi Arnd,
On Tue, Apr 17, 2018 at 9:53 PM, Arnd Bergmann wrote:
> On Tue, Apr 17, 2018 at 8: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.
>>
>> Drop the dependency on HAS_DMA, as DMA is always available on ARM and
>> ARM64 platforms, and doing so will increase compile coverage.
>>
>> Signed-off-by: Geert Uytterhoeven
>> ---
>> Is ARM || ARM64 OK?
>> Or should this be limited to either ARM or ARM64? Or something else?
>
> ARM || ARM64 seems fine, but don't you need '|| (HAS_DMA && COMPILE_TEST)'?
>
> I assume the HAS_DMA dependency was added to prevent compile
> testing to run into a build error.
Probably it was. But in v4.17-rc1, dummies are present in the NO_DMA case,
so everything compile-tests fine.
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
