From: Gary R Hook Subject: Re: [PATCH V2 2/3] crypto: ccp - Enable support for AES GCM on v5 CCPs Date: Tue, 14 Mar 2017 09:34:00 -0500 Message-ID: <58e1a934-4007-1ac3-8588-8267436c62b6@amd.com> References: <20170310180341.21062.82465.stgit@taos> <4585707.bK1Ho6b7OP@tauon.atsec.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 8bit Cc: "linux-crypto@vger.kernel.org" To: =?UTF-8?Q?Stephan_M=c3=bcller?= , "Hook, Gary" Return-path: Received: from mail-sn1nam02on0070.outbound.protection.outlook.com ([104.47.36.70]:62016 "EHLO NAM02-SN1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750909AbdCNOeQ (ORCPT ); Tue, 14 Mar 2017 10:34:16 -0400 In-Reply-To: <4585707.bK1Ho6b7OP@tauon.atsec.com> Sender: linux-crypto-owner@vger.kernel.org List-ID: On 03/14/2017 02:17 AM, Stephan M?ller wrote: > Am Montag, 13. M?rz 2017, 20:35:07 CET schrieb Gary R Hook: > > Hi Gary, Is it acceptable to snip stuff out? Most of this code seems irrelevant to this discussion.... > >> On 03/03/2017 7:15 AM, Stephan Mueller wrote: >> > Am Donnerstag, 2. M?rz 2017, 22:26:54 CET schrieb Gary R Hook: >> > >> > Hi Gary, >> >> Thanks for your comments, Stephan. >> >> > > A version 5 device provides the primitive commands >> > > required for AES GCM. This patch adds support for >> > > en/decryption. >> > > >> > > Signed-off-by: Gary R Hook >> > > --- >> > > >> > > drivers/crypto/ccp/Makefile | 1 >> > > drivers/crypto/ccp/ccp-crypto-aes-galois.c | 257 >> > > >> > > ++++++++++++++++++++++++++++ drivers/crypto/ccp/ccp-crypto-main.c >> > > | >> > > 12 + >> > > >> > > drivers/crypto/ccp/ccp-crypto.h | 14 ++ >> > > drivers/crypto/ccp/ccp-dev-v5.c | 2 >> > > drivers/crypto/ccp/ccp-dev.h | 1 >> > > drivers/crypto/ccp/ccp-ops.c | 252 >> > > >> > > +++++++++++++++++++++++++++ include/linux/ccp.h | >> > > 9 + >> > > >> > > 8 files changed, 548 insertions(+) >> > > create mode 100644 drivers/crypto/ccp/ccp-crypto-aes-galois.c >> > > >> > > diff --git a/drivers/crypto/ccp/ccp-crypto-aes-galois.c >> > > b/drivers/crypto/ccp/ccp-crypto-aes-galois.c new file mode 100644 >> > > index 0000000..8bc18c9 >> > > --- /dev/null >> > > +++ b/drivers/crypto/ccp/ccp-crypto-aes-galois.c >> > > @@ -0,0 +1,257 @@ >> > > +/* >> > > + * AMD Cryptographic Coprocessor (CCP) AES GCM crypto API support >> > > + * >> > > + * Copyright (C) 2016 Advanced Micro Devices, Inc. >> > > + * >> > > + * Author: Gary R Hook >> > > + * >> > > + * This program is free software; you can redistribute it and/or modify >> > > + * it under the terms of the GNU General Public License version 2 as >> > > + * published by the Free Software Foundation. >> > > + */ >> > > + >> > > +#include >> > > +#include >> > > +#include >> > > +#include >> > > +#include >> > > +#include >> > > +#include >> > > +#include >> > > +#include >> > > +#include >> > > +#include >> > > + >> > > +#include "ccp-crypto.h" >> > > + >> > > +#define AES_GCM_IVSIZE 12 >> > > + >> > > +static int ccp_aes_gcm_complete(struct crypto_async_request *async_req, >> > > int ret) +{ >> > > + return ret; >> > > +} >> > > + >> > > +static int ccp_aes_gcm_setkey(struct crypto_aead *tfm, const u8 *key, >> > > + unsigned int key_len) >> > > +{ >> > > + struct ccp_ctx *ctx = crypto_aead_ctx(tfm); >> > > + >> > > + switch (key_len) { >> > > + case AES_KEYSIZE_128: >> > > + ctx->u.aes.type = CCP_AES_TYPE_128; >> > > + break; >> > > + case AES_KEYSIZE_192: >> > > + ctx->u.aes.type = CCP_AES_TYPE_192; >> > > + break; >> > > + case AES_KEYSIZE_256: >> > > + ctx->u.aes.type = CCP_AES_TYPE_256; >> > > + break; >> > > + default: >> > > + crypto_aead_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN); >> > > + return -EINVAL; >> > > + } >> > > + >> > > + ctx->u.aes.mode = CCP_AES_MODE_GCM; >> > > + ctx->u.aes.key_len = key_len; >> > > + >> > > + memcpy(ctx->u.aes.key, key, key_len); >> > > + sg_init_one(&ctx->u.aes.key_sg, ctx->u.aes.key, key_len); >> > > + >> > > + return 0; >> > > +} >> > > + >> > > +static int ccp_aes_gcm_setauthsize(struct crypto_aead *tfm, >> > > + unsigned int authsize) >> > > +{ >> > > + return 0; >> > > +} >> > > + >> > > +static int ccp_aes_gcm_crypt(struct aead_request *req, bool encrypt) >> > > +{ >> > > + struct crypto_aead *tfm = crypto_aead_reqtfm(req); >> > > + struct ccp_ctx *ctx = crypto_aead_ctx(tfm); >> > > + struct ccp_aes_req_ctx *rctx = aead_request_ctx(req); >> > > + struct scatterlist *iv_sg = NULL; >> > > + unsigned int iv_len = 0; >> > > + int i; >> > > + int ret = 0; >> > > + >> > > + if (!ctx->u.aes.key_len) >> > > + return -EINVAL; >> > > + >> > > + if (ctx->u.aes.mode != CCP_AES_MODE_GCM) >> > > + return -EINVAL; >> > > + >> > > + if (!req->iv) >> > > + return -EINVAL; >> > > + >> > > + /* >> > > + * 5 parts: >> > > + * plaintext/ciphertext input >> > > + * AAD >> > > + * key >> > > + * IV >> > > + * Destination+tag buffer >> > > + */ >> > > + >> > > + /* According to the way AES GCM has been implemented here, >> > > + * per RFC 4106 it seems, the provided IV is fixed at 12 bytes, >> > >> > When you have that restriction, should the cipher be called >> > rfc4106(gcm(aes))? >> > >> > But then, the key is 4 bytes longer than a normal AES key as it contains >> > the leading 32 bits of the IV. >> >> I had my wires crossed due to an incomplete understanding of an AEAD cipher >> in general, and GCM in particular. I'm hopeful that someone can help me >> understand: >> >> For the AES GCM encryption tests in testmgr.h, where there is an IV, >> they're all >> 12 bytes in length. As I understand AES GCM the IV can be anywhere from >> 1 to 2^64 >> bits in length; the value of 96 makes for convenience and efficiency. >> But it's >> neither a requirement nor restriction. > > That is correct. For longer IVs, you would need to use Ghash to compress > it to > 96 bits. The remaining 32 bits to get to one AES block is the counter > that is > used for the CTR AES mode in GCM. Yes, understood. It's all falling into place now. What seems to be missing (to me) is a way for the transform to indicate that it allows all valid (GCM) IV lengths, as opposed to the (specified in the data structure) 12 bytes. I get the context of IPSec, but I would think AF_ALG allowing access to the transforms means that we can't rely upon a context. And there seems to be no way for an implementation to let a user know about any IV restrictions (or not). Do we just let the implementation return an error when it can't handle something? Or (highly possible) am I missing the obvious? >> There are no tests (in testmgr.h) that use an IV length other than 0 or 96. > > See aes_gcm_rfc4106_enc_tv_template for other types of IV. All 8 bytes, it seems, which makes sense for 4106. >> My comment about RFC4106 has to do with requiring an IV 0f 96 bits + a word >> that >> is incremented for each block (making every nonce unique, per the >> requirement). >> But let's ignore that, please. >> >> It looks as if: >> >> What seems to be missing is the ability to register a (GCM) transform >> that can >> handle an IV of arbitrary (allowable) length. I have to specify the >> length (ivsize) >> when I register an algorithm, and everything I see in the existing code >> appears >> to expect a GCM ivsize to be 96 bits, period (or zero). This is what I >> meant when >> I referenced RFC4106: I perceive restrictions not in my code, but n the >> way GCM seems >> to be supported in the crypto AEAD framework. A complete GCM >> implementation would not >> seem to have a restriction to a specific IV length (rather, a range of >> allowed >> values). > > 96 bits is the use case in IPSEC. As the kernel crypto API transforms > are used > for IPSEC. Nobody would prevent you from supporting other IV sizes. But > then > you would need to add a Ghash operation to compress it to the right > length. No > other GCM implementation has that and hence the limitation. Of course. That's the component that I'm missing, and I want to understand whether there's a compelling need. > But 96 bits is not the common case. See the 4106 implementations, you > see the > ivsize being 8. This is correct because setkey requires AES keysize + 4 > bytes > in length (see crypto_rfc4106_setkey for an example). The trailing 4 > bytes of > the key are the initial 4 bytes of the GCM IV. Yes. The RFC4106 document is pretty clear on the layout of the IV. > My comment was about your comment to refer to RFC4106. I just wanted to > understand your code and and make sense of your comments. :-) >> >> Is my reading of the GCM description in error? Do we need/want the ability >> to have a flexible IV length for GCM? What am I not understanding? > > In your case, just change the wording in the comment slightly and we are > all good. Will do. I appreciate the discussion! Very helpful. >> >> For reference, I'm working from the NIST doc: >> http://csrc.nist.gov/groups/ST/toolkit/BCM/documents/proposedmodes/gcm/gcm-s >> pec.pdf > >> >> > > + rctx->cmd.u.aes.key = &ctx->u.aes.key_sg; >> > > + rctx->cmd.u.aes.key_len = ctx->u.aes.key_len; >> > > + rctx->cmd.u.aes.iv = iv_sg; >> > > + rctx->cmd.u.aes.iv_len = iv_len; >> > > + rctx->cmd.u.aes.src = req->src; >> > > + rctx->cmd.u.aes.src_len = req->cryptlen; >> > > + rctx->cmd.u.aes.aad_len = req->assoclen; >> > >> > Just to be on the safe side: is the implementation good when cryptlen or >> > assoclen is 0? >> >> The engine has been designed to handle those two conditions. I've been >> watching the discussions around these issues. >> >> The first encryption test in testmgr.h has no input data nor IV. This >> implementation passes that test. >> >> The second encryption test in testmgr.h has input data but no IV, and this >> implementation passes. >> >> Is that an acceptable validation, or do we need more? > > I would recommend at least a private test with no input and no AAD (i.e. > authentication only). > > Maybe you can add a patch to testmgr for this case. An example is found at > [1]. Here, tag and exp is the expected result of the operation. > > [1] https://github.com/smuellerDD/libkcapi/blob/master/test/test.sh#L330 >> >> Thanks, >> Gary > > > > Ciao > Stephan -- This is my day job. Follow me at: IG/Twitter/Facebook: @grhookphoto IG/Twitter/Facebook: @grhphotographer