| Message ID | 1562309364-942-3-git-send-email-pvanleeuwen@verimatrix.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: inside-secure - add more AEAD ciphersuites | expand |
Hi Pascal, On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote: > Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com> Could you add a commit message? > - /* H/W capabilities selection */ > - val = EIP197_FUNCTION_RSVD; > - val |= EIP197_PROTOCOL_ENCRYPT_ONLY | EIP197_PROTOCOL_HASH_ONLY; > - val |= EIP197_PROTOCOL_ENCRYPT_HASH | EIP197_PROTOCOL_HASH_DECRYPT; > - val |= EIP197_ALG_DES_ECB | EIP197_ALG_DES_CBC; > - val |= EIP197_ALG_3DES_ECB | EIP197_ALG_3DES_CBC; > - val |= EIP197_ALG_AES_ECB | EIP197_ALG_AES_CBC; > - val |= EIP197_ALG_MD5 | EIP197_ALG_HMAC_MD5; > - val |= EIP197_ALG_SHA1 | EIP197_ALG_HMAC_SHA1; > - val |= EIP197_ALG_SHA2 | EIP197_ALG_HMAC_SHA2; > - writel(val, EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe)); > + /* H/W capabilities selection: just enable everything */ > + writel(EIP197_FUNCTION_ALL, > + EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe)); This should be in a separate patch. I'm also not sure about it, as controlling exactly what algs are enabled in the h/w could prevent misconfiguration issues in the control descriptors. > @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv, > u32 length) > - if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) { > + if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) { I think it's better for maintenance and readability to have something like: if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC || ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD) > +struct safexcel_alg_template safexcel_alg_ctr_aes = { > + .type = SAFEXCEL_ALG_TYPE_SKCIPHER, Same comment as in patch 1 about the .engines member of the struct. > + .alg.skcipher = { > + .setkey = safexcel_skcipher_aesctr_setkey, > + .encrypt = safexcel_ctr_aes_encrypt, > + .decrypt = safexcel_ctr_aes_decrypt, > + /* Add 4 to include the 4 byte nonce! */ > + .min_keysize = AES_MIN_KEY_SIZE + 4, > + .max_keysize = AES_MAX_KEY_SIZE + 4, You could use CTR_RFC3686_NONCE_SIZE here (maybe in other places in the patch as well). > + .ivsize = 8, And CTR_RFC3686_IV_SIZE here. Thanks! Antoine
Antoine, > Hi Pascal, > > On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote: > > Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com> > > Could you add a commit message? > I can do that. Just didn't know it was really needed. > > - /* H/W capabilities selection */ > > - val = EIP197_FUNCTION_RSVD; > > - val |= EIP197_PROTOCOL_ENCRYPT_ONLY | EIP197_PROTOCOL_HASH_ONLY; > > - val |= EIP197_PROTOCOL_ENCRYPT_HASH | EIP197_PROTOCOL_HASH_DECRYPT; > > - val |= EIP197_ALG_DES_ECB | EIP197_ALG_DES_CBC; > > - val |= EIP197_ALG_3DES_ECB | EIP197_ALG_3DES_CBC; > > - val |= EIP197_ALG_AES_ECB | EIP197_ALG_AES_CBC; > > - val |= EIP197_ALG_MD5 | EIP197_ALG_HMAC_MD5; > > - val |= EIP197_ALG_SHA1 | EIP197_ALG_HMAC_SHA1; > > - val |= EIP197_ALG_SHA2 | EIP197_ALG_HMAC_SHA2; > > - writel(val, EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe)); > > + /* H/W capabilities selection: just enable everything */ > > + writel(EIP197_FUNCTION_ALL, > > + EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe)); > > This should be in a separate patch. > It was added specifically to get this functionality working as CTR mode was not enabled. So I don't see why it should be a seperate patch, really? Instead of adding CTR mode to the list (which would have been perfectly valid in this context anyway), I just chose to enable everything instead. > I'm also not sure about it, as > controlling exactly what algs are enabled in the h/w could prevent > misconfiguration issues in the control descriptors. > That's not really what it's supposed to be used for. It's supposed to be used for disabling algorithms the application is not ALLOWED to use e.g. because they are not deemed secure enough (in case you have to comply, with, say, FIPS or so). As for misconfiguration: you may just as well hit another enabled algorithm, the odds of which will increase as I add more. And I'm very busy adding ALL of them, by which time you wouldn't want to disable any anyway. > > @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv, > > u32 length) > > - if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) { > > + if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) { > > I think it's better for maintenance and readability to have something > like: > > if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC || > ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD) > Not really. I *really* want to execute that for any mode other than ECB, ECB being the *only* mode that does not require an IV (which I know for a fact, being the architect and all :-). And I don't believe a long list of modes that *do* require an IV would be more readable or easy to maintain than this single compare ... > > +struct safexcel_alg_template safexcel_alg_ctr_aes = { > > + .type = SAFEXCEL_ALG_TYPE_SKCIPHER, > > Same comment as in patch 1 about the .engines member of the struct. > Same answer: depends on the disappearing patch I will resend shortly. > > + .alg.skcipher = { > > + .setkey = safexcel_skcipher_aesctr_setkey, > > + .encrypt = safexcel_ctr_aes_encrypt, > > + .decrypt = safexcel_ctr_aes_decrypt, > > + /* Add 4 to include the 4 byte nonce! */ > > + .min_keysize = AES_MIN_KEY_SIZE + 4, > > + .max_keysize = AES_MAX_KEY_SIZE + 4, > > You could use CTR_RFC3686_NONCE_SIZE here (maybe in other places in the > patch as well). > Makes sense. I did not know such a constant existed already. > > + .ivsize = 8, > > And CTR_RFC3686_IV_SIZE here. > Idem > Thanks! > Antoine > > -- > Antoine Ténart, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com Regards, Pascal van Leeuwen Silicon IP Architect, Multi-Protocol Engines @ Verimatrix www.insidesecure.com
Hi Pascal, On Fri, Jul 26, 2019 at 01:28:13PM +0000, Pascal Van Leeuwen wrote: > > On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote: > > > > - /* H/W capabilities selection */ > > > - val = EIP197_FUNCTION_RSVD; > > > - val |= EIP197_PROTOCOL_ENCRYPT_ONLY | EIP197_PROTOCOL_HASH_ONLY; > > > - val |= EIP197_PROTOCOL_ENCRYPT_HASH | EIP197_PROTOCOL_HASH_DECRYPT; > > > - val |= EIP197_ALG_DES_ECB | EIP197_ALG_DES_CBC; > > > - val |= EIP197_ALG_3DES_ECB | EIP197_ALG_3DES_CBC; > > > - val |= EIP197_ALG_AES_ECB | EIP197_ALG_AES_CBC; > > > - val |= EIP197_ALG_MD5 | EIP197_ALG_HMAC_MD5; > > > - val |= EIP197_ALG_SHA1 | EIP197_ALG_HMAC_SHA1; > > > - val |= EIP197_ALG_SHA2 | EIP197_ALG_HMAC_SHA2; > > > - writel(val, EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe)); > > > + /* H/W capabilities selection: just enable everything */ > > > + writel(EIP197_FUNCTION_ALL, > > > + EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe)); > > > > This should be in a separate patch. > > > It was added specifically to get this functionality working as CTR mode was not > enabled. So I don't see why it should be a seperate patch, really? > Instead of adding CTR mode to the list (which would have been perfectly valid > in this context anyway), I just chose to enable everything instead. Because it's not the same thing to enable everything and to add one extra alg. This makes bissecting the driver harder because one would not except this change in this kind of patch. > > I'm also not sure about it, as > > controlling exactly what algs are enabled in the h/w could prevent > > misconfiguration issues in the control descriptors. > > > That's not really what it's supposed to be used for. It's supposed to be used for > disabling algorithms the application is not ALLOWED to use e.g. because they are > not deemed secure enough (in case you have to comply, with, say, FIPS or so). OK, that answer my question and this makes sense. > > > @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv, > > > u32 length) > > > - if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) { > > > + if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) { > > > > I think it's better for maintenance and readability to have something > > like: > > > > if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC || > > ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD) > > > Not really. I *really* want to execute that for any mode other than ECB, > ECB being the *only* mode that does not require an IV (which I know > for a fact, being the architect and all :-). > And I don't believe a long list of modes that *do* require an IV would > be more readable or easy to maintain than this single compare ... That's where I disagree as you need extra knowledge to be aware of this. Being explicit removes any possible question one may ask. But that's a small point really :) Thanks! Antoine
Antoine, > -----Original Message----- > From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of Antoine Tenart > Sent: Friday, July 26, 2019 3:47 PM > To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com> > Cc: Antoine Tenart <antoine.tenart@bootlin.com>; Pascal van Leeuwen <pascalvanl@gmail.com>; linux-crypto@vger.kernel.org; > herbert@gondor.apana.org.au; davem@davemloft.net > Subject: Re: [PATCH 2/3] crypto: inside-secure - added support for rfc3686(ctr(aes)) > > Hi Pascal, > > On Fri, Jul 26, 2019 at 01:28:13PM +0000, Pascal Van Leeuwen wrote: > > > On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote: > > > > > > @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv, > > > > u32 length) > > > > - if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) { > > > > + if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) { > > > > > > I think it's better for maintenance and readability to have something > > > like: > > > > > > if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC || > > > ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD) > > > > > Not really. I *really* want to execute that for any mode other than ECB, > > ECB being the *only* mode that does not require an IV (which I know > > for a fact, being the architect and all :-). > > And I don't believe a long list of modes that *do* require an IV would > > be more readable or easy to maintain than this single compare ... > > That's where I disagree as you need extra knowledge to be aware of this. > Being explicit removes any possible question one may ask. But that's a > small point really :) > Well, while we're disagreeing ... I disagree with your assertion that you would need more knowledge to know which modes do NOT need an IV than to know which modes DO need an IV. There's really no fundamental difference, it's two sides of the exact same coin ... you need that knowledge either way. That being said: 1) This code is executed for each individual cipher call, i.e. it's in the critical path. Having just 1 compare-and-branch there is better for performance than having many. 2) Generally, all else being equal, having less code is easier to maintain than having more code. 3) Stuffing an IV in the token while you don't need it is harmless (apart from wasting some cycles). NOT stuffing an IV in the token while you DO need it is fatal. i.e. the single compare always errs on the safe side ... 4) If there is anything unclear about an otherwise fine code construct, then you clarify it by adding a comment, not by rewriting it to be inefficient and redundant ;-) > Thanks! > Antoine > > -- > Antoine Ténart, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com Regards, Pascal van Leeuwen Silicon IP Architect, Multi-Protocol Engines @ Verimatrix www.insidesecure.com
On Fri, Jul 26, 2019 at 02:29:48PM +0000, Pascal Van Leeuwen wrote: > > From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of Antoine Tenart > > On Fri, Jul 26, 2019 at 01:28:13PM +0000, Pascal Van Leeuwen wrote: > > > > On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote: > > > > > > > > @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv, > > > > > u32 length) > > > > > - if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) { > > > > > + if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) { > > > > > > > > I think it's better for maintenance and readability to have something > > > > like: > > > > > > > > if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC || > > > > ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD) > > > > > > > Not really. I *really* want to execute that for any mode other than ECB, > > > ECB being the *only* mode that does not require an IV (which I know > > > for a fact, being the architect and all :-). > > > And I don't believe a long list of modes that *do* require an IV would > > > be more readable or easy to maintain than this single compare ... > > > > That's where I disagree as you need extra knowledge to be aware of this. > > Being explicit removes any possible question one may ask. But that's a > > small point really :) > > > Well, while we're disagreeing ... I disagree with your assertion that you > would need more knowledge to know which modes do NOT need an IV > than to know which modes DO need an IV. There's really no fundamental > difference, it's two sides of the exact same coin ... you need that > knowledge either way. The point is if you look for occurrences of, let's say CONTEXT_CONTROL_CRYPTO_MODE_CBC, to see the code path it'll be way easier if you have direct comparisons. > 1) This code is executed for each individual cipher call, i.e. it's in the > critical path. Having just 1 compare-and-branch there is better for > performance than having many. Not sure about what the impact really is. > 2) Generally, all else being equal, having less code is easier to maintain > than having more code. That really depends, having readable code is easier to maintain :) > 4) If there is anything unclear about an otherwise fine code construct, > then you clarify it by adding a comment, not by rewriting it to be inefficient > and redundant ;-) Fair point. Thanks, Antoine
> -----Original Message----- > From: Antoine Tenart <antoine.tenart@bootlin.com> > Sent: Tuesday, July 30, 2019 10:24 AM > To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com> > Cc: Antoine Tenart <antoine.tenart@bootlin.com>; Pascal van Leeuwen > <pascalvanl@gmail.com>; linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au; > davem@davemloft.net > Subject: Re: [PATCH 2/3] crypto: inside-secure - added support for rfc3686(ctr(aes)) > > On Fri, Jul 26, 2019 at 02:29:48PM +0000, Pascal Van Leeuwen wrote: > > > From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On > Behalf Of Antoine Tenart > > > On Fri, Jul 26, 2019 at 01:28:13PM +0000, Pascal Van Leeuwen wrote: > > > > > On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote: > > > > > > > > > > @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx > *ctx, u8 *iv, > > > > > > u32 length) > > > > > > - if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) { > > > > > > + if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) { > > > > > > > > > > I think it's better for maintenance and readability to have something > > > > > like: > > > > > > > > > > if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC || > > > > > ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD) > > > > > > > > > Not really. I *really* want to execute that for any mode other than ECB, > > > > ECB being the *only* mode that does not require an IV (which I know > > > > for a fact, being the architect and all :-). > > > > And I don't believe a long list of modes that *do* require an IV would > > > > be more readable or easy to maintain than this single compare ... > > > > > > That's where I disagree as you need extra knowledge to be aware of this. > > > Being explicit removes any possible question one may ask. But that's a > > > small point really :) > > > > > Well, while we're disagreeing ... I disagree with your assertion that you > > would need more knowledge to know which modes do NOT need an IV > > than to know which modes DO need an IV. There's really no fundamental > > difference, it's two sides of the exact same coin ... you need that > > knowledge either way. > > The point is if you look for occurrences of, let's say > CONTEXT_CONTROL_CRYPTO_MODE_CBC, to see the code path it'll be way > easier if you have direct comparisons. > Not a good enough reason considering the downsides of having less performant code that is more difficult to maintain, IMHO (you will have to keep adding modes as we have plenty more and may add even more to the hardware later - and make sure the list is and stays complete going forward). So how often do you really need to do that search? Speaking as someone who has been adding new modes fairly recently but never had any need for that ... > > 1) This code is executed for each individual cipher call, i.e. it's in the > > critical path. Having just 1 compare-and-branch there is better for > > performance than having many. > > Not sure about what the impact really is. > Well, the driver is not exactly extracting maximum performance from our engine for small blocks at the moment, so I would say any cycle gained matters. Software overhead is the Achilles heel of hardware acceleration. And if you don't accelerate, your hardware is defacto useless. So anything in the critical path should be carefully considered. (which is also why I object so strongly to all these API corner cases we're forced to support, but now I digress ...) You can debate about significance, but at some point it all adds up. Better to not add the cycles - if it's no effort at all! - then to have to optimize them away later. Speaking as someone who has spent a major part of his professional life squeezing out those cycles to meet very real and hard performance requirements. > > 2) Generally, all else being equal, having less code is easier to maintain > > than having more code. > > That really depends, having readable code is easier to maintain :) > But how would it be more readable? "Do this for any mode other than ECB" seems pretty clear and readable to me? And is exactly what should happen here. Also, less code of the same complexity level (i.e. the same simple compares!) is still more readable than more code. > > 4) If there is anything unclear about an otherwise fine code construct, > > then you clarify it by adding a comment, not by rewriting it to be inefficient > > and redundant ;-) > > Fair point. > > Thanks, > Antoine > > -- > Antoine Ténart, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com Regards, Pascal van Leeuwen Silicon IP Architect, Multi-Protocol Engines @ Verimatrix www.insidesecure.com
diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c index c3bb177..26f086b 100644 --- a/drivers/crypto/inside-secure/safexcel.c +++ b/drivers/crypto/inside-secure/safexcel.c @@ -506,17 +506,9 @@ static int safexcel_hw_init(struct safexcel_crypto_priv *priv) EIP197_PE_EIP96_TOKEN_CTRL_POST_REUSE_CTX; writel(val, EIP197_PE(priv) + EIP197_PE_EIP96_TOKEN_CTRL(pe)); - /* H/W capabilities selection */ - val = EIP197_FUNCTION_RSVD; - val |= EIP197_PROTOCOL_ENCRYPT_ONLY | EIP197_PROTOCOL_HASH_ONLY; - val |= EIP197_PROTOCOL_ENCRYPT_HASH | EIP197_PROTOCOL_HASH_DECRYPT; - val |= EIP197_ALG_DES_ECB | EIP197_ALG_DES_CBC; - val |= EIP197_ALG_3DES_ECB | EIP197_ALG_3DES_CBC; - val |= EIP197_ALG_AES_ECB | EIP197_ALG_AES_CBC; - val |= EIP197_ALG_MD5 | EIP197_ALG_HMAC_MD5; - val |= EIP197_ALG_SHA1 | EIP197_ALG_HMAC_SHA1; - val |= EIP197_ALG_SHA2 | EIP197_ALG_HMAC_SHA2; - writel(val, EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe)); + /* H/W capabilities selection: just enable everything */ + writel(EIP197_FUNCTION_ALL, + EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe)); } /* Command Descriptor Rings prepare */ @@ -987,6 +979,7 @@ static int safexcel_request_ring_irq(void *pdev, int irqid, &safexcel_alg_cbc_des3_ede, &safexcel_alg_ecb_aes, &safexcel_alg_cbc_aes, + &safexcel_alg_ctr_aes, &safexcel_alg_md5, &safexcel_alg_sha1, &safexcel_alg_sha224, diff --git a/drivers/crypto/inside-secure/safexcel.h b/drivers/crypto/inside-secure/safexcel.h index 765f481..af71120 100644 --- a/drivers/crypto/inside-secure/safexcel.h +++ b/drivers/crypto/inside-secure/safexcel.h @@ -279,35 +279,7 @@ #define EIP197_PE_EIP96_TOKEN_CTRL_POST_REUSE_CTX BIT(20) /* EIP197_PE_EIP96_FUNCTION_EN */ -#define EIP197_FUNCTION_RSVD (BIT(6) | BIT(15) | BIT(20) | BIT(23)) -#define EIP197_PROTOCOL_HASH_ONLY BIT(0) -#define EIP197_PROTOCOL_ENCRYPT_ONLY BIT(1) -#define EIP197_PROTOCOL_HASH_ENCRYPT BIT(2) -#define EIP197_PROTOCOL_HASH_DECRYPT BIT(3) -#define EIP197_PROTOCOL_ENCRYPT_HASH BIT(4) -#define EIP197_PROTOCOL_DECRYPT_HASH BIT(5) -#define EIP197_ALG_ARC4 BIT(7) -#define EIP197_ALG_AES_ECB BIT(8) -#define EIP197_ALG_AES_CBC BIT(9) -#define EIP197_ALG_AES_CTR_ICM BIT(10) -#define EIP197_ALG_AES_OFB BIT(11) -#define EIP197_ALG_AES_CFB BIT(12) -#define EIP197_ALG_DES_ECB BIT(13) -#define EIP197_ALG_DES_CBC BIT(14) -#define EIP197_ALG_DES_OFB BIT(16) -#define EIP197_ALG_DES_CFB BIT(17) -#define EIP197_ALG_3DES_ECB BIT(18) -#define EIP197_ALG_3DES_CBC BIT(19) -#define EIP197_ALG_3DES_OFB BIT(21) -#define EIP197_ALG_3DES_CFB BIT(22) -#define EIP197_ALG_MD5 BIT(24) -#define EIP197_ALG_HMAC_MD5 BIT(25) -#define EIP197_ALG_SHA1 BIT(26) -#define EIP197_ALG_HMAC_SHA1 BIT(27) -#define EIP197_ALG_SHA2 BIT(28) -#define EIP197_ALG_HMAC_SHA2 BIT(29) -#define EIP197_ALG_AES_XCBC_MAC BIT(30) -#define EIP197_ALG_GCM_HASH BIT(31) +#define EIP197_FUNCTION_ALL 0xffffffff /* EIP197_PE_EIP96_CONTEXT_CTRL */ #define EIP197_CONTEXT_SIZE(n) (n) @@ -356,6 +328,7 @@ struct safexcel_context_record { /* control1 */ #define CONTEXT_CONTROL_CRYPTO_MODE_ECB (0 << 0) #define CONTEXT_CONTROL_CRYPTO_MODE_CBC (1 << 0) +#define CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD (6 << 0) #define CONTEXT_CONTROL_IV0 BIT(5) #define CONTEXT_CONTROL_IV1 BIT(6) #define CONTEXT_CONTROL_IV2 BIT(7) @@ -748,6 +721,7 @@ int safexcel_hmac_setkey(const char *alg, const u8 *key, unsigned int keylen, extern struct safexcel_alg_template safexcel_alg_cbc_des3_ede; extern struct safexcel_alg_template safexcel_alg_ecb_aes; extern struct safexcel_alg_template safexcel_alg_cbc_aes; +extern struct safexcel_alg_template safexcel_alg_ctr_aes; extern struct safexcel_alg_template safexcel_alg_md5; extern struct safexcel_alg_template safexcel_alg_sha1; extern struct safexcel_alg_template safexcel_alg_sha224; diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c index 5eed890..91945b1 100644 --- a/drivers/crypto/inside-secure/safexcel_cipher.c +++ b/drivers/crypto/inside-secure/safexcel_cipher.c @@ -40,6 +40,7 @@ struct safexcel_cipher_ctx { bool aead; __le32 key[8]; + u32 nonce; unsigned int key_len; /* All the below is AEAD specific */ @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv, u32 length) { struct safexcel_token *token; - u32 offset = 0, block_sz = 0; + u32 block_sz = 0; - if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) { + if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) { switch (ctx->alg) { case SAFEXCEL_DES: block_sz = DES_BLOCK_SIZE; @@ -80,11 +81,20 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv, break; } - offset = block_sz / sizeof(u32); - memcpy(cdesc->control_data.token, iv, block_sz); + if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD) { + /* 32 bit nonce */ + cdesc->control_data.token[0] = ctx->nonce; + /* 64 bit IV part */ + memcpy(&cdesc->control_data.token[1], iv, 8); + /* 32 bit counter, start at 1 (big endian!) */ + cdesc->control_data.token[3] = cpu_to_be32(1); + } else { + memcpy(cdesc->control_data.token, iv, block_sz); + } } - token = (struct safexcel_token *)(cdesc->control_data.token + offset); + /* skip over worst case IV of 4 dwords, no need to be exact */ + token = (struct safexcel_token *)(cdesc->control_data.token + 4); token[0].opcode = EIP197_TOKEN_OPCODE_DIRECTION; token[0].packet_length = length; @@ -101,33 +111,35 @@ static void safexcel_aead_token(struct safexcel_cipher_ctx *ctx, u8 *iv, u32 cryptlen, u32 assoclen, u32 digestsize) { struct safexcel_token *token; - unsigned offset = 0; + u32 block_sz = 0; - if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) { - offset = AES_BLOCK_SIZE / sizeof(u32); - memcpy(cdesc->control_data.token, iv, AES_BLOCK_SIZE); + if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) { + switch (ctx->alg) { + case SAFEXCEL_DES: + block_sz = DES_BLOCK_SIZE; + cdesc->control_data.options |= EIP197_OPTION_2_TOKEN_IV_CMD; + break; + case SAFEXCEL_3DES: + block_sz = DES3_EDE_BLOCK_SIZE; + cdesc->control_data.options |= EIP197_OPTION_2_TOKEN_IV_CMD; + break; + case SAFEXCEL_AES: + block_sz = AES_BLOCK_SIZE; + cdesc->control_data.options |= EIP197_OPTION_4_TOKEN_IV_CMD; + break; + } - cdesc->control_data.options |= EIP197_OPTION_4_TOKEN_IV_CMD; + memcpy(cdesc->control_data.token, iv, block_sz); } - token = (struct safexcel_token *)(cdesc->control_data.token + offset); - if (direction == SAFEXCEL_DECRYPT) cryptlen -= digestsize; - token[0].opcode = EIP197_TOKEN_OPCODE_DIRECTION; - token[0].packet_length = assoclen; - token[0].instructions = EIP197_TOKEN_INS_TYPE_HASH; - - token[1].opcode = EIP197_TOKEN_OPCODE_DIRECTION; - token[1].packet_length = cryptlen; - token[1].stat = EIP197_TOKEN_STAT_LAST_HASH; - token[1].instructions = EIP197_TOKEN_INS_LAST | - EIP197_TOKEN_INS_TYPE_CRYPTO | - EIP197_TOKEN_INS_TYPE_HASH | - EIP197_TOKEN_INS_TYPE_OUTPUT; - if (direction == SAFEXCEL_ENCRYPT) { + /* align end of instruction sequence to end of token */ + token = (struct safexcel_token *)(cdesc->control_data.token + + EIP197_MAX_TOKENS - 3); + token[2].opcode = EIP197_TOKEN_OPCODE_INSERT; token[2].packet_length = digestsize; token[2].stat = EIP197_TOKEN_STAT_LAST_HASH | @@ -135,6 +147,10 @@ static void safexcel_aead_token(struct safexcel_cipher_ctx *ctx, u8 *iv, token[2].instructions = EIP197_TOKEN_INS_TYPE_OUTPUT | EIP197_TOKEN_INS_INSERT_HASH_DIGEST; } else { + /* align end of instruction sequence to end of token */ + token = (struct safexcel_token *)(cdesc->control_data.token + + EIP197_MAX_TOKENS - 4); + token[2].opcode = EIP197_TOKEN_OPCODE_RETRIEVE; token[2].packet_length = digestsize; token[2].stat = EIP197_TOKEN_STAT_LAST_HASH | @@ -148,6 +164,19 @@ static void safexcel_aead_token(struct safexcel_cipher_ctx *ctx, u8 *iv, EIP197_TOKEN_STAT_LAST_PACKET; token[3].instructions = EIP197_TOKEN_INS_TYPE_OUTPUT; } + + token[0].opcode = EIP197_TOKEN_OPCODE_DIRECTION; + token[0].packet_length = assoclen; + token[0].instructions = EIP197_TOKEN_INS_TYPE_HASH; + + token[1].opcode = EIP197_TOKEN_OPCODE_DIRECTION; + token[1].packet_length = cryptlen; + token[1].stat = EIP197_TOKEN_STAT_LAST_HASH; + token[1].instructions = EIP197_TOKEN_INS_LAST | + EIP197_TOKEN_INS_TYPE_CRYPTO | + EIP197_TOKEN_INS_TYPE_HASH | + EIP197_TOKEN_INS_TYPE_OUTPUT; + } static int safexcel_skcipher_aes_setkey(struct crypto_skcipher *ctfm, @@ -1044,6 +1073,84 @@ struct safexcel_alg_template safexcel_alg_cbc_aes = { }, }; +static int safexcel_ctr_aes_encrypt(struct skcipher_request *req) +{ + return safexcel_queue_req(&req->base, skcipher_request_ctx(req), + SAFEXCEL_ENCRYPT, CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD, + SAFEXCEL_AES); +} + +static int safexcel_ctr_aes_decrypt(struct skcipher_request *req) +{ + return safexcel_queue_req(&req->base, skcipher_request_ctx(req), + SAFEXCEL_DECRYPT, CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD, + SAFEXCEL_AES); +} + +static int safexcel_skcipher_aesctr_setkey(struct crypto_skcipher *ctfm, + const u8 *key, unsigned int len) +{ + struct crypto_tfm *tfm = crypto_skcipher_tfm(ctfm); + struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm); + struct safexcel_crypto_priv *priv = ctx->priv; + struct crypto_aes_ctx aes; + int ret, i; + unsigned int keylen; + + /* last 4 bytes of key are the nonce! */ + ctx->nonce = *(u32 *)(key + len - 4); + /* exclude the nonce here */ + keylen = len - 4; + ret = crypto_aes_expand_key(&aes, key, keylen); + if (ret) { + crypto_skcipher_set_flags(ctfm, CRYPTO_TFM_RES_BAD_KEY_LEN); + return ret; + } + + if (priv->flags & EIP197_TRC_CACHE && ctx->base.ctxr_dma) { + for (i = 0; i < keylen / sizeof(u32); i++) { + if (ctx->key[i] != cpu_to_le32(aes.key_enc[i])) { + ctx->base.needs_inv = true; + break; + } + } + } + + for (i = 0; i < keylen / sizeof(u32); i++) + ctx->key[i] = cpu_to_le32(aes.key_enc[i]); + + ctx->key_len = keylen; + + memzero_explicit(&aes, sizeof(aes)); + return 0; +} + +struct safexcel_alg_template safexcel_alg_ctr_aes = { + .type = SAFEXCEL_ALG_TYPE_SKCIPHER, + .alg.skcipher = { + .setkey = safexcel_skcipher_aesctr_setkey, + .encrypt = safexcel_ctr_aes_encrypt, + .decrypt = safexcel_ctr_aes_decrypt, + /* Add 4 to include the 4 byte nonce! */ + .min_keysize = AES_MIN_KEY_SIZE + 4, + .max_keysize = AES_MAX_KEY_SIZE + 4, + .ivsize = 8, + .base = { + .cra_name = "rfc3686(ctr(aes))", + .cra_driver_name = "safexcel-ctr-aes", + .cra_priority = 300, + .cra_flags = CRYPTO_ALG_ASYNC | + CRYPTO_ALG_KERN_DRIVER_ONLY, + .cra_blocksize = 1, + .cra_ctxsize = sizeof(struct safexcel_cipher_ctx), + .cra_alignmask = 0, + .cra_init = safexcel_skcipher_cra_init, + .cra_exit = safexcel_skcipher_cra_exit, + .cra_module = THIS_MODULE, + }, + }, +}; + static int safexcel_cbc_des_encrypt(struct skcipher_request *req) { return safexcel_queue_req(&req->base, skcipher_request_ctx(req),
Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com> --- drivers/crypto/inside-secure/safexcel.c | 15 +-- drivers/crypto/inside-secure/safexcel.h | 32 +---- drivers/crypto/inside-secure/safexcel_cipher.c | 155 +++++++++++++++++++++---- 3 files changed, 138 insertions(+), 64 deletions(-)