| Message ID | 20170602122446.2427-2-david@sigma-star.at (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
On 6/2/2017 3:25 PM, David Gstir wrote: > Certain cipher modes like CTS expect the IV (req->info) of > ablkcipher_request (or equivalently req->iv of skcipher_request) to > contain the last ciphertext block when the {en,de}crypt operation is done. > This is currently not the case for the CAAM driver which in turn breaks > e.g. cts(cbc(aes)) when the CAAM driver is enabled. > > This patch fixes the CAAM driver to properly set the IV after the > {en,de}crypt operation of ablkcipher finishes. > > Signed-off-by: David Gstir <david@sigma-star.at> > --- > drivers/crypto/caam/caamalg.c | 26 ++++++++++++++++++++++++-- > 1 file changed, 24 insertions(+), 2 deletions(-) > > diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c > index 398807d1b77e..d13c1aee4427 100644 > --- a/drivers/crypto/caam/caamalg.c > +++ b/drivers/crypto/caam/caamalg.c > @@ -882,10 +882,11 @@ static void ablkcipher_encrypt_done(struct device *jrdev, u32 *desc, u32 err, > { > struct ablkcipher_request *req = context; > struct ablkcipher_edesc *edesc; > -#ifdef DEBUG > struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req); > int ivsize = crypto_ablkcipher_ivsize(ablkcipher); > + int nents; > > +#ifdef DEBUG > dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err); > #endif > > @@ -904,6 +905,19 @@ static void ablkcipher_encrypt_done(struct device *jrdev, u32 *desc, u32 err, > #endif > > ablkcipher_unmap(jrdev, edesc, req); > + > + if (req->src == req->dst) > + nents = edesc->src_nents; > + else > + nents = edesc->dst_nents; > + > + /* > + * The crypto API expects us to set the IV (req->info) to the last > + * ciphertext block. This is used e.g. by the CTS mode. > + */ IIUC, IV update is required only in case of CBC. Since this callback is used also for CTR, we should avoid the copy: if ((ctx->cdata.algtype & OP_ALG_AAI_MASK) == OP_ALG_AAI_CBC) ... > + sg_pcopy_to_buffer(req->dst, nents, req->info, ivsize, > + req->nbytes - ivsize); scatterwalk_map_and_copy() should be used instead. > + > kfree(edesc); > > ablkcipher_request_complete(req, err); > @@ -914,10 +928,10 @@ static void ablkcipher_decrypt_done(struct device *jrdev, u32 *desc, u32 err, > { > struct ablkcipher_request *req = context; > struct ablkcipher_edesc *edesc; > -#ifdef DEBUG > struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req); > int ivsize = crypto_ablkcipher_ivsize(ablkcipher); > > +#ifdef DEBUG > dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err); > #endif > > @@ -935,6 +949,14 @@ static void ablkcipher_decrypt_done(struct device *jrdev, u32 *desc, u32 err, > #endif > > ablkcipher_unmap(jrdev, edesc, req); > + > + /* > + * The crypto API expects us to set the IV (req->info) to the last > + * ciphertext block. > + */ > + sg_pcopy_to_buffer(req->src, edesc->src_nents, req->info, ivsize, > + req->nbytes - ivsize); > + > kfree(edesc); > > ablkcipher_request_complete(req, err); >
On Mon, Jun 19, 2017 at 10:31:27AM +0000, Horia Geantă wrote: > > IIUC, IV update is required only in case of CBC. > Since this callback is used also for CTR, we should avoid the copy: > if ((ctx->cdata.algtype & OP_ALG_AAI_MASK) == OP_ALG_AAI_CBC) ... No it is needed for CTR too. Cheers,
Herbert, > On 20 Jun 2017, at 03:28, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Mon, Jun 19, 2017 at 10:31:27AM +0000, Horia Geantă wrote: >> >> IIUC, IV update is required only in case of CBC. >> Since this callback is used also for CTR, we should avoid the copy: >> if ((ctx->cdata.algtype & OP_ALG_AAI_MASK) == OP_ALG_AAI_CBC) ... > > No it is needed for CTR too. So, am I correct in assuming that it is required for all modes including AEAD modes like GCM? In that case I'll include a fix for the CAAM GCM mode too. Thanks, David
On Mon, Jun 26, 2017 at 07:40:58AM +0200, David Gstir wrote: > > So, am I correct in assuming that it is required for all modes including AEAD modes like GCM? > In that case I'll include a fix for the CAAM GCM mode too. It's only required for skcihper. As we do not do chunking/streaming with our AEAD interface it is not required for GCM. Cheers,
On 6/19/2017 1:31 PM, Horia Geantă wrote: > On 6/2/2017 3:25 PM, David Gstir wrote: >> Certain cipher modes like CTS expect the IV (req->info) of >> ablkcipher_request (or equivalently req->iv of skcipher_request) to >> contain the last ciphertext block when the {en,de}crypt operation is done. >> This is currently not the case for the CAAM driver which in turn breaks >> e.g. cts(cbc(aes)) when the CAAM driver is enabled. >> >> This patch fixes the CAAM driver to properly set the IV after the >> {en,de}crypt operation of ablkcipher finishes. >> >> Signed-off-by: David Gstir <david@sigma-star.at> >> --- >> drivers/crypto/caam/caamalg.c | 26 ++++++++++++++++++++++++-- >> 1 file changed, 24 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c >> index 398807d1b77e..d13c1aee4427 100644 >> --- a/drivers/crypto/caam/caamalg.c >> +++ b/drivers/crypto/caam/caamalg.c >> @@ -882,10 +882,11 @@ static void ablkcipher_encrypt_done(struct device *jrdev, u32 *desc, u32 err, >> { >> struct ablkcipher_request *req = context; >> struct ablkcipher_edesc *edesc; >> -#ifdef DEBUG >> struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req); >> int ivsize = crypto_ablkcipher_ivsize(ablkcipher); >> + int nents; >> >> +#ifdef DEBUG >> dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err); >> #endif >> >> @@ -904,6 +905,19 @@ static void ablkcipher_encrypt_done(struct device *jrdev, u32 *desc, u32 err, >> #endif [snip] >> + sg_pcopy_to_buffer(req->dst, nents, req->info, ivsize, >> + req->nbytes - ivsize); > > scatterwalk_map_and_copy() should be used instead. > David, IIUC this is the only change needed in this patch (applies both for encryption and decryption, of course). Will you formally resubmit? Thanks, Horia
Horia, > On 28 Jun 2017, at 10:32, Horia Geantă <horia.geanta@nxp.com> wrote: > >>> + sg_pcopy_to_buffer(req->dst, nents, req->info, ivsize, >>> + req->nbytes - ivsize); >> >> scatterwalk_map_and_copy() should be used instead. >> > David, IIUC this is the only change needed in this patch (applies both > for encryption and decryption, of course). > Will you formally resubmit? Thanks for the reminder. I did not get arround it yet. Will send the new patch within the next few hours. Thanks, David
diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c index 398807d1b77e..d13c1aee4427 100644 --- a/drivers/crypto/caam/caamalg.c +++ b/drivers/crypto/caam/caamalg.c @@ -882,10 +882,11 @@ static void ablkcipher_encrypt_done(struct device *jrdev, u32 *desc, u32 err, { struct ablkcipher_request *req = context; struct ablkcipher_edesc *edesc; -#ifdef DEBUG struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req); int ivsize = crypto_ablkcipher_ivsize(ablkcipher); + int nents; +#ifdef DEBUG dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err); #endif @@ -904,6 +905,19 @@ static void ablkcipher_encrypt_done(struct device *jrdev, u32 *desc, u32 err, #endif ablkcipher_unmap(jrdev, edesc, req); + + if (req->src == req->dst) + nents = edesc->src_nents; + else + nents = edesc->dst_nents; + + /* + * The crypto API expects us to set the IV (req->info) to the last + * ciphertext block. This is used e.g. by the CTS mode. + */ + sg_pcopy_to_buffer(req->dst, nents, req->info, ivsize, + req->nbytes - ivsize); + kfree(edesc); ablkcipher_request_complete(req, err); @@ -914,10 +928,10 @@ static void ablkcipher_decrypt_done(struct device *jrdev, u32 *desc, u32 err, { struct ablkcipher_request *req = context; struct ablkcipher_edesc *edesc; -#ifdef DEBUG struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req); int ivsize = crypto_ablkcipher_ivsize(ablkcipher); +#ifdef DEBUG dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err); #endif @@ -935,6 +949,14 @@ static void ablkcipher_decrypt_done(struct device *jrdev, u32 *desc, u32 err, #endif ablkcipher_unmap(jrdev, edesc, req); + + /* + * The crypto API expects us to set the IV (req->info) to the last + * ciphertext block. + */ + sg_pcopy_to_buffer(req->src, edesc->src_nents, req->info, ivsize, + req->nbytes - ivsize); + kfree(edesc); ablkcipher_request_complete(req, err);
Certain cipher modes like CTS expect the IV (req->info) of ablkcipher_request (or equivalently req->iv of skcipher_request) to contain the last ciphertext block when the {en,de}crypt operation is done. This is currently not the case for the CAAM driver which in turn breaks e.g. cts(cbc(aes)) when the CAAM driver is enabled. This patch fixes the CAAM driver to properly set the IV after the {en,de}crypt operation of ablkcipher finishes. Signed-off-by: David Gstir <david@sigma-star.at> --- drivers/crypto/caam/caamalg.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-)