| Message ID | 20170628132710.97278-1-david@sigma-star.at (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
On 6/28/2017 4:27 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. > > This issue was revealed by the changes in the SW CTS mode in commit > 0605c41cc53ca ("crypto: cts - Convert to skcipher") > > Cc: <stable@vger.kernel.org> # 4.8+ > Signed-off-by: David Gstir <david@sigma-star.at> Reviewed-by: Horia Geantă <horia.geanta@nxp.com> Thanks, Horia
On 6/28/2017 4:42 PM, Horia Geantă wrote: > On 6/28/2017 4:27 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. >> >> This issue was revealed by the changes in the SW CTS mode in commit >> 0605c41cc53ca ("crypto: cts - Convert to skcipher") >> >> Cc: <stable@vger.kernel.org> # 4.8+ >> Signed-off-by: David Gstir <david@sigma-star.at> > Reviewed-by: Horia Geantă <horia.geanta@nxp.com> > Btw, instead of updating the IV in SW, CAAM engine could be programmed to do it - by saving the Context Register of the AES accelerator. Unfortunately this would require changes in quite a few places: shared descriptor, HW S/G generation logic, IV dma (un)mapping and maybe others. So it's better to have this fix now (which, considering size, is appropriate for -stable) and later, if needed, offload IV updating in HW. Regards, Horia
On Wed, Jun 28, 2017 at 03:27:10PM +0200, 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. > > This issue was revealed by the changes in the SW CTS mode in commit > 0605c41cc53ca ("crypto: cts - Convert to skcipher") > > Cc: <stable@vger.kernel.org> # 4.8+ > Signed-off-by: David Gstir <david@sigma-star.at> Patch applied. Thanks.
Hi, On Thu, Jun 29, 2017 at 1:19 PM, Horia Geantă <horia.geanta@nxp.com> wrote: > On 6/28/2017 4:42 PM, Horia Geantă wrote: >> On 6/28/2017 4:27 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. >>> >>> This issue was revealed by the changes in the SW CTS mode in commit >>> 0605c41cc53ca ("crypto: cts - Convert to skcipher") >>> >>> Cc: <stable@vger.kernel.org> # 4.8+ >>> Signed-off-by: David Gstir <david@sigma-star.at> >> Reviewed-by: Horia Geantă <horia.geanta@nxp.com> >> > Btw, instead of updating the IV in SW, CAAM engine could be programmed > to do it - by saving the Context Register of the AES accelerator. > > Unfortunately this would require changes in quite a few places: shared > descriptor, HW S/G generation logic, IV dma (un)mapping and maybe others. > > So it's better to have this fix now (which, considering size, is > appropriate for -stable) and later, if needed, offload IV updating in HW. > My apologies for reviving this thread from the dead, but doesn't the patch fail for in-place decryption since we are copying from req->dst after the operation is done, and therefore it no longer contains the ciphertext? I'm asking since I ran into a similar issue in the ccree driver and thought to deploy a similar fix but could not convince myself why this works. Thanks! Gilad
On 8/14/2017 10:59 AM, Gilad Ben-Yossef wrote: > Hi, > > On Thu, Jun 29, 2017 at 1:19 PM, Horia Geantă <horia.geanta@nxp.com> wrote: >> On 6/28/2017 4:42 PM, Horia Geantă wrote: >>> On 6/28/2017 4:27 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. >>>> >>>> This issue was revealed by the changes in the SW CTS mode in commit >>>> 0605c41cc53ca ("crypto: cts - Convert to skcipher") >>>> >>>> Cc: <stable@vger.kernel.org> # 4.8+ >>>> Signed-off-by: David Gstir <david@sigma-star.at> >>> Reviewed-by: Horia Geantă <horia.geanta@nxp.com> >>> >> Btw, instead of updating the IV in SW, CAAM engine could be programmed >> to do it - by saving the Context Register of the AES accelerator. >> >> Unfortunately this would require changes in quite a few places: shared >> descriptor, HW S/G generation logic, IV dma (un)mapping and maybe others. >> >> So it's better to have this fix now (which, considering size, is >> appropriate for -stable) and later, if needed, offload IV updating in HW. >> > > My apologies for reviving this thread from the dead, but doesn't the patch fail > for in-place decryption since we are copying from req->dst after > the operation is done, and therefore it no longer contains the ciphertext? > You are right, thanks! Will follow up with a fix. Though cts(cbc(aes)) in particular is working, see below. > I'm asking since I ran into a similar issue in the ccree driver and thought > to deploy a similar fix but could not convince myself why this works. > IIUC cts(cbc(aes)) in-place decryption (with cbc(aes) offloaded to CAAM engine) works since SW implementation of cts, when performing the ciphertext stealing phase in cts_cbc_decrypt() does not use req->iv, but a previously value, saved before staring decryption in crypto_cts_decrypt(): if (cbc_blocks <= 1) memcpy(space, req->iv, bsize); else scatterwalk_map_and_copy(space, req->src, offset - 2 * bsize, bsize, 0); Horia
On Tue, Sep 5, 2017 at 6:33 PM, Horia Geantă <horia.geanta@nxp.com> wrote: > On 8/14/2017 10:59 AM, Gilad Ben-Yossef wrote: >> Hi, >> >> On Thu, Jun 29, 2017 at 1:19 PM, Horia Geantă <horia.geanta@nxp.com> wrote: >>> On 6/28/2017 4:42 PM, Horia Geantă wrote: >>>> On 6/28/2017 4:27 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. >>>>> >>>>> This issue was revealed by the changes in the SW CTS mode in commit >>>>> 0605c41cc53ca ("crypto: cts - Convert to skcipher") >>>>> >>>>> Cc: <stable@vger.kernel.org> # 4.8+ >>>>> Signed-off-by: David Gstir <david@sigma-star.at> >>>> Reviewed-by: Horia Geantă <horia.geanta@nxp.com> >>>> >>> Btw, instead of updating the IV in SW, CAAM engine could be programmed >>> to do it - by saving the Context Register of the AES accelerator. >>> >>> Unfortunately this would require changes in quite a few places: shared >>> descriptor, HW S/G generation logic, IV dma (un)mapping and maybe others. >>> >>> So it's better to have this fix now (which, considering size, is >>> appropriate for -stable) and later, if needed, offload IV updating in HW. >>> >> >> My apologies for reviving this thread from the dead, but doesn't the patch fail >> for in-place decryption since we are copying from req->dst after >> the operation is done, and therefore it no longer contains the ciphertext? >> > You are right, thanks! Will follow up with a fix. > Though cts(cbc(aes)) in particular is working, see below. > >> I'm asking since I ran into a similar issue in the ccree driver and thought >> to deploy a similar fix but could not convince myself why this works. >> > IIUC cts(cbc(aes)) in-place decryption (with cbc(aes) offloaded to CAAM > engine) works since SW implementation of cts, when performing the > ciphertext stealing phase in cts_cbc_decrypt() does not use req->iv, but > a previously value, saved before staring decryption in crypto_cts_decrypt(): > > if (cbc_blocks <= 1) > memcpy(space, req->iv, bsize); > else > scatterwalk_map_and_copy(space, req->src, offset - 2 * bsize, > bsize, 0); > Is that not a performance bug in software CTS than? I mean all those transformation drivers doing that extra copy and possibly malloc and free to save the data for the info and than have the CTS implementation ignore that and do its own memory copy? Gilad
On 9/6/2017 1:14 PM, Gilad Ben-Yossef wrote: > On Tue, Sep 5, 2017 at 6:33 PM, Horia Geantă <horia.geanta@nxp.com> wrote: >> On 8/14/2017 10:59 AM, Gilad Ben-Yossef wrote: >>> Hi, >>> >>> On Thu, Jun 29, 2017 at 1:19 PM, Horia Geantă <horia.geanta@nxp.com> wrote: >>>> On 6/28/2017 4:42 PM, Horia Geantă wrote: >>>>> On 6/28/2017 4:27 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. >>>>>> >>>>>> This issue was revealed by the changes in the SW CTS mode in commit >>>>>> 0605c41cc53ca ("crypto: cts - Convert to skcipher") >>>>>> >>>>>> Cc: <stable@vger.kernel.org> # 4.8+ >>>>>> Signed-off-by: David Gstir <david@sigma-star.at> >>>>> Reviewed-by: Horia Geantă <horia.geanta@nxp.com> >>>>> >>>> Btw, instead of updating the IV in SW, CAAM engine could be programmed >>>> to do it - by saving the Context Register of the AES accelerator. >>>> >>>> Unfortunately this would require changes in quite a few places: shared >>>> descriptor, HW S/G generation logic, IV dma (un)mapping and maybe others. >>>> >>>> So it's better to have this fix now (which, considering size, is >>>> appropriate for -stable) and later, if needed, offload IV updating in HW. >>>> >>> >>> My apologies for reviving this thread from the dead, but doesn't the patch fail >>> for in-place decryption since we are copying from req->dst after >>> the operation is done, and therefore it no longer contains the ciphertext? >>> >> You are right, thanks! Will follow up with a fix. >> Though cts(cbc(aes)) in particular is working, see below. >> >>> I'm asking since I ran into a similar issue in the ccree driver and thought >>> to deploy a similar fix but could not convince myself why this works. >>> >> IIUC cts(cbc(aes)) in-place decryption (with cbc(aes) offloaded to CAAM >> engine) works since SW implementation of cts, when performing the >> ciphertext stealing phase in cts_cbc_decrypt() does not use req->iv, but >> a previously value, saved before staring decryption in crypto_cts_decrypt(): >> >> if (cbc_blocks <= 1) >> memcpy(space, req->iv, bsize); >> else >> scatterwalk_map_and_copy(space, req->src, offset - 2 * bsize, >> bsize, 0); >> > > Is that not a performance bug in software CTS than? I mean all those > transformation > drivers doing that extra copy and possibly malloc and free to save the > data for the info > and than have the CTS implementation ignore that and do its own memory copy? > AFAICT, in case cbc_blocks > 1 cts saves the second from last full block, while underlying cbc subrequest saves the last full block. Horia
diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c index 398807d1b77e..c45b5bf65254 100644 --- a/drivers/crypto/caam/caamalg.c +++ b/drivers/crypto/caam/caamalg.c @@ -882,10 +882,10 @@ 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); +#ifdef DEBUG dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err); #endif @@ -904,6 +904,14 @@ static void ablkcipher_encrypt_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. This is used e.g. by the CTS mode. + */ + scatterwalk_map_and_copy(req->info, req->dst, req->nbytes - ivsize, + ivsize, 0); + kfree(edesc); ablkcipher_request_complete(req, err); @@ -914,10 +922,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 +943,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. + */ + scatterwalk_map_and_copy(req->info, req->src, req->nbytes - ivsize, + ivsize, 0); + 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. This issue was revealed by the changes in the SW CTS mode in commit 0605c41cc53ca ("crypto: cts - Convert to skcipher") Cc: <stable@vger.kernel.org> # 4.8+ Signed-off-by: David Gstir <david@sigma-star.at> --- drivers/crypto/caam/caamalg.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)