Re: [RFC PATCH v2 00/12] crypto: Adiantum support
Hey Eric, On Mon, Oct 22, 2018 at 12:23 AM Eric Biggers wrote: > I started a branch based on Zinc: Nice to see. I'm heading to bed in a second, so I'll give this a thorough read-through tomorrow, but some preliminary notes on your comments: > For Poly1305, for now I decided to just use the existing functions, passing 0 > for the 16-byte element is added at the end. This causes some unnecessary > overhead, but it's not very much. It also results in a much larger size of > 'struct nhpoly1305_state', but that doesn't matter too much anymore either > [1]. > [1] Originally we were going to define Adiantum's hash function to be > Poly1305(message_length || tweak_length || tweak || NH(message)), which > would have made it desirable to export the Poly1305 state before NH, so > that > it could be imported for the second hash step to avoid redundantly hashing > the message length and tweak. But later we changed it to > Poly1305(message_length || tweak) + Poly1305(NH(message)). Out of curiosity, why this change? > For ChaCha, I haven't yet updated all the "Zinc" assembly to support 12 > rounds. > So far I've updated my ARM scalar implementation. I still don't see how you > expect people to maintain the files like chacha20-x86_64.S from which all > comments, register aliases, etc. were removed in comparison to the original > OpenSSL code. For at least the ARM[64] and MIPS64 code, I think it will be feasible to import the .pl eventually. There's an open PR from Andy importing some of the necessary changes. For the x86_64, that might be a little trickier, but I can take another stab at it. > I don't see how dumping thousands of lines of undocumented, > generated assembly code into the kernel fits with your goals of "Zinc's focus > is > on simplicity and clarity" and "inviting collaboration". It's not totally "undocumented" and totally "dumped"; that's a bit hyperbolic. But I can understand it's not as friendly as we'd like. I'll try to improve that. > Note that the > OpenSSL-derived assembly files still have an unclear license as well. Andy's been pretty clear about the CRYPTOGAMS aspect with me. But, as you pointed out on lkml and in the private thread, it hasn't yet migrated over to the CRYPTOGAMS repo. I don't think this is a cause for immediate concern, because it seems pretty certain it will wind up there soon enough. > (I haven't yet gotten around to adding > "zinc tests" for XChaCha12, though I did add "crypto tests". Note that > "crypto > tests" are much easier to add, since all algorithms of the same type share a > common test framework -- not the case for Zinc.) Actually the advantage of not working with a winding abstraction layer is that specific tests can test particular aspects of particular primitives -- for example, by looking at different chunking patterns. It also enables you to write tests for internal, non-exported functions. > nor is there a documentation file > explaining things. Sorry, my bad on delaying that one. I'll be sure the Documentation/ stuff is ready before posting another series. > So please understand that until it's clear that Zinc is > ready, I still have to have Adiantum ready to go without Zinc, just in case. Makes sense. I do really appreciate you taking the time, though, to try this out with Zinc as well. Thanks for that. Regards, Jason
Re: [RFC PATCH v2 00/12] crypto: Adiantum support
On Fri, Oct 19, 2018 at 12:04:11PM -0700, Eric Biggers wrote: > Hi Jason, > > On Fri, Oct 19, 2018 at 05:58:35PM +0200, Jason A. Donenfeld wrote: > > Hello Eric, > > > > > As before, some of these patches conflict with the new "Zinc" crypto > > > library. But I don't know when Zinc will be merged, so for now I've > > > continued to base this patchset on the current 'cryptodev'. > > > > I'd appreciate it if you waited to merge this until you can rebase it > > on top of Zinc. In fact, if you already want to build it on top of > > Zinc, I'm happy to work with you on that in a shared repo or similar. > > We can also hash out the details of that in person in Vancouver in a > > few weeks. I think pushing this in before will create undesirable > > churn for both of us. > > > > I won't be at Plumbers, sorry! For if/when it's needed, I'll start a version > of > this based on Zinc. The basic requirements are that we need (1) xchacha12 and > xchacha20 available as 'skciphers' in the crypto API, and (2) the > poly1305_core > functions (see patch 08/12). In principle, these can be implemented in Zinc. > The Adiantum template and all the NHPoly1305 stuff will be the same either > way. > (Unless you'll want one or both of those moved to Zinc too. To be honest, > even > after your explanations I still don't have a clear idea of what is supposed to > go in Zinc and what isn't...) > > However, for now I'm hesitant to completely abandon the current approach and > bet > the farm on Zinc. Zinc has a large scope and various controversies that > haven't > yet been fully resolved to everyone's satisfaction, including unclear licenses > on some of the essential assembly files. It's not appropriate to grind kernel > crypto development to grind a halt while everyone waits for Zinc. > > So if Zinc is ready, then it makes sense for it to go first; > otherwise, it doesn't. It's not yet clear which is the case. > I started a branch based on Zinc: https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git, branch "adiantum-zinc". For Poly1305, for now I decided to just use the existing functions, passing 0 for the 16-byte element is added at the end. This causes some unnecessary overhead, but it's not very much. It also results in a much larger size of 'struct nhpoly1305_state', but that doesn't matter too much anymore either [1]. For ChaCha, I haven't yet updated all the "Zinc" assembly to support 12 rounds. So far I've updated my ARM scalar implementation. I still don't see how you expect people to maintain the files like chacha20-x86_64.S from which all comments, register aliases, etc. were removed in comparison to the original OpenSSL code. I find it hard to very understand what's going on from what is nearly an 'objdump' output. (I'll figure it out eventually, but it will take some time.) I don't see how dumping thousands of lines of undocumented, generated assembly code into the kernel fits with your goals of "Zinc's focus is on simplicity and clarity" and "inviting collaboration". Note that the OpenSSL-derived assembly files still have an unclear license as well. I'm also still not a fan of the remaining duplication between "zinc" and "crypto", e.g. we still have both crypto/chacha.h and zinc/chacha.h, and separate tests for "zinc" and "crypto". (I haven't yet gotten around to adding "zinc tests" for XChaCha12, though I did add "crypto tests". Note that "crypto tests" are much easier to add, since all algorithms of the same type share a common test framework -- not the case for Zinc.) Of course, both myself and others have expressed concerns about these issues previously too, yet they remain unaddressed nor is there a documentation file explaining things. So please understand that until it's clear that Zinc is ready, I still have to have Adiantum ready to go without Zinc, just in case. Thanks, - Eric [1] Originally we were going to define Adiantum's hash function to be Poly1305(message_length || tweak_length || tweak || NH(message)), which would have made it desirable to export the Poly1305 state before NH, so that it could be imported for the second hash step to avoid redundantly hashing the message length and tweak. But later we changed it to Poly1305(message_length || tweak) + Poly1305(NH(message)).
Re: [PATCH 1/2] crypto: fix cfb mode decryption
On 21 October 2018 at 11:00, James Bottomley
wrote:
> On October 21, 2018 9:58:04 AM GMT, Ard Biesheuvel
> wrote:
>>On 21 October 2018 at 10:07, James Bottomley
>> wrote:
>>> On Sun, 2018-10-21 at 09:05 +0200, Ard Biesheuvel wrote:
(+ James)
>>>
>>> Thanks!
>>>
On 20 October 2018 at 01:01, Dmitry Eremin-Solenikov
wrote:
> crypto_cfb_decrypt_segment() incorrectly XOR'ed generated
>>keystream
> with
> IV, rather than with data stream, resulting in incorrect
> decryption.
> Test vectors will be added in the next patch.
>
> Signed-off-by: Dmitry Eremin-Solenikov
> Cc: sta...@vger.kernel.org
> ---
> crypto/cfb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/crypto/cfb.c b/crypto/cfb.c
> index a0d68c09e1b9..fd4e8500e121 100644
> --- a/crypto/cfb.c
> +++ b/crypto/cfb.c
> @@ -144,7 +144,7 @@ static int crypto_cfb_decrypt_segment(struct
> skcipher_walk *walk,
>
> do {
> crypto_cfb_encrypt_one(tfm, iv, dst);
> - crypto_xor(dst, iv, bsize);
> + crypto_xor(dst, src, bsize);
>>>
>>> This does look right. I think the reason the TPM code works is that
>>it
>>> always does encrypt/decrypt in-place, which is a separate piece of
>>the
>>> code which appears to be correct.
>>>
>>
>>Yeah I figured that.
>>
>>So where is the TPM code that actually uses this code?
>
> It was posted to the integrity list a while ago. I'm planning a repost
> shortly.
>
OK, found it. Mind cc'ing me on that repost?
Re: [PATCH 1/2] crypto: fix cfb mode decryption
On October 21, 2018 9:58:04 AM GMT, Ard Biesheuvel
wrote:
>On 21 October 2018 at 10:07, James Bottomley
> wrote:
>> On Sun, 2018-10-21 at 09:05 +0200, Ard Biesheuvel wrote:
>>> (+ James)
>>
>> Thanks!
>>
>>> On 20 October 2018 at 01:01, Dmitry Eremin-Solenikov
>>> wrote:
>>> > crypto_cfb_decrypt_segment() incorrectly XOR'ed generated
>keystream
>>> > with
>>> > IV, rather than with data stream, resulting in incorrect
>>> > decryption.
>>> > Test vectors will be added in the next patch.
>>> >
>>> > Signed-off-by: Dmitry Eremin-Solenikov
>>> > Cc: sta...@vger.kernel.org
>>> > ---
>>> > crypto/cfb.c | 2 +-
>>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>>> >
>>> > diff --git a/crypto/cfb.c b/crypto/cfb.c
>>> > index a0d68c09e1b9..fd4e8500e121 100644
>>> > --- a/crypto/cfb.c
>>> > +++ b/crypto/cfb.c
>>> > @@ -144,7 +144,7 @@ static int crypto_cfb_decrypt_segment(struct
>>> > skcipher_walk *walk,
>>> >
>>> > do {
>>> > crypto_cfb_encrypt_one(tfm, iv, dst);
>>> > - crypto_xor(dst, iv, bsize);
>>> > + crypto_xor(dst, src, bsize);
>>
>> This does look right. I think the reason the TPM code works is that
>it
>> always does encrypt/decrypt in-place, which is a separate piece of
>the
>> code which appears to be correct.
>>
>
>Yeah I figured that.
>
>So where is the TPM code that actually uses this code?
It was posted to the integrity list a while ago. I'm planning a repost
shortly.
James
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH 1/2] crypto: fix cfb mode decryption
On 21 October 2018 at 10:07, James Bottomley
wrote:
> On Sun, 2018-10-21 at 09:05 +0200, Ard Biesheuvel wrote:
>> (+ James)
>
> Thanks!
>
>> On 20 October 2018 at 01:01, Dmitry Eremin-Solenikov
>> wrote:
>> > crypto_cfb_decrypt_segment() incorrectly XOR'ed generated keystream
>> > with
>> > IV, rather than with data stream, resulting in incorrect
>> > decryption.
>> > Test vectors will be added in the next patch.
>> >
>> > Signed-off-by: Dmitry Eremin-Solenikov
>> > Cc: sta...@vger.kernel.org
>> > ---
>> > crypto/cfb.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/crypto/cfb.c b/crypto/cfb.c
>> > index a0d68c09e1b9..fd4e8500e121 100644
>> > --- a/crypto/cfb.c
>> > +++ b/crypto/cfb.c
>> > @@ -144,7 +144,7 @@ static int crypto_cfb_decrypt_segment(struct
>> > skcipher_walk *walk,
>> >
>> > do {
>> > crypto_cfb_encrypt_one(tfm, iv, dst);
>> > - crypto_xor(dst, iv, bsize);
>> > + crypto_xor(dst, src, bsize);
>
> This does look right. I think the reason the TPM code works is that it
> always does encrypt/decrypt in-place, which is a separate piece of the
> code which appears to be correct.
>
Yeah I figured that.
So where is the TPM code that actually uses this code?
Re: [PATCH 1/2] crypto: fix cfb mode decryption
On Sun, 2018-10-21 at 09:05 +0200, Ard Biesheuvel wrote:
> (+ James)
Thanks!
> On 20 October 2018 at 01:01, Dmitry Eremin-Solenikov
> wrote:
> > crypto_cfb_decrypt_segment() incorrectly XOR'ed generated keystream
> > with
> > IV, rather than with data stream, resulting in incorrect
> > decryption.
> > Test vectors will be added in the next patch.
> >
> > Signed-off-by: Dmitry Eremin-Solenikov
> > Cc: sta...@vger.kernel.org
> > ---
> > crypto/cfb.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/crypto/cfb.c b/crypto/cfb.c
> > index a0d68c09e1b9..fd4e8500e121 100644
> > --- a/crypto/cfb.c
> > +++ b/crypto/cfb.c
> > @@ -144,7 +144,7 @@ static int crypto_cfb_decrypt_segment(struct
> > skcipher_walk *walk,
> >
> > do {
> > crypto_cfb_encrypt_one(tfm, iv, dst);
> > - crypto_xor(dst, iv, bsize);
> > + crypto_xor(dst, src, bsize);
This does look right. I think the reason the TPM code works is that it
always does encrypt/decrypt in-place, which is a separate piece of the
code which appears to be correct.
James
Re: [PATCH 2/2] crypto: testmgr: add AES-CFB tests
(+ James)
On 20 October 2018 at 01:01, Dmitry Eremin-Solenikov
wrote:
> Add AES128/192/256-CFB testvectors from NIST SP800-38A.
>
> Signed-off-by: Dmitry Eremin-Solenikov
> Cc: sta...@vger.kernel.org
> Signed-off-by: Dmitry Eremin-Solenikov
> ---
> crypto/tcrypt.c | 5
> crypto/testmgr.c | 7 +
> crypto/testmgr.h | 76
> 3 files changed, 88 insertions(+)
>
> diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
> index bdde95e8d369..a6315827d240 100644
> --- a/crypto/tcrypt.c
> +++ b/crypto/tcrypt.c
> @@ -1733,6 +1733,7 @@ static int do_test(const char *alg, u32 type, u32 mask,
> int m, u32 num_mb)
> ret += tcrypt_test("xts(aes)");
> ret += tcrypt_test("ctr(aes)");
> ret += tcrypt_test("rfc3686(ctr(aes))");
> + ret += tcrypt_test("cfb(aes)");
> break;
>
> case 11:
> @@ -2059,6 +2060,10 @@ static int do_test(const char *alg, u32 type, u32
> mask, int m, u32 num_mb)
> speed_template_16_24_32);
> test_cipher_speed("ctr(aes)", DECRYPT, sec, NULL, 0,
> speed_template_16_24_32);
> + test_cipher_speed("cfb(aes)", ENCRYPT, sec, NULL, 0,
> + speed_template_16_24_32);
> + test_cipher_speed("cfb(aes)", DECRYPT, sec, NULL, 0,
> + speed_template_16_24_32);
> break;
>
> case 201:
> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> index a1d42245082a..016d61c419fc 100644
> --- a/crypto/testmgr.c
> +++ b/crypto/testmgr.c
> @@ -2684,6 +2684,13 @@ static const struct alg_test_desc alg_test_descs[] = {
> .dec = __VECS(aes_ccm_dec_tv_template)
> }
> }
> + }, {
> + .alg = "cfb(aes)",
> + .test = alg_test_skcipher,
> + .fips_allowed = 1,
> + .suite = {
> + .cipher = __VECS(aes_cfb_tv_template)
> + },
> }, {
> .alg = "chacha20",
> .test = alg_test_skcipher,
> diff --git a/crypto/testmgr.h b/crypto/testmgr.h
> index 173111c70746..19b6d184c8fb 100644
> --- a/crypto/testmgr.h
> +++ b/crypto/testmgr.h
> @@ -12081,6 +12081,82 @@ static const struct cipher_testvec
> aes_cbc_tv_template[] = {
> },
> };
>
> +static const struct cipher_testvec aes_cfb_tv_template[] = {
> + { /* From NIST SP800-38A */
> + .key= "\x2b\x7e\x15\x16\x28\xae\xd2\xa6"
> + "\xab\xf7\x15\x88\x09\xcf\x4f\x3c",
> + .klen = 16,
> + .iv = "\x00\x01\x02\x03\x04\x05\x06\x07"
> + "\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f",
> + .ptext = "\x6b\xc1\xbe\xe2\x2e\x40\x9f\x96"
> + "\xe9\x3d\x7e\x11\x73\x93\x17\x2a"
> + "\xae\x2d\x8a\x57\x1e\x03\xac\x9c"
> + "\x9e\xb7\x6f\xac\x45\xaf\x8e\x51"
> + "\x30\xc8\x1c\x46\xa3\x5c\xe4\x11"
> + "\xe5\xfb\xc1\x19\x1a\x0a\x52\xef"
> + "\xf6\x9f\x24\x45\xdf\x4f\x9b\x17"
> + "\xad\x2b\x41\x7b\xe6\x6c\x37\x10",
> + .ctext = "\x3b\x3f\xd9\x2e\xb7\x2d\xad\x20"
> + "\x33\x34\x49\xf8\xe8\x3c\xfb\x4a"
> + "\xc8\xa6\x45\x37\xa0\xb3\xa9\x3f"
> + "\xcd\xe3\xcd\xad\x9f\x1c\xe5\x8b"
> + "\x26\x75\x1f\x67\xa3\xcb\xb1\x40"
> + "\xb1\x80\x8c\xf1\x87\xa4\xf4\xdf"
> + "\xc0\x4b\x05\x35\x7c\x5d\x1c\x0e"
> + "\xea\xc4\xc6\x6f\x9f\xf7\xf2\xe6",
> + .len= 64,
> + }, {
> + .key= "\x8e\x73\xb0\xf7\xda\x0e\x64\x52"
> + "\xc8\x10\xf3\x2b\x80\x90\x79\xe5"
> + "\x62\xf8\xea\xd2\x52\x2c\x6b\x7b",
> + .klen = 24,
> + .iv = "\x00\x01\x02\x03\x04\x05\x06\x07"
> + "\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f",
> + .ptext = "\x6b\xc1\xbe\xe2\x2e\x40\x9f\x96"
> + "\xe9\x3d\x7e\x11\x73\x93\x17\x2a"
> + "\xae\x2d\x8a\x57\x1e\x03\xac\x9c"
> + "\x9e\xb7\x6f\xac\x45\xaf\x8e\x51"
> + "\x30\xc8\x1c\x46\xa3\x5c\xe4\x11"
> + "\xe5\xfb\xc1\x19\x1a\x0a\x52\xef"
> + "\xf6\x9f\x24\x45\xdf\x4f\x9b\x17"
> + "\xad\x2b\x41\x7b\xe6\x6c\x37\x10",
> + .ctext = "\xcd\xc8\x0d\x6f\xdd\xf1\x8c\xab"
> + "\x34\xc2\x59\x09\xc9\x9a\x41\x74"
> + "\x67\xce\x7f\x7f\x81\x17\x36\x21"
> + "\x96\x1a\x2b\x
RE: [PATCH 0/3] crypto: ccree: add SM3 support
Hi Herbert, I'm sorry for notifying just now, but this patch set should be applies on top of "crypto: ccree: add CryptoCell 713 baseline support" patch set by Gilad Ben-Yossef Hence failures reported by kbuild test robot . > -Original Message- > From: Gilad Ben-Yossef > Sent: Thursday, 18 October 2018 16:26 > To: yaeceh01 > Cc: Herbert Xu ; David Miller > ; Linux Crypto Mailing List cry...@vger.kernel.org>; Ofir Drang ; Linux kernel > mailing list > Subject: Re: [PATCH 0/3] crypto: ccree: add SM3 support > > On Thu, Oct 18, 2018 at 4:00 PM Yael Chemla > wrote: > > > > Add support for SM3 in CryptoCell 713. > > > > Yael Chemla (3): > > crypto: ccree: adjust hash length to suit certain context specifics > > crypto: ccree: modify set_cipher_mode usage from cc_hash > > crypto: ccree: add SM3 support > > > > drivers/crypto/Kconfig | 1 + > > drivers/crypto/ccree/cc_aead.c | 19 +++- > > drivers/crypto/ccree/cc_crypto_ctx.h| 4 +- > > drivers/crypto/ccree/cc_driver.c| 10 +- > > drivers/crypto/ccree/cc_driver.h| 2 +- > > drivers/crypto/ccree/cc_hash.c | 175 +++-- > --- > > drivers/crypto/ccree/cc_hw_queue_defs.h | 27 + > > 7 files changed, 182 insertions(+), 56 deletions(-) > > > > -- > > 2.7.4 > > > > Herbert, these go on top of my own previous patch set that adds the > CryptoCell 713 and its SM4 cipher support. > > Acked-by: Gilad Ben-Yossef > > Thanks, > Gilad > > -- > Gilad Ben-Yossef > Chief Coffee Drinker > > values of β will give rise to dom!
Re: [PATCH 1/2] crypto: fix cfb mode decryption
(+ James)
On 20 October 2018 at 01:01, Dmitry Eremin-Solenikov
wrote:
> crypto_cfb_decrypt_segment() incorrectly XOR'ed generated keystream with
> IV, rather than with data stream, resulting in incorrect decryption.
> Test vectors will be added in the next patch.
>
> Signed-off-by: Dmitry Eremin-Solenikov
> Cc: sta...@vger.kernel.org
> ---
> crypto/cfb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/crypto/cfb.c b/crypto/cfb.c
> index a0d68c09e1b9..fd4e8500e121 100644
> --- a/crypto/cfb.c
> +++ b/crypto/cfb.c
> @@ -144,7 +144,7 @@ static int crypto_cfb_decrypt_segment(struct
> skcipher_walk *walk,
>
> do {
> crypto_cfb_encrypt_one(tfm, iv, dst);
> - crypto_xor(dst, iv, bsize);
> + crypto_xor(dst, src, bsize);
> iv = src;
>
> src += bsize;
> --
> 2.19.1
>
