From: Gary R Hook <[email protected]>
The CCP driver generally uses a round-robin approach when
assigning operations to available CCPs. For the DMA engine,
however, the DMA mappings of the SGs are associated with a
specific CCP. When an IOMMU is enabled, the IOMMU is
programmed based on this specific device.
If the DMA operations are not performed by that specific
CCP then addressing errors and I/O page faults will occur.
Update the CCP driver to allow a specific CCP device to be
requested for an operation and use this in the DMA engine
support.
Cc: <[email protected]> # 4.9.x-
Signed-off-by: Gary R Hook <[email protected]>
---
drivers/crypto/ccp/ccp-dev.c | 5 ++++-
drivers/crypto/ccp/ccp-dmaengine.c | 1 +
include/linux/ccp.h | 2 +-
3 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/crypto/ccp/ccp-dev.c b/drivers/crypto/ccp/ccp-dev.c
index 511ab04..92d1c69 100644
--- a/drivers/crypto/ccp/ccp-dev.c
+++ b/drivers/crypto/ccp/ccp-dev.c
@@ -283,11 +283,14 @@ unsigned int ccp_version(void)
*/
int ccp_enqueue_cmd(struct ccp_cmd *cmd)
{
- struct ccp_device *ccp = ccp_get_device();
+ struct ccp_device *ccp;
unsigned long flags;
unsigned int i;
int ret;
+ /* Some commands might need to be sent to a specific device */
+ ccp = cmd->ccp ? cmd->ccp : ccp_get_device();
+
if (!ccp)
return -ENODEV;
diff --git a/drivers/crypto/ccp/ccp-dmaengine.c b/drivers/crypto/ccp/ccp-dmaengine.c
index e5d9278..8d0eeb4 100644
--- a/drivers/crypto/ccp/ccp-dmaengine.c
+++ b/drivers/crypto/ccp/ccp-dmaengine.c
@@ -390,6 +390,7 @@ static struct ccp_dma_desc *ccp_create_desc(struct dma_chan *dma_chan,
goto err;
ccp_cmd = &cmd->ccp_cmd;
+ ccp_cmd->ccp = chan->ccp;
ccp_pt = &ccp_cmd->u.passthru_nomap;
ccp_cmd->flags = CCP_CMD_MAY_BACKLOG;
ccp_cmd->flags |= CCP_CMD_PASSTHRU_NO_DMA_MAP;
diff --git a/include/linux/ccp.h b/include/linux/ccp.h
index c71dd8f..c41b8d99 100644
--- a/include/linux/ccp.h
+++ b/include/linux/ccp.h
@@ -556,7 +556,7 @@ enum ccp_engine {
* struct ccp_cmd - CCP operation request
* @entry: list element (ccp driver use only)
* @work: work element used for callbacks (ccp driver use only)
- * @ccp: CCP device to be run on (ccp driver use only)
+ * @ccp: CCP device to be run on
* @ret: operation return code (ccp driver use only)
* @flags: cmd processing flags
* @engine: CCP operation to perform
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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > + *
> > + * 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 <linux/module.h>
> > +#include <linux/sched.h>
> > +#include <linux/delay.h>
> > +#include <linux/scatterlist.h>
> > +#include <linux/crypto.h>
> > +#include <crypto/internal/aead.h>
> > +#include <crypto/algapi.h>
> > +#include <crypto/aes.h>
> > +#include <crypto/ctr.h>
> > +#include <crypto/scatterwalk.h>
> > +#include <linux/delay.h>
> > +
> > +#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.
There are no tests (in testmgr.h) that use an IV length other than 0 or 96.
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).
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?
For reference, I'm working from the NIST doc:
http://csrc.nist.gov/groups/ST/toolkit/BCM/documents/proposedmodes/gcm/gcm-spec.pdf
>
> > + * occupies the beginning of the IV array. Write a 32-bit
> > + * integer after that (bytes 13-16) with a value of "1".
> > + */
> > + memcpy(rctx->iv, req->iv, AES_GCM_IVSIZE);
> > + for (i = 0; i < 3; i++)
> > + rctx->iv[i + AES_GCM_IVSIZE] = 0;
> > + rctx->iv[AES_BLOCK_SIZE - 1] = 1;
> > +
> > + /* Set up a scatterlist for the IV */
> > + iv_sg = &rctx->iv_sg;
> > + iv_len = AES_BLOCK_SIZE;
> > + sg_init_one(iv_sg, rctx->iv, iv_len);
> > +
> > + /* The AAD + plaintext are concatenated in the src buffer */
> > + memset(&rctx->cmd, 0, sizeof(rctx->cmd));
> > + INIT_LIST_HEAD(&rctx->cmd.entry);
> > + rctx->cmd.engine = CCP_ENGINE_AES;
> > + rctx->cmd.u.aes.type = ctx->u.aes.type;
> > + rctx->cmd.u.aes.mode = ctx->u.aes.mode;
> > + rctx->cmd.u.aes.action =
> > + (encrypt) ? CCP_AES_ACTION_ENCRYPT : CCP_AES_ACTION_DECRYPT;
>
> Instead of this condition, why not changing the encrypt/decrypt function to
> directly provide the enc/dec variables?
Our existing code that uses this construct doesn't do that, but I
have no problem with the idea. Done.
> > + 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?
Thanks,
Gary
--
This is my day job. Follow me at:
IG/Twitter/Facebook: @grhookphoto
IG/Twitter/Facebook: @grhphotographer
Am Montag, 13. M?rz 2017, 20:35:07 CET schrieb Gary R Hook:
Hi Gary,
> 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 <[email protected]>
> > > ---
> > >
> > > 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 <[email protected]>
> > > + *
> > > + * 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 <linux/module.h>
> > > +#include <linux/sched.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/scatterlist.h>
> > > +#include <linux/crypto.h>
> > > +#include <crypto/internal/aead.h>
> > > +#include <crypto/algapi.h>
> > > +#include <crypto/aes.h>
> > > +#include <crypto/ctr.h>
> > > +#include <crypto/scatterwalk.h>
> > > +#include <linux/delay.h>
> > > +
> > > +#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.
>
> 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.
> 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.
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.
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.
>
> 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
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 <[email protected]>
>> > > ---
>> > >
>> > > 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 <[email protected]>
>> > > + *
>> > > + * 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 <linux/module.h>
>> > > +#include <linux/sched.h>
>> > > +#include <linux/delay.h>
>> > > +#include <linux/scatterlist.h>
>> > > +#include <linux/crypto.h>
>> > > +#include <crypto/internal/aead.h>
>> > > +#include <crypto/algapi.h>
>> > > +#include <crypto/aes.h>
>> > > +#include <crypto/ctr.h>
>> > > +#include <crypto/scatterwalk.h>
>> > > +#include <linux/delay.h>
>> > > +
>> > > +#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
Am Dienstag, 14. M?rz 2017, 15:34:00 CET schrieb Gary R Hook:
Hi Gary,
> 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....
Let us snip it :-)
> >> >
> >> > 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.
The kernel crypto API does not support varying IV sizes. So, simply stay with
12 / 8 bytes as explained should suffice.
> 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).
In algif_aead, we simply have:
unsigned ivsize =
crypto_aead_ivsize(crypto_aead_reqtfm(&ctx->aead_req));
...
if (con.iv && con.iv->ivlen != ivsize)
return -EINVAL;
Thus, if the user space caller does not provide exactly ivsize bytes of IV, he
gets an error.
>
> 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.
I would not think that there is any need for it. If there would be, a generic
helper for the IV compression should be added instead of having the algos
implementing it itself over and over again.
>
> > 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.
>
Ciao
Stephan
On Fri, Mar 10, 2017 at 12:28:18PM -0600, Gary R Hook wrote:
> From: Gary R Hook <[email protected]>
>
> The CCP driver generally uses a round-robin approach when
> assigning operations to available CCPs. For the DMA engine,
> however, the DMA mappings of the SGs are associated with a
> specific CCP. When an IOMMU is enabled, the IOMMU is
> programmed based on this specific device.
>
> If the DMA operations are not performed by that specific
> CCP then addressing errors and I/O page faults will occur.
>
> Update the CCP driver to allow a specific CCP device to be
> requested for an operation and use this in the DMA engine
> support.
>
> Cc: <[email protected]> # 4.9.x-
> Signed-off-by: Gary R Hook <[email protected]>
Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt