| Message ID | 1562309364-942-2-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:22AM +0200, Pascal van Leeuwen wrote: > Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com> Could you provide a commit message, explaining briefly what the patch is doing? > @@ -199,6 +201,15 @@ static int safexcel_aead_aes_setkey(struct crypto_aead *ctfm, const u8 *key, > goto badkey; > > /* Encryption key */ > + if (ctx->alg == SAFEXCEL_3DES) { > + flags = crypto_aead_get_flags(ctfm); > + err = __des3_verify_key(&flags, keys.enckey); > + crypto_aead_set_flags(ctfm, flags); You could use directly des3_verify_key() which does exactly this. > +struct safexcel_alg_template safexcel_alg_authenc_hmac_sha1_cbc_des3_ede = { > + .type = SAFEXCEL_ALG_TYPE_AEAD, You either missed to fill .engines member of this struct, or this series is based on another one not merged yet. > + .alg.aead = { > + .setkey = safexcel_aead_setkey, > + .encrypt = safexcel_aead_encrypt_3des, > + .decrypt = safexcel_aead_decrypt_3des, > + .ivsize = DES3_EDE_BLOCK_SIZE, > + .maxauthsize = SHA1_DIGEST_SIZE, > + .base = { > + .cra_name = "authenc(hmac(sha1),cbc(des3_ede))", > + .cra_driver_name = "safexcel-authenc-hmac-sha1-cbc-des3_ede", You could drop "_ede" here, or s/_/-/. Apart from those small comments, the patch looks good. 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 2:20 PM > To: Pascal van Leeuwen <pascalvanl@gmail.com> > Cc: linux-crypto@vger.kernel.org; antoine.tenart@bootlin.com; herbert@gondor.apana.org.au; davem@davemloft.net; Pascal Van > Leeuwen <pvanleeuwen@verimatrix.com> > Subject: Re: [PATCH 1/3] crypto: inside-secure - add support for authenc(hmac(sha1),cbc(des3_ede)) > > Hi Pascal, > > On Fri, Jul 05, 2019 at 08:49:22AM +0200, Pascal van Leeuwen wrote: > > Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com> > > Could you provide a commit message, explaining briefly what the patch is > doing? > I initially figured that to be redundant if the subject already covered it completely. But now that I think of it, it's possible the subject does not end up in the commit at all ... if that is the case, would it work if I just copy-paste the relevant part of the subject message? Or do I need to be more verbose? > > @@ -199,6 +201,15 @@ static int safexcel_aead_aes_setkey(struct crypto_aead *ctfm, const u8 *key, > > goto badkey; > > > > /* Encryption key */ > > + if (ctx->alg == SAFEXCEL_3DES) { > > + flags = crypto_aead_get_flags(ctfm); > > + err = __des3_verify_key(&flags, keys.enckey); > > + crypto_aead_set_flags(ctfm, flags); > > You could use directly des3_verify_key() which does exactly this. > Actually, I couldn't due to des3_verify_key expecting a struct crypto_skcipher as input, and not a struct crypto_aead, that's why I had to do it this way ... > > +struct safexcel_alg_template safexcel_alg_authenc_hmac_sha1_cbc_des3_ede = { > > + .type = SAFEXCEL_ALG_TYPE_AEAD, > > You either missed to fill .engines member of this struct, or this series > is based on another one not merged yet. > Yes, that happened in the patchset of which v2 did not make it to the mailing list ... > > + .alg.aead = { > > + .setkey = safexcel_aead_setkey, > > + .encrypt = safexcel_aead_encrypt_3des, > > + .decrypt = safexcel_aead_decrypt_3des, > > + .ivsize = DES3_EDE_BLOCK_SIZE, > > + .maxauthsize = SHA1_DIGEST_SIZE, > > + .base = { > > + .cra_name = "authenc(hmac(sha1),cbc(des3_ede))", > > + .cra_driver_name = "safexcel-authenc-hmac-sha1-cbc-des3_ede", > > You could drop "_ede" here, or s/_/-/. > Agree the underscore should not be there. Our HW does not support any other form of 3DES so EDE doesn't really add much here, therefore I will just remove "_ede" entirely. > Apart from those small comments, the patch looks good. > > 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 12:57:21PM +0000, Pascal Van Leeuwen wrote: > > On Fri, Jul 05, 2019 at 08:49:22AM +0200, Pascal van Leeuwen wrote: > > > Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com> > > > > Could you provide a commit message, explaining briefly what the patch is > > doing? > > > I initially figured that to be redundant if the subject already covered it completely. > But now that I think of it, it's possible the subject does not end up in the commit > at all ... if that is the case, would it work if I just copy-paste the relevant part of the > subject message? Or do I need to be more verbose? The subject will be the commit title. I know sometimes the commit message is trivial or redundant, but it's still a good practice to always have one (and many maintainers will ask for one). Even if it's only two lines :) > > > @@ -199,6 +201,15 @@ static int safexcel_aead_aes_setkey(struct crypto_aead *ctfm, const u8 *key, > > > goto badkey; > > > > > > /* Encryption key */ > > > + if (ctx->alg == SAFEXCEL_3DES) { > > > + flags = crypto_aead_get_flags(ctfm); > > > + err = __des3_verify_key(&flags, keys.enckey); > > > + crypto_aead_set_flags(ctfm, flags); > > > > You could use directly des3_verify_key() which does exactly this. > > > Actually, I couldn't due to des3_verify_key expecting a struct crypto_skcipher as input, > and not a struct crypto_aead, that's why I had to do it this way ... I see. Maybe a good way would be to provide a function taking 'struct crypto_aead' as an argument so that not every single driver reimplement the same logic. But this can come later if needed. > > > +struct safexcel_alg_template safexcel_alg_authenc_hmac_sha1_cbc_des3_ede = { > > > + .type = SAFEXCEL_ALG_TYPE_AEAD, > > > > You either missed to fill .engines member of this struct, or this series > > is based on another one not merged yet. > > > Yes, that happened in the patchset of which v2 did not make it to the mailing list ... :) So in general if there's a dependency you should say so in the cover letter. 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:07 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 1/3] crypto: inside-secure - add support for authenc(hmac(sha1),cbc(des3_ede)) > > Hi Pascal, > > On Fri, Jul 26, 2019 at 12:57:21PM +0000, Pascal Van Leeuwen wrote: > > > On Fri, Jul 05, 2019 at 08:49:22AM +0200, Pascal van Leeuwen wrote: > > > > Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com> > > > > > > Could you provide a commit message, explaining briefly what the patch is > > > doing? > > > > > I initially figured that to be redundant if the subject already covered it completely. > > But now that I think of it, it's possible the subject does not end up in the commit > > at all ... if that is the case, would it work if I just copy-paste the relevant part of the > > subject message? Or do I need to be more verbose? > > The subject will be the commit title. I know sometimes the commit > message is trivial or redundant, but it's still a good practice to > always have one (and many maintainers will ask for one). Even if it's > only two lines :) > Ok, good to know. I'm still learning how this works. I'll try and remember ;-) > > > > @@ -199,6 +201,15 @@ static int safexcel_aead_aes_setkey(struct crypto_aead *ctfm, const u8 *key, > > > > goto badkey; > > > > > > > > /* Encryption key */ > > > > + if (ctx->alg == SAFEXCEL_3DES) { > > > > + flags = crypto_aead_get_flags(ctfm); > > > > + err = __des3_verify_key(&flags, keys.enckey); > > > > + crypto_aead_set_flags(ctfm, flags); > > > > > > You could use directly des3_verify_key() which does exactly this. > > > > > Actually, I couldn't due to des3_verify_key expecting a struct crypto_skcipher as input, > > and not a struct crypto_aead, that's why I had to do it this way ... > > I see. Maybe a good way would be to provide a function taking > 'struct crypto_aead' as an argument so that not every single driver > reimplement the same logic. But this can come later if needed. > Agree. But being a newby and all, I did not dare to touch des.h itself ... > > > > +struct safexcel_alg_template safexcel_alg_authenc_hmac_sha1_cbc_des3_ede = { > > > > + .type = SAFEXCEL_ALG_TYPE_AEAD, > > > > > > You either missed to fill .engines member of this struct, or this series > > > is based on another one not merged yet. > > > > > Yes, that happened in the patchset of which v2 did not make it to the mailing list ... > > :) > > So in general if there's a dependency you should say so in the cover > letter. > I'll try to do that insofar I actually realise such a dependency exists ... > 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
> -----Original Message----- > From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of > Pascal Van Leeuwen > Sent: Friday, July 26, 2019 2:57 PM > To: Antoine Tenart <antoine.tenart@bootlin.com>; Pascal van Leeuwen <pascalvanl@gmail.com> > Cc: linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net > Subject: RE: [PATCH 1/3] crypto: inside-secure - add support for > authenc(hmac(sha1),cbc(des3_ede)) > > Antoine, > > > > > + .alg.aead = { > > > + .setkey = safexcel_aead_setkey, > > > + .encrypt = safexcel_aead_encrypt_3des, > > > + .decrypt = safexcel_aead_decrypt_3des, > > > + .ivsize = DES3_EDE_BLOCK_SIZE, > > > + .maxauthsize = SHA1_DIGEST_SIZE, > > > + .base = { > > > + .cra_name = "authenc(hmac(sha1),cbc(des3_ede))", > > > + .cra_driver_name = "safexcel-authenc-hmac-sha1-cbc-des3_ede", > > > > You could drop "_ede" here, or s/_/-/. > > > Agree the underscore should not be there. > Our HW does not support any other form of 3DES so EDE doesn't > really add much here, therefore I will just remove "_ede" entirely. > Actually, while looking into fixing this, I noticed that this naming style is actually consistent with the already existing 3des ecb and cbc ciphersuites, e.g.: "safexcel-cbc-des3_ebe", so for consistency I would then suggest keeping it (or change the other 2 3des references at the same time, but I don't know if that would break any legacy dependency). > > > Apart from those small comments, the patch looks good. > > > > 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 Tue, Jul 30, 2019 at 02:01:46PM +0000, Pascal Van Leeuwen wrote: > > From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of > > Pascal Van Leeuwen > > Sent: Friday, July 26, 2019 2:57 PM > > To: Antoine Tenart <antoine.tenart@bootlin.com>; Pascal van Leeuwen <pascalvanl@gmail.com> > > Cc: linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net > > Subject: RE: [PATCH 1/3] crypto: inside-secure - add support for > > authenc(hmac(sha1),cbc(des3_ede)) > > > > > > + .alg.aead = { > > > > + .setkey = safexcel_aead_setkey, > > > > + .encrypt = safexcel_aead_encrypt_3des, > > > > + .decrypt = safexcel_aead_decrypt_3des, > > > > + .ivsize = DES3_EDE_BLOCK_SIZE, > > > > + .maxauthsize = SHA1_DIGEST_SIZE, > > > > + .base = { > > > > + .cra_name = "authenc(hmac(sha1),cbc(des3_ede))", > > > > + .cra_driver_name = "safexcel-authenc-hmac-sha1-cbc-des3_ede", > > > > > > You could drop "_ede" here, or s/_/-/. > > > > > Agree the underscore should not be there. > > Our HW does not support any other form of 3DES so EDE doesn't > > really add much here, therefore I will just remove "_ede" entirely. > > > Actually, while looking into fixing this, I noticed that this > naming style is actually consistent with the already existing > 3des ecb and cbc ciphersuites, e.g.: "safexcel-cbc-des3_ebe", > so for consistency I would then suggest keeping it (or > change the other 2 3des references at the same time, but I > don't know if that would break any legacy dependency). Good catch :) As you said, you should keep it this way then. Thanks, Antoine
diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c index 8e8c01d..c3bb177 100644 --- a/drivers/crypto/inside-secure/safexcel.c +++ b/drivers/crypto/inside-secure/safexcel.c @@ -1004,6 +1004,7 @@ static int safexcel_request_ring_irq(void *pdev, int irqid, &safexcel_alg_authenc_hmac_sha256_cbc_aes, &safexcel_alg_authenc_hmac_sha384_cbc_aes, &safexcel_alg_authenc_hmac_sha512_cbc_aes, + &safexcel_alg_authenc_hmac_sha1_cbc_des3_ede, }; static int safexcel_register_algorithms(struct safexcel_crypto_priv *priv) diff --git a/drivers/crypto/inside-secure/safexcel.h b/drivers/crypto/inside-secure/safexcel.h index b68fec3..765f481 100644 --- a/drivers/crypto/inside-secure/safexcel.h +++ b/drivers/crypto/inside-secure/safexcel.h @@ -765,5 +765,6 @@ int safexcel_hmac_setkey(const char *alg, const u8 *key, unsigned int keylen, extern struct safexcel_alg_template safexcel_alg_authenc_hmac_sha256_cbc_aes; extern struct safexcel_alg_template safexcel_alg_authenc_hmac_sha384_cbc_aes; extern struct safexcel_alg_template safexcel_alg_authenc_hmac_sha512_cbc_aes; +extern struct safexcel_alg_template safexcel_alg_authenc_hmac_sha1_cbc_des3_ede; #endif diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c index ea122dd..5eed890 100644 --- a/drivers/crypto/inside-secure/safexcel_cipher.c +++ b/drivers/crypto/inside-secure/safexcel_cipher.c @@ -183,14 +183,16 @@ static int safexcel_skcipher_aes_setkey(struct crypto_skcipher *ctfm, return 0; } -static int safexcel_aead_aes_setkey(struct crypto_aead *ctfm, const u8 *key, - unsigned int len) +static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 *key, + unsigned int len) { struct crypto_tfm *tfm = crypto_aead_tfm(ctfm); struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm); struct safexcel_ahash_export_state istate, ostate; struct safexcel_crypto_priv *priv = ctx->priv; struct crypto_authenc_keys keys; + u32 flags; + int err; if (crypto_authenc_extractkeys(&keys, key, len) != 0) goto badkey; @@ -199,6 +201,15 @@ static int safexcel_aead_aes_setkey(struct crypto_aead *ctfm, const u8 *key, goto badkey; /* Encryption key */ + if (ctx->alg == SAFEXCEL_3DES) { + flags = crypto_aead_get_flags(ctfm); + err = __des3_verify_key(&flags, keys.enckey); + crypto_aead_set_flags(ctfm, flags); + + if (unlikely(err)) + return err; + } + if (priv->flags & EIP197_TRC_CACHE && ctx->base.ctxr_dma && memcmp(ctx->key, keys.enckey, keys.enckeylen)) ctx->base.needs_inv = true; @@ -1240,7 +1251,7 @@ struct safexcel_alg_template safexcel_alg_ecb_des3_ede = { }, }; -static int safexcel_aead_encrypt(struct aead_request *req) +static int safexcel_aead_encrypt_aes(struct aead_request *req) { struct safexcel_cipher_req *creq = aead_request_ctx(req); @@ -1248,7 +1259,7 @@ static int safexcel_aead_encrypt(struct aead_request *req) CONTEXT_CONTROL_CRYPTO_MODE_CBC, SAFEXCEL_AES); } -static int safexcel_aead_decrypt(struct aead_request *req) +static int safexcel_aead_decrypt_aes(struct aead_request *req) { struct safexcel_cipher_req *creq = aead_request_ctx(req); @@ -1287,9 +1298,9 @@ static int safexcel_aead_sha1_cra_init(struct crypto_tfm *tfm) struct safexcel_alg_template safexcel_alg_authenc_hmac_sha1_cbc_aes = { .type = SAFEXCEL_ALG_TYPE_AEAD, .alg.aead = { - .setkey = safexcel_aead_aes_setkey, - .encrypt = safexcel_aead_encrypt, - .decrypt = safexcel_aead_decrypt, + .setkey = safexcel_aead_setkey, + .encrypt = safexcel_aead_encrypt_aes, + .decrypt = safexcel_aead_decrypt_aes, .ivsize = AES_BLOCK_SIZE, .maxauthsize = SHA1_DIGEST_SIZE, .base = { @@ -1321,9 +1332,9 @@ static int safexcel_aead_sha256_cra_init(struct crypto_tfm *tfm) struct safexcel_alg_template safexcel_alg_authenc_hmac_sha256_cbc_aes = { .type = SAFEXCEL_ALG_TYPE_AEAD, .alg.aead = { - .setkey = safexcel_aead_aes_setkey, - .encrypt = safexcel_aead_encrypt, - .decrypt = safexcel_aead_decrypt, + .setkey = safexcel_aead_setkey, + .encrypt = safexcel_aead_encrypt_aes, + .decrypt = safexcel_aead_decrypt_aes, .ivsize = AES_BLOCK_SIZE, .maxauthsize = SHA256_DIGEST_SIZE, .base = { @@ -1355,9 +1366,9 @@ static int safexcel_aead_sha224_cra_init(struct crypto_tfm *tfm) struct safexcel_alg_template safexcel_alg_authenc_hmac_sha224_cbc_aes = { .type = SAFEXCEL_ALG_TYPE_AEAD, .alg.aead = { - .setkey = safexcel_aead_aes_setkey, - .encrypt = safexcel_aead_encrypt, - .decrypt = safexcel_aead_decrypt, + .setkey = safexcel_aead_setkey, + .encrypt = safexcel_aead_encrypt_aes, + .decrypt = safexcel_aead_decrypt_aes, .ivsize = AES_BLOCK_SIZE, .maxauthsize = SHA224_DIGEST_SIZE, .base = { @@ -1389,9 +1400,9 @@ static int safexcel_aead_sha512_cra_init(struct crypto_tfm *tfm) struct safexcel_alg_template safexcel_alg_authenc_hmac_sha512_cbc_aes = { .type = SAFEXCEL_ALG_TYPE_AEAD, .alg.aead = { - .setkey = safexcel_aead_aes_setkey, - .encrypt = safexcel_aead_encrypt, - .decrypt = safexcel_aead_decrypt, + .setkey = safexcel_aead_setkey, + .encrypt = safexcel_aead_encrypt_aes, + .decrypt = safexcel_aead_decrypt_aes, .ivsize = AES_BLOCK_SIZE, .maxauthsize = SHA512_DIGEST_SIZE, .base = { @@ -1423,9 +1434,9 @@ static int safexcel_aead_sha384_cra_init(struct crypto_tfm *tfm) struct safexcel_alg_template safexcel_alg_authenc_hmac_sha384_cbc_aes = { .type = SAFEXCEL_ALG_TYPE_AEAD, .alg.aead = { - .setkey = safexcel_aead_aes_setkey, - .encrypt = safexcel_aead_encrypt, - .decrypt = safexcel_aead_decrypt, + .setkey = safexcel_aead_setkey, + .encrypt = safexcel_aead_encrypt_aes, + .decrypt = safexcel_aead_decrypt_aes, .ivsize = AES_BLOCK_SIZE, .maxauthsize = SHA384_DIGEST_SIZE, .base = { @@ -1443,3 +1454,43 @@ struct safexcel_alg_template safexcel_alg_authenc_hmac_sha384_cbc_aes = { }, }, }; + +static int safexcel_aead_encrypt_3des(struct aead_request *req) +{ + struct safexcel_cipher_req *creq = aead_request_ctx(req); + + return safexcel_queue_req(&req->base, creq, SAFEXCEL_ENCRYPT, + CONTEXT_CONTROL_CRYPTO_MODE_CBC, SAFEXCEL_3DES); +} + +static int safexcel_aead_decrypt_3des(struct aead_request *req) +{ + struct safexcel_cipher_req *creq = aead_request_ctx(req); + + return safexcel_queue_req(&req->base, creq, SAFEXCEL_DECRYPT, + CONTEXT_CONTROL_CRYPTO_MODE_CBC, SAFEXCEL_3DES); +} + +struct safexcel_alg_template safexcel_alg_authenc_hmac_sha1_cbc_des3_ede = { + .type = SAFEXCEL_ALG_TYPE_AEAD, + .alg.aead = { + .setkey = safexcel_aead_setkey, + .encrypt = safexcel_aead_encrypt_3des, + .decrypt = safexcel_aead_decrypt_3des, + .ivsize = DES3_EDE_BLOCK_SIZE, + .maxauthsize = SHA1_DIGEST_SIZE, + .base = { + .cra_name = "authenc(hmac(sha1),cbc(des3_ede))", + .cra_driver_name = "safexcel-authenc-hmac-sha1-cbc-des3_ede", + .cra_priority = 300, + .cra_flags = CRYPTO_ALG_ASYNC | + CRYPTO_ALG_KERN_DRIVER_ONLY, + .cra_blocksize = DES3_EDE_BLOCK_SIZE, + .cra_ctxsize = sizeof(struct safexcel_cipher_ctx), + .cra_alignmask = 0, + .cra_init = safexcel_aead_sha1_cra_init, + .cra_exit = safexcel_aead_cra_exit, + .cra_module = THIS_MODULE, + }, + }, +};
Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com> --- drivers/crypto/inside-secure/safexcel.c | 1 + drivers/crypto/inside-secure/safexcel.h | 1 + drivers/crypto/inside-secure/safexcel_cipher.c | 89 ++++++++++++++++++++------ 3 files changed, 72 insertions(+), 19 deletions(-)