linux-crypto.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] crypto: generic/cts - fix regression in iv handling
@ 2017-01-16  9:16 Ard Biesheuvel
  2017-01-17  9:11 ` Herbert Xu
  0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2017-01-16  9:16 UTC (permalink / raw)
  To: linux-crypto, herbert; +Cc: Ard Biesheuvel

Since the skcipher conversion in commit 0605c41cc53c ("crypto:
cts - Convert to skcipher"), the cts code tacitly assumes that
the underlying CBC encryption transform performed on the first
part of the plaintext returns an IV in req->iv that is suitable
for encrypting the final bit.

While this is usually the case, it is not mandated by the API, and
given that the CTS code already accesses the ciphertext scatterlist
to retrieve those bytes, we can simply copy them into req->iv before
proceeding.

Fixes: 0605c41cc53c ("crypto: cts - Convert to skcipher")
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 crypto/cts.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/crypto/cts.c b/crypto/cts.c
index a1335d6c35fb..3270ce8f278d 100644
--- a/crypto/cts.c
+++ b/crypto/cts.c
@@ -114,6 +114,7 @@ static int cts_cbc_encrypt(struct skcipher_request *req)
 
 	sg = scatterwalk_ffwd(rctx->sg, req->dst, offset - bsize);
 	scatterwalk_map_and_copy(d + bsize, sg, 0, bsize, 0);
+	memcpy(req->iv, d + bsize, bsize);
 
 	memset(d, 0, bsize);
 	scatterwalk_map_and_copy(d, req->src, offset, lastn, 0);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] crypto: generic/cts - fix regression in iv handling
  2017-01-16  9:16 [PATCH] crypto: generic/cts - fix regression in iv handling Ard Biesheuvel
@ 2017-01-17  9:11 ` Herbert Xu
  2017-01-17  9:20   ` Ard Biesheuvel
  0 siblings, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2017-01-17  9:11 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: linux-crypto

On Mon, Jan 16, 2017 at 09:16:35AM +0000, Ard Biesheuvel wrote:
> Since the skcipher conversion in commit 0605c41cc53c ("crypto:
> cts - Convert to skcipher"), the cts code tacitly assumes that
> the underlying CBC encryption transform performed on the first
> part of the plaintext returns an IV in req->iv that is suitable
> for encrypting the final bit.
> 
> While this is usually the case, it is not mandated by the API, and
> given that the CTS code already accesses the ciphertext scatterlist
> to retrieve those bytes, we can simply copy them into req->iv before
> proceeding.

Ugh while there are some legacy drivers that break this is certainly
part of the API.

Which underlying CBC implementation is breaking this?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] crypto: generic/cts - fix regression in iv handling
  2017-01-17  9:11 ` Herbert Xu
@ 2017-01-17  9:20   ` Ard Biesheuvel
  2017-01-17  9:25     ` Herbert Xu
  0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2017-01-17  9:20 UTC (permalink / raw)
  To: Herbert Xu; +Cc: linux-crypto

On 17 January 2017 at 09:11, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Mon, Jan 16, 2017 at 09:16:35AM +0000, Ard Biesheuvel wrote:
>> Since the skcipher conversion in commit 0605c41cc53c ("crypto:
>> cts - Convert to skcipher"), the cts code tacitly assumes that
>> the underlying CBC encryption transform performed on the first
>> part of the plaintext returns an IV in req->iv that is suitable
>> for encrypting the final bit.
>>
>> While this is usually the case, it is not mandated by the API, and
>> given that the CTS code already accesses the ciphertext scatterlist
>> to retrieve those bytes, we can simply copy them into req->iv before
>> proceeding.
>
> Ugh while there are some legacy drivers that break this is certainly
> part of the API.
>
> Which underlying CBC implementation is breaking this?
>

arch/arm64/crypto/aes-modes.S does not return the IV back to the
caller, so cts(cbc-aes-ce) is currently broken.

So to be clear, it is part of the API that after calling
crypto_skcipher_encrypt(req), and completing the request, req->iv
should contain a value that could potentially be used to encrypt
additional data? That sounds highly specific to CBC (e.g., this could
never work with XTS, since the tweak generation is only performed
once), so it does not make sense for skciphers in general. For
instance, drivers for h/w peripherals that never need to map the data
to begin with (since they only pass the physical addresses to the
hardware) will need to explicitly map the destination buffer to
retrieve those bytes, on the off chance that the transform may be
wrapped by CTS.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] crypto: generic/cts - fix regression in iv handling
  2017-01-17  9:20   ` Ard Biesheuvel
@ 2017-01-17  9:25     ` Herbert Xu
  2017-01-17  9:30       ` Ard Biesheuvel
  0 siblings, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2017-01-17  9:25 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: linux-crypto

On Tue, Jan 17, 2017 at 09:20:11AM +0000, Ard Biesheuvel wrote:
> 
> So to be clear, it is part of the API that after calling
> crypto_skcipher_encrypt(req), and completing the request, req->iv
> should contain a value that could potentially be used to encrypt
> additional data? That sounds highly specific to CBC (e.g., this could
> never work with XTS, since the tweak generation is only performed
> once), so it does not make sense for skciphers in general. For
> instance, drivers for h/w peripherals that never need to map the data
> to begin with (since they only pass the physical addresses to the
> hardware) will need to explicitly map the destination buffer to
> retrieve those bytes, on the off chance that the transform may be
> wrapped by CTS.

Yes this is part of the API.  There was a patch to test this in
testmgr but I wanted to give the drivers some more time before
adding it.

It isn't just CBC that uses chaining.  Other modes such as CTR
use it too.  Disk encryption in general don't chaining but that's
because they are sector-oriented.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] crypto: generic/cts - fix regression in iv handling
  2017-01-17  9:25     ` Herbert Xu
@ 2017-01-17  9:30       ` Ard Biesheuvel
  2017-01-17  9:37         ` Herbert Xu
  0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2017-01-17  9:30 UTC (permalink / raw)
  To: Herbert Xu; +Cc: linux-crypto

On 17 January 2017 at 09:25, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Tue, Jan 17, 2017 at 09:20:11AM +0000, Ard Biesheuvel wrote:
>>
>> So to be clear, it is part of the API that after calling
>> crypto_skcipher_encrypt(req), and completing the request, req->iv
>> should contain a value that could potentially be used to encrypt
>> additional data? That sounds highly specific to CBC (e.g., this could
>> never work with XTS, since the tweak generation is only performed
>> once), so it does not make sense for skciphers in general. For
>> instance, drivers for h/w peripherals that never need to map the data
>> to begin with (since they only pass the physical addresses to the
>> hardware) will need to explicitly map the destination buffer to
>> retrieve those bytes, on the off chance that the transform may be
>> wrapped by CTS.
>
> Yes this is part of the API.  There was a patch to test this in
> testmgr but I wanted to give the drivers some more time before
> adding it.
>

Got a link?

> It isn't just CBC that uses chaining.  Other modes such as CTR
> use it too.  Disk encryption in general don't chaining but that's
> because they are sector-oriented.
>

OK, so that means chaining skcipher_set_crypt() calls, where req->iv
is passed on between requests? Are there chaining modes beyond
cts(cbc) encryption that rely on this?

It is easily fixed in the chaining mode code, so I'm perfectly happy
to fix it there instead, but I'd like to understand the requirements
exactly before doing so

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] crypto: generic/cts - fix regression in iv handling
  2017-01-17  9:30       ` Ard Biesheuvel
@ 2017-01-17  9:37         ` Herbert Xu
  0 siblings, 0 replies; 6+ messages in thread
From: Herbert Xu @ 2017-01-17  9:37 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: linux-crypto

On Tue, Jan 17, 2017 at 09:30:30AM +0000, Ard Biesheuvel wrote:
>
> Got a link?

http://lkml.iu.edu/hypermail/linux/kernel/1506.2/00346.html

> OK, so that means chaining skcipher_set_crypt() calls, where req->iv
> is passed on between requests? Are there chaining modes beyond
> cts(cbc) encryption that rely on this?

I think algif_skcipher relies on this too.

> It is easily fixed in the chaining mode code, so I'm perfectly happy
> to fix it there instead, but I'd like to understand the requirements
> exactly before doing so

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-01-17 10:00 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-16  9:16 [PATCH] crypto: generic/cts - fix regression in iv handling Ard Biesheuvel
2017-01-17  9:11 ` Herbert Xu
2017-01-17  9:20   ` Ard Biesheuvel
2017-01-17  9:25     ` Herbert Xu
2017-01-17  9:30       ` Ard Biesheuvel
2017-01-17  9:37         ` Herbert Xu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).