[RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Ard Biesheuvel]

Stephan reports that the arm64 implementation of cts(cbc(aes)) deviates
from the generic implementation in what it returns as the output IV. So
fix this, and add some test vectors to catch other non-compliant
implementations.

Stephan, could you provide a reference for the NIST validation tool and
how it flags this behaviour as non-compliant? Thanks.

Cc: Stephan Mueller <smueller@chronox.de>

Ard Biesheuvel (2):
  crypto: arm64/aes - align output IV with generic CBC-CTS driver
  crypto: testmgr - add output IVs for AES-CBC with ciphertext stealing

 arch/arm64/crypto/aes-modes.S |  2 ++
 crypto/testmgr.h              | 12 ++++++++++++
 2 files changed, 14 insertions(+)

-- 
2.20.1

[RFC/RFT PATCH 1/2] crypto: arm64/aes - align output IV with generic CBC-CTS driver

[Ard Biesheuvel]

The generic CTS chaining mode wraps the CBC mode driver in a way that
results in the IV buffer referenced by the skcipher request to be
updated with the last block of ciphertext. The arm64 implementation
deviates from this, given that CTS itself does not specify the concept
of an output IV, or how it should be generated, and so it was assumed
that the output IV does not matter.

However, Stephan reports that code exists that relies on this behavior,
and that there is even a NIST validation tool that flags it as
non-compliant [citation needed. Stephan?]

So let's align with the generic implementation here, and return the
penultimate block of ciphertext as the output IV.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/crypto/aes-modes.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/crypto/aes-modes.S b/arch/arm64/crypto/aes-modes.S
index cf618d8f6cec..80832464df50 100644
--- a/arch/arm64/crypto/aes-modes.S
+++ b/arch/arm64/crypto/aes-modes.S
@@ -275,6 +275,7 @@ AES_FUNC_START(aes_cbc_cts_encrypt)
 	add		x4, x0, x4
 	st1		{v0.16b}, [x4]			/* overlapping stores */
 	st1		{v1.16b}, [x0]
+	st1		{v1.16b}, [x5]
 	ret
 AES_FUNC_END(aes_cbc_cts_encrypt)
 
@@ -291,6 +292,7 @@ AES_FUNC_START(aes_cbc_cts_decrypt)
 	ld1		{v1.16b}, [x1]
 
 	ld1		{v5.16b}, [x5]			/* get iv */
+	st1		{v0.16b}, [x5]
 	dec_prepare	w3, x2, x6
 
 	decrypt_block	v0, w3, x2, x6, w7
-- 
2.20.1

[RFC/RFT PATCH 2/2] crypto: testmgr - add output IVs for AES-CBC with ciphertext stealing

[Ard Biesheuvel]

Add some test vectors to get coverage for the IV that is output by CTS
implementations.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 crypto/testmgr.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/crypto/testmgr.h b/crypto/testmgr.h
index d29983908c38..d45fa1ad91ee 100644
--- a/crypto/testmgr.h
+++ b/crypto/testmgr.h
@@ -31041,6 +31041,8 @@ static const struct cipher_testvec cts_mode_tv_template[] = {
 		.ctext	= "\xc6\x35\x35\x68\xf2\xbf\x8c\xb4"
 			  "\xd8\xa5\x80\x36\x2d\xa7\xff\x7f"
 			  "\x97",
+		.iv_out	= "\xc6\x35\x35\x68\xf2\xbf\x8c\xb4"
+			  "\xd8\xa5\x80\x36\x2d\xa7\xff\x7f",
 	}, {
 		.klen	= 16,
 		.key    = "\x63\x68\x69\x63\x6b\x65\x6e\x20"
@@ -31054,6 +31056,8 @@ static const struct cipher_testvec cts_mode_tv_template[] = {
 			  "\xd4\x45\xd4\xc8\xef\xf7\xed\x22"
 			  "\x97\x68\x72\x68\xd6\xec\xcc\xc0"
 			  "\xc0\x7b\x25\xe2\x5e\xcf\xe5",
+		.iv_out	= "\xfc\x00\x78\x3e\x0e\xfd\xb2\xc1"
+			  "\xd4\x45\xd4\xc8\xef\xf7\xed\x22",
 	}, {
 		.klen	= 16,
 		.key    = "\x63\x68\x69\x63\x6b\x65\x6e\x20"
@@ -31067,6 +31071,8 @@ static const struct cipher_testvec cts_mode_tv_template[] = {
 			  "\xbe\x7f\xcb\xcc\x98\xeb\xf5\xa8"
 			  "\x97\x68\x72\x68\xd6\xec\xcc\xc0"
 			  "\xc0\x7b\x25\xe2\x5e\xcf\xe5\x84",
+		.iv_out	= "\x39\x31\x25\x23\xa7\x86\x62\xd5"
+			  "\xbe\x7f\xcb\xcc\x98\xeb\xf5\xa8",
 	}, {
 		.klen	= 16,
 		.key    = "\x63\x68\x69\x63\x6b\x65\x6e\x20"
@@ -31084,6 +31090,8 @@ static const struct cipher_testvec cts_mode_tv_template[] = {
 			  "\x1b\x55\x49\xd2\xf8\x38\x02\x9e"
 			  "\x39\x31\x25\x23\xa7\x86\x62\xd5"
 			  "\xbe\x7f\xcb\xcc\x98\xeb\xf5",
+		.iv_out	= "\xb3\xff\xfd\x94\x0c\x16\xa1\x8c"
+			  "\x1b\x55\x49\xd2\xf8\x38\x02\x9e",
 	}, {
 		.klen	= 16,
 		.key    = "\x63\x68\x69\x63\x6b\x65\x6e\x20"
@@ -31101,6 +31109,8 @@ static const struct cipher_testvec cts_mode_tv_template[] = {
 			  "\x3b\xc1\x03\xe1\xa1\x94\xbb\xd8"
 			  "\x39\x31\x25\x23\xa7\x86\x62\xd5"
 			  "\xbe\x7f\xcb\xcc\x98\xeb\xf5\xa8",
+		.iv_out	= "\x9d\xad\x8b\xbb\x96\xc4\xcd\xc0"
+			  "\x3b\xc1\x03\xe1\xa1\x94\xbb\xd8",
 	}, {
 		.klen	= 16,
 		.key    = "\x63\x68\x69\x63\x6b\x65\x6e\x20"
@@ -31122,6 +31132,8 @@ static const struct cipher_testvec cts_mode_tv_template[] = {
 			  "\x26\x73\x0d\xbc\x2f\x7b\xc8\x40"
 			  "\x9d\xad\x8b\xbb\x96\xc4\xcd\xc0"
 			  "\x3b\xc1\x03\xe1\xa1\x94\xbb\xd8",
+		.iv_out	= "\x48\x07\xef\xe8\x36\xee\x89\xa5"
+			  "\x26\x73\x0d\xbc\x2f\x7b\xc8\x40",
 	}
 };
 
-- 
2.20.1

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Ard Biesheuvel]

(add Gilad for cc-ree)

On Tue, 19 May 2020 at 21:02, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Stephan reports that the arm64 implementation of cts(cbc(aes)) deviates
> from the generic implementation in what it returns as the output IV. So
> fix this, and add some test vectors to catch other non-compliant
> implementations.
>
> Stephan, could you provide a reference for the NIST validation tool and
> how it flags this behaviour as non-compliant? Thanks.
>
> Cc: Stephan Mueller <smueller@chronox.de>
>
> Ard Biesheuvel (2):
>   crypto: arm64/aes - align output IV with generic CBC-CTS driver
>   crypto: testmgr - add output IVs for AES-CBC with ciphertext stealing
>
>  arch/arm64/crypto/aes-modes.S |  2 ++
>  crypto/testmgr.h              | 12 ++++++++++++
>  2 files changed, 14 insertions(+)
>
> --
> 2.20.1
>

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Stephan Mueller]

Am Dienstag, 19. Mai 2020, 21:02:09 CEST schrieb Ard Biesheuvel:

Hi Ard,

> Stephan reports that the arm64 implementation of cts(cbc(aes)) deviates
> from the generic implementation in what it returns as the output IV. So
> fix this, and add some test vectors to catch other non-compliant
> implementations.
> 
> Stephan, could you provide a reference for the NIST validation tool and
> how it flags this behaviour as non-compliant? Thanks.

The test definition that identified the inconsistent behavior is specified 
with [1]. Note, this testing is intended to become an RFC standard.

To facilitate that testing, NIST offers an internet service, the ACVP server, 
that allows obtaining test vectors and uploading responses. You see the large 
number of concluded testing with [2]. A particular completion of the CTS 
testing I finished yesterday is given in [3]. That particular testing was also 
performed on an ARM system with CE where the issue was detected.

I am performing the testing with [4] that has an extension to test the kernel 
crypto API.

[1] https://github.com/usnistgov/ACVP/blob/master/artifacts/draft-celi-acvp-block-ciph-00.txt#L366

[2] https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program/
validation-search?searchMode=validation&family=1&productType=-1&ipp=25

[3] https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program/
details?validation=32608

[4] https://github.com/smuellerDD/acvpparser
> 
> Cc: Stephan Mueller <smueller@chronox.de>
> 
> Ard Biesheuvel (2):
>   crypto: arm64/aes - align output IV with generic CBC-CTS driver
>   crypto: testmgr - add output IVs for AES-CBC with ciphertext stealing
> 
>  arch/arm64/crypto/aes-modes.S |  2 ++
>  crypto/testmgr.h              | 12 ++++++++++++
>  2 files changed, 14 insertions(+)


Ciao
Stephan

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Ard Biesheuvel]

On Wed, 20 May 2020 at 08:03, Stephan Mueller <smueller@chronox.de> wrote:
>
> Am Dienstag, 19. Mai 2020, 21:02:09 CEST schrieb Ard Biesheuvel:
>
> Hi Ard,
>
> > Stephan reports that the arm64 implementation of cts(cbc(aes)) deviates
> > from the generic implementation in what it returns as the output IV. So
> > fix this, and add some test vectors to catch other non-compliant
> > implementations.
> >
> > Stephan, could you provide a reference for the NIST validation tool and
> > how it flags this behaviour as non-compliant? Thanks.
>
> The test definition that identified the inconsistent behavior is specified
> with [1]. Note, this testing is intended to become an RFC standard.
>

Are you referring to the line

CT[j] = AES_CBC_CS_ENCRYPT(Key[i], PT[j])

where the CTS transform is invoked without an IV altogether? That
simply seems like a bug to me. In an abstract specification like this,
it would be insane for pseudocode functions to be stateful objects,
and there is nothing in the pseudocode that explicitly captures the
'output IV' of that function call.


> To facilitate that testing, NIST offers an internet service, the ACVP server,
> that allows obtaining test vectors and uploading responses. You see the large
> number of concluded testing with [2]. A particular completion of the CTS
> testing I finished yesterday is given in [3]. That particular testing was also
> performed on an ARM system with CE where the issue was detected.
>
> I am performing the testing with [4] that has an extension to test the kernel
> crypto API.
>

OK. So given that that neither the CTS spec nor this document makes
any mention of an output IV or what its value should be, my suggestion
would be to capture the IV directly from the ciphertext, rather than
relying on some unspecified behavior to give you the right data. Note
that we have other implementations of cts(cbc(aes)) in the kernel as
well (h/w accelerated ones) and if there is no specification that
defines this behavior, you really shouldn't be relying on it.


That 'specification' invokes AES_CBC_CS_ENCRYPT() twice using a
different prototype, without any mention whatsoever what the implied
value of IV[] is if it is missing. This is especially problematic
given that it seems to cover all of CS1/2/3, and the relation between
next IV and ciphertext is not even the same between those for inputs
that are a multiple of the blocksize.


> [1] https://github.com/usnistgov/ACVP/blob/master/artifacts/draft-celi-acvp-block-ciph-00.txt#L366
>
> [2] https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program/
> validation-search?searchMode=validation&family=1&productType=-1&ipp=25
>
> [3] https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program/
> details?validation=32608
>
> [4] https://github.com/smuellerDD/acvpparser
> >
> > Cc: Stephan Mueller <smueller@chronox.de>
> >
> > Ard Biesheuvel (2):
> >   crypto: arm64/aes - align output IV with generic CBC-CTS driver
> >   crypto: testmgr - add output IVs for AES-CBC with ciphertext stealing
> >
> >  arch/arm64/crypto/aes-modes.S |  2 ++
> >  crypto/testmgr.h              | 12 ++++++++++++
> >  2 files changed, 14 insertions(+)
>
>
> Ciao
> Stephan
>
>

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Stephan Mueller]

Am Mittwoch, 20. Mai 2020, 08:40:57 CEST schrieb Ard Biesheuvel:

Hi Ard,

> On Wed, 20 May 2020 at 08:03, Stephan Mueller <smueller@chronox.de> wrote:
> > Am Dienstag, 19. Mai 2020, 21:02:09 CEST schrieb Ard Biesheuvel:
> > 
> > Hi Ard,
> > 
> > > Stephan reports that the arm64 implementation of cts(cbc(aes)) deviates
> > > from the generic implementation in what it returns as the output IV. So
> > > fix this, and add some test vectors to catch other non-compliant
> > > implementations.
> > > 
> > > Stephan, could you provide a reference for the NIST validation tool and
> > > how it flags this behaviour as non-compliant? Thanks.
> > 
> > The test definition that identified the inconsistent behavior is specified
> > with [1]. Note, this testing is intended to become an RFC standard.
> 
> Are you referring to the line
> 
> CT[j] = AES_CBC_CS_ENCRYPT(Key[i], PT[j])
> 
> where the CTS transform is invoked without an IV altogether?

Precisely.

> That
> simply seems like a bug to me. In an abstract specification like this,
> it would be insane for pseudocode functions to be stateful objects,
> and there is nothing in the pseudocode that explicitly captures the
> 'output IV' of that function call.

I think the description may be updated by simply refer to IV[j-1]. Then you 
would not have a stateful operation, but you rest on the IV of the previous 
operation.

The state of all block chaining modes we currently have is defined with the 
IV. That is the reason why I mentioned it can be implemented stateless when I 
am able to get the IV output from the previous operation.

> 
> > To facilitate that testing, NIST offers an internet service, the ACVP
> > server, that allows obtaining test vectors and uploading responses. You
> > see the large number of concluded testing with [2]. A particular
> > completion of the CTS testing I finished yesterday is given in [3]. That
> > particular testing was also performed on an ARM system with CE where the
> > issue was detected.
> > 
> > I am performing the testing with [4] that has an extension to test the
> > kernel crypto API.
> 
> OK. So given that that neither the CTS spec nor this document makes
> any mention of an output IV or what its value should be, my suggestion
> would be to capture the IV directly from the ciphertext, rather than
> relying on some unspecified behavior to give you the right data. Note
> that we have other implementations of cts(cbc(aes)) in the kernel as
> well (h/w accelerated ones) and if there is no specification that
> defines this behavior, you really shouldn't be relying on it.

Agreed, but all I need is the IV from the previous round without relying on 
any state.
> 
> 
> That 'specification' invokes AES_CBC_CS_ENCRYPT() twice using a
> different prototype, without any mention whatsoever what the implied
> value of IV[] is if it is missing. This is especially problematic
> given that it seems to cover all of CS1/2/3, and the relation between
> next IV and ciphertext is not even the same between those for inputs
> that are a multiple of the blocksize.

I will relay that comment back to the authors for update.
> 
> > [1]
> > https://github.com/usnistgov/ACVP/blob/master/artifacts/draft-celi-acvp-b
> > lock-ciph-00.txt#L366
> > 
> > [2]
> > https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program
> > / validation-search?searchMode=validation&family=1&productType=-1&ipp=25
> > 
> > [3]
> > https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program
> > / details?validation=32608
> > 
> > [4] https://github.com/smuellerDD/acvpparser
> > 
> > > Cc: Stephan Mueller <smueller@chronox.de>
> > > 
> > > Ard Biesheuvel (2):
> > >   crypto: arm64/aes - align output IV with generic CBC-CTS driver
> > >   crypto: testmgr - add output IVs for AES-CBC with ciphertext stealing
> > >  
> > >  arch/arm64/crypto/aes-modes.S |  2 ++
> > >  crypto/testmgr.h              | 12 ++++++++++++
> > >  2 files changed, 14 insertions(+)
> > 
> > Ciao
> > Stephan


Ciao
Stephan

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Ard Biesheuvel]

On Wed, 20 May 2020 at 08:47, Stephan Mueller <smueller@chronox.de> wrote:
>
> Am Mittwoch, 20. Mai 2020, 08:40:57 CEST schrieb Ard Biesheuvel:
>
> Hi Ard,
>
> > On Wed, 20 May 2020 at 08:03, Stephan Mueller <smueller@chronox.de> wrote:
> > > Am Dienstag, 19. Mai 2020, 21:02:09 CEST schrieb Ard Biesheuvel:
> > >
> > > Hi Ard,
> > >
> > > > Stephan reports that the arm64 implementation of cts(cbc(aes)) deviates
> > > > from the generic implementation in what it returns as the output IV. So
> > > > fix this, and add some test vectors to catch other non-compliant
> > > > implementations.
> > > >
> > > > Stephan, could you provide a reference for the NIST validation tool and
> > > > how it flags this behaviour as non-compliant? Thanks.
> > >
> > > The test definition that identified the inconsistent behavior is specified
> > > with [1]. Note, this testing is intended to become an RFC standard.
> >
> > Are you referring to the line
> >
> > CT[j] = AES_CBC_CS_ENCRYPT(Key[i], PT[j])
> >
> > where the CTS transform is invoked without an IV altogether?
>
> Precisely.
>
> > That
> > simply seems like a bug to me. In an abstract specification like this,
> > it would be insane for pseudocode functions to be stateful objects,
> > and there is nothing in the pseudocode that explicitly captures the
> > 'output IV' of that function call.
>
> I think the description may be updated by simply refer to IV[j-1]. Then you
> would not have a stateful operation, but you rest on the IV of the previous
> operation.
>

But that value is not the value you are using now, right? I suspect
that the line

IV[i+1] = MSB(CT[j], IV.length)

needs to be duplicated in the inner loop for j, although that would
require different versions for CS1/2/3


> The state of all block chaining modes we currently have is defined with the
> IV. That is the reason why I mentioned it can be implemented stateless when I
> am able to get the IV output from the previous operation.
>

But it is simply the same as the penultimate block of ciphertext. So
you can simply capture it after encrypt, or before decrypt. There is
really no need to rely on the CTS transformation to pass it back to
you via the buffer that is only specified to provide an input to the
CTS transform.


> >
> > > To facilitate that testing, NIST offers an internet service, the ACVP
> > > server, that allows obtaining test vectors and uploading responses. You
> > > see the large number of concluded testing with [2]. A particular
> > > completion of the CTS testing I finished yesterday is given in [3]. That
> > > particular testing was also performed on an ARM system with CE where the
> > > issue was detected.
> > >
> > > I am performing the testing with [4] that has an extension to test the
> > > kernel crypto API.
> >
> > OK. So given that that neither the CTS spec nor this document makes
> > any mention of an output IV or what its value should be, my suggestion
> > would be to capture the IV directly from the ciphertext, rather than
> > relying on some unspecified behavior to give you the right data. Note
> > that we have other implementations of cts(cbc(aes)) in the kernel as
> > well (h/w accelerated ones) and if there is no specification that
> > defines this behavior, you really shouldn't be relying on it.
>
> Agreed, but all I need is the IV from the previous round without relying on
> any state.

So just grab it from the ciphertext of the previous round.

> >
> >
> > That 'specification' invokes AES_CBC_CS_ENCRYPT() twice using a
> > different prototype, without any mention whatsoever what the implied
> > value of IV[] is if it is missing. This is especially problematic
> > given that it seems to cover all of CS1/2/3, and the relation between
> > next IV and ciphertext is not even the same between those for inputs
> > that are a multiple of the blocksize.
>
> I will relay that comment back to the authors for update.

Thanks.


> >
> > > [1]
> > > https://github.com/usnistgov/ACVP/blob/master/artifacts/draft-celi-acvp-b
> > > lock-ciph-00.txt#L366
> > >
> > > [2]
> > > https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program
> > > / validation-search?searchMode=validation&family=1&productType=-1&ipp=25
> > >
> > > [3]
> > > https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program
> > > / details?validation=32608
> > >
> > > [4] https://github.com/smuellerDD/acvpparser
> > >
> > > > Cc: Stephan Mueller <smueller@chronox.de>
> > > >
> > > > Ard Biesheuvel (2):
> > > >   crypto: arm64/aes - align output IV with generic CBC-CTS driver
> > > >   crypto: testmgr - add output IVs for AES-CBC with ciphertext stealing
> > > >
> > > >  arch/arm64/crypto/aes-modes.S |  2 ++
> > > >  crypto/testmgr.h              | 12 ++++++++++++
> > > >  2 files changed, 14 insertions(+)
> > >
> > > Ciao
> > > Stephan
>
>
> Ciao
> Stephan
>
>

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Stephan Mueller]

Am Mittwoch, 20. Mai 2020, 08:54:10 CEST schrieb Ard Biesheuvel:

Hi Ard,

> On Wed, 20 May 2020 at 08:47, Stephan Mueller <smueller@chronox.de> wrote:
> > Am Mittwoch, 20. Mai 2020, 08:40:57 CEST schrieb Ard Biesheuvel:
> > 
> > Hi Ard,
> > 
> > > On Wed, 20 May 2020 at 08:03, Stephan Mueller <smueller@chronox.de> 
wrote:
> > > > Am Dienstag, 19. Mai 2020, 21:02:09 CEST schrieb Ard Biesheuvel:
> > > > 
> > > > Hi Ard,
> > > > 
> > > > > Stephan reports that the arm64 implementation of cts(cbc(aes))
> > > > > deviates
> > > > > from the generic implementation in what it returns as the output IV.
> > > > > So
> > > > > fix this, and add some test vectors to catch other non-compliant
> > > > > implementations.
> > > > > 
> > > > > Stephan, could you provide a reference for the NIST validation tool
> > > > > and
> > > > > how it flags this behaviour as non-compliant? Thanks.
> > > > 
> > > > The test definition that identified the inconsistent behavior is
> > > > specified
> > > > with [1]. Note, this testing is intended to become an RFC standard.
> > > 
> > > Are you referring to the line
> > > 
> > > CT[j] = AES_CBC_CS_ENCRYPT(Key[i], PT[j])
> > > 
> > > where the CTS transform is invoked without an IV altogether?
> > 
> > Precisely.
> > 
> > > That
> > > simply seems like a bug to me. In an abstract specification like this,
> > > it would be insane for pseudocode functions to be stateful objects,
> > > and there is nothing in the pseudocode that explicitly captures the
> > > 'output IV' of that function call.
> > 
> > I think the description may be updated by simply refer to IV[j-1]. Then
> > you
> > would not have a stateful operation, but you rest on the IV of the
> > previous
> > operation.
> 
> But that value is not the value you are using now, right? I suspect
> that the line
> 
> IV[i+1] = MSB(CT[j], IV.length)

Yes, such a line would be needed.
> 
> needs to be duplicated in the inner loop for j, although that would
> require different versions for CS1/2/3

Correct in the sense to specify exactly what the IV after a cipher operation 
actually is.
> 
> > The state of all block chaining modes we currently have is defined with
> > the
> > IV. That is the reason why I mentioned it can be implemented stateless
> > when I am able to get the IV output from the previous operation.
> 
> But it is simply the same as the penultimate block of ciphertext. So
> you can simply capture it after encrypt, or before decrypt. There is
> really no need to rely on the CTS transformation to pass it back to
> you via the buffer that is only specified to provide an input to the
> CTS transform.

Let me recheck that as I am not fully sure on that one. But if it can be 
handled that way, it would make life easier.
> 
> > > > To facilitate that testing, NIST offers an internet service, the ACVP
> > > > server, that allows obtaining test vectors and uploading responses.
> > > > You
> > > > see the large number of concluded testing with [2]. A particular
> > > > completion of the CTS testing I finished yesterday is given in [3].
> > > > That
> > > > particular testing was also performed on an ARM system with CE where
> > > > the
> > > > issue was detected.
> > > > 
> > > > I am performing the testing with [4] that has an extension to test the
> > > > kernel crypto API.
> > > 
> > > OK. So given that that neither the CTS spec nor this document makes
> > > any mention of an output IV or what its value should be, my suggestion
> > > would be to capture the IV directly from the ciphertext, rather than
> > > relying on some unspecified behavior to give you the right data. Note
> > > that we have other implementations of cts(cbc(aes)) in the kernel as
> > > well (h/w accelerated ones) and if there is no specification that
> > > defines this behavior, you really shouldn't be relying on it.
> > 
> > Agreed, but all I need is the IV from the previous round without relying
> > on
> > any state.
> 
> So just grab it from the ciphertext of the previous round.
> 
> > > That 'specification' invokes AES_CBC_CS_ENCRYPT() twice using a
> > > different prototype, without any mention whatsoever what the implied
> > > value of IV[] is if it is missing. This is especially problematic
> > > given that it seems to cover all of CS1/2/3, and the relation between
> > > next IV and ciphertext is not even the same between those for inputs
> > > that are a multiple of the blocksize.
> > 
> > I will relay that comment back to the authors for update.
> 
> Thanks.

Thank you for your ideas!
> 
> > > > [1]
> > > > https://github.com/usnistgov/ACVP/blob/master/artifacts/draft-celi-acv
> > > > p-b
> > > > lock-ciph-00.txt#L366
> > > > 
> > > > [2]
> > > > https://csrc.nist.gov/projects/cryptographic-algorithm-validation-prog
> > > > ram
> > > > /
> > > > validation-search?searchMode=validation&family=1&productType=-1&ipp=2
> > > > 5
> > > > 
> > > > [3]
> > > > https://csrc.nist.gov/projects/cryptographic-algorithm-validation-prog
> > > > ram
> > > > / details?validation=32608
> > > > 
> > > > [4] https://github.com/smuellerDD/acvpparser
> > > > 
> > > > > Cc: Stephan Mueller <smueller@chronox.de>
> > > > > 
> > > > > Ard Biesheuvel (2):
> > > > >   crypto: arm64/aes - align output IV with generic CBC-CTS driver
> > > > >   crypto: testmgr - add output IVs for AES-CBC with ciphertext
> > > > >   stealing
> > > > >  
> > > > >  arch/arm64/crypto/aes-modes.S |  2 ++
> > > > >  crypto/testmgr.h              | 12 ++++++++++++
> > > > >  2 files changed, 14 insertions(+)
> > > > 
> > > > Ciao
> > > > Stephan
> > 
> > Ciao
> > Stephan


Ciao
Stephan

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Ard Biesheuvel]

On Wed, 20 May 2020 at 09:01, Stephan Mueller <smueller@chronox.de> wrote:
>
> Am Mittwoch, 20. Mai 2020, 08:54:10 CEST schrieb Ard Biesheuvel:
>
> Hi Ard,
>
> > On Wed, 20 May 2020 at 08:47, Stephan Mueller <smueller@chronox.de> wrote:
...
> > > The state of all block chaining modes we currently have is defined with
> > > the
> > > IV. That is the reason why I mentioned it can be implemented stateless
> > > when I am able to get the IV output from the previous operation.
> >
> > But it is simply the same as the penultimate block of ciphertext. So
> > you can simply capture it after encrypt, or before decrypt. There is
> > really no need to rely on the CTS transformation to pass it back to
> > you via the buffer that is only specified to provide an input to the
> > CTS transform.
>
> Let me recheck that as I am not fully sure on that one. But if it can be
> handled that way, it would make life easier.

Please refer to patch 2. The .iv_out test vectors were all simply
copied from the appropriate offset into the associated .ctext member.

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Gilad Ben-Yossef]

Hi Ard,

Thank you for looping me in.

On Wed, May 20, 2020 at 10:09 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 20 May 2020 at 09:01, Stephan Mueller <smueller@chronox.de> wrote:
> >
> > Am Mittwoch, 20. Mai 2020, 08:54:10 CEST schrieb Ard Biesheuvel:
> >
> > Hi Ard,
> >
> > > On Wed, 20 May 2020 at 08:47, Stephan Mueller <smueller@chronox.de> wrote:
> ...
> > > > The state of all block chaining modes we currently have is defined with
> > > > the
> > > > IV. That is the reason why I mentioned it can be implemented stateless
> > > > when I am able to get the IV output from the previous operation.
> > >
> > > But it is simply the same as the penultimate block of ciphertext. So
> > > you can simply capture it after encrypt, or before decrypt. There is
> > > really no need to rely on the CTS transformation to pass it back to
> > > you via the buffer that is only specified to provide an input to the
> > > CTS transform.
> >
> > Let me recheck that as I am not fully sure on that one. But if it can be
> > handled that way, it would make life easier.
>
> Please refer to patch 2. The .iv_out test vectors were all simply
> copied from the appropriate offset into the associated .ctext member.

Not surprisingly since to the best of my understanding this behaviour
is not strictly specified, ccree currently fails the IV output check
with the 2nd version of the patch.

If I understand you correctly, the expected output IV is simply the
next to last block of the ciphertext?

Thanks,
Gilad



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

values of β will give rise to dom!

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Ard Biesheuvel]

On Thu, 21 May 2020 at 15:01, Gilad Ben-Yossef <gilad@benyossef.com> wrote:
>
> Hi Ard,
>
> Thank you for looping me in.
>
> On Wed, May 20, 2020 at 10:09 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 20 May 2020 at 09:01, Stephan Mueller <smueller@chronox.de> wrote:
> > >
> > > Am Mittwoch, 20. Mai 2020, 08:54:10 CEST schrieb Ard Biesheuvel:
> > >
> > > Hi Ard,
> > >
> > > > On Wed, 20 May 2020 at 08:47, Stephan Mueller <smueller@chronox.de> wrote:
> > ...
> > > > > The state of all block chaining modes we currently have is defined with
> > > > > the
> > > > > IV. That is the reason why I mentioned it can be implemented stateless
> > > > > when I am able to get the IV output from the previous operation.
> > > >
> > > > But it is simply the same as the penultimate block of ciphertext. So
> > > > you can simply capture it after encrypt, or before decrypt. There is
> > > > really no need to rely on the CTS transformation to pass it back to
> > > > you via the buffer that is only specified to provide an input to the
> > > > CTS transform.
> > >
> > > Let me recheck that as I am not fully sure on that one. But if it can be
> > > handled that way, it would make life easier.
> >
> > Please refer to patch 2. The .iv_out test vectors were all simply
> > copied from the appropriate offset into the associated .ctext member.
>
> Not surprisingly since to the best of my understanding this behaviour
> is not strictly specified, ccree currently fails the IV output check
> with the 2nd version of the patch.
>

That is what I suspected, hence the cc:

> If I understand you correctly, the expected output IV is simply the
> next to last block of the ciphertext?

Yes. But this happens to work for the generic case because the CTS
driver itself requires the encapsulated CBC mode to return the output
IV, which is simply passed through back to the caller. CTS mode itself
does not specify any kind of output IV, so we should not rely on this
behavior.

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Stephan Müller]

Am Donnerstag, 21. Mai 2020, 15:23:41 CEST schrieb Ard Biesheuvel:

Hi Ard,

> On Thu, 21 May 2020 at 15:01, Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> > Hi Ard,
> > 
> > Thank you for looping me in.
> > 
> > On Wed, May 20, 2020 at 10:09 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > On Wed, 20 May 2020 at 09:01, Stephan Mueller <smueller@chronox.de> 
wrote:
> > > > Am Mittwoch, 20. Mai 2020, 08:54:10 CEST schrieb Ard Biesheuvel:
> > > > 
> > > > Hi Ard,
> > > > 
> > > > > On Wed, 20 May 2020 at 08:47, Stephan Mueller <smueller@chronox.de> 
wrote:
> > > ...
> > > 
> > > > > > The state of all block chaining modes we currently have is defined
> > > > > > with
> > > > > > the
> > > > > > IV. That is the reason why I mentioned it can be implemented
> > > > > > stateless
> > > > > > when I am able to get the IV output from the previous operation.
> > > > > 
> > > > > But it is simply the same as the penultimate block of ciphertext. So
> > > > > you can simply capture it after encrypt, or before decrypt. There is
> > > > > really no need to rely on the CTS transformation to pass it back to
> > > > > you via the buffer that is only specified to provide an input to the
> > > > > CTS transform.
> > > > 
> > > > Let me recheck that as I am not fully sure on that one. But if it can
> > > > be
> > > > handled that way, it would make life easier.
> > > 
> > > Please refer to patch 2. The .iv_out test vectors were all simply
> > > copied from the appropriate offset into the associated .ctext member.
> > 
> > Not surprisingly since to the best of my understanding this behaviour
> > is not strictly specified, ccree currently fails the IV output check
> > with the 2nd version of the patch.
> 
> That is what I suspected, hence the cc:
> > If I understand you correctly, the expected output IV is simply the
> > next to last block of the ciphertext?
> 
> Yes. But this happens to work for the generic case because the CTS
> driver itself requires the encapsulated CBC mode to return the output
> IV, which is simply passed through back to the caller. CTS mode itself
> does not specify any kind of output IV, so we should not rely on this
> behavior.

Note, the update to the spec based on your suggestion is already in a merge 
request:

https://github.com/usnistgov/ACVP/issues/860

Thanks for your input.

Ciao
Stephan

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Ard Biesheuvel]

On Sat, 23 May 2020 at 20:52, Stephan Müller <smueller@chronox.de> wrote:
>
> Am Donnerstag, 21. Mai 2020, 15:23:41 CEST schrieb Ard Biesheuvel:
>
> Hi Ard,
>
> > On Thu, 21 May 2020 at 15:01, Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> > > Hi Ard,
> > >
> > > Thank you for looping me in.
> > >
> > > On Wed, May 20, 2020 at 10:09 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > On Wed, 20 May 2020 at 09:01, Stephan Mueller <smueller@chronox.de>
> wrote:
> > > > > Am Mittwoch, 20. Mai 2020, 08:54:10 CEST schrieb Ard Biesheuvel:
> > > > >
> > > > > Hi Ard,
> > > > >
> > > > > > On Wed, 20 May 2020 at 08:47, Stephan Mueller <smueller@chronox.de>
> wrote:
> > > > ...
> > > >
> > > > > > > The state of all block chaining modes we currently have is defined
> > > > > > > with
> > > > > > > the
> > > > > > > IV. That is the reason why I mentioned it can be implemented
> > > > > > > stateless
> > > > > > > when I am able to get the IV output from the previous operation.
> > > > > >
> > > > > > But it is simply the same as the penultimate block of ciphertext. So
> > > > > > you can simply capture it after encrypt, or before decrypt. There is
> > > > > > really no need to rely on the CTS transformation to pass it back to
> > > > > > you via the buffer that is only specified to provide an input to the
> > > > > > CTS transform.
> > > > >
> > > > > Let me recheck that as I am not fully sure on that one. But if it can
> > > > > be
> > > > > handled that way, it would make life easier.
> > > >
> > > > Please refer to patch 2. The .iv_out test vectors were all simply
> > > > copied from the appropriate offset into the associated .ctext member.
> > >
> > > Not surprisingly since to the best of my understanding this behaviour
> > > is not strictly specified, ccree currently fails the IV output check
> > > with the 2nd version of the patch.
> >
> > That is what I suspected, hence the cc:
> > > If I understand you correctly, the expected output IV is simply the
> > > next to last block of the ciphertext?
> >
> > Yes. But this happens to work for the generic case because the CTS
> > driver itself requires the encapsulated CBC mode to return the output
> > IV, which is simply passed through back to the caller. CTS mode itself
> > does not specify any kind of output IV, so we should not rely on this
> > behavior.
>
> Note, the update to the spec based on your suggestion is already in a merge
> request:
>
> https://github.com/usnistgov/ACVP/issues/860
>
> Thanks for your input.
>

Thanks for the head's up. I've left a comment there, as the proposed
change is not equivalent to the unspecified current behavior.

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Herbert Xu]

Ard Biesheuvel <ardb@kernel.org> wrote:
> Stephan reports that the arm64 implementation of cts(cbc(aes)) deviates
> from the generic implementation in what it returns as the output IV. So
> fix this, and add some test vectors to catch other non-compliant
> implementations.
> 
> Stephan, could you provide a reference for the NIST validation tool and
> how it flags this behaviour as non-compliant? Thanks.

I think our CTS and XTS are both broken with respect to af_alg.

The reason we use output IVs in general is to support chaining
which is required by algif_skcipher to break up large requests
into smaller ones.

For CTS and XTS that simply doesn't work.  So we should fix this
by changing algif_skcipher to not do chaining (and hence drop
support for large requests like algif_aead) for algorithms like
CTS/XTS.

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

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Ard Biesheuvel]

On Thu, 28 May 2020 at 09:33, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> Ard Biesheuvel <ardb@kernel.org> wrote:
> > Stephan reports that the arm64 implementation of cts(cbc(aes)) deviates
> > from the generic implementation in what it returns as the output IV. So
> > fix this, and add some test vectors to catch other non-compliant
> > implementations.
> >
> > Stephan, could you provide a reference for the NIST validation tool and
> > how it flags this behaviour as non-compliant? Thanks.
>
> I think our CTS and XTS are both broken with respect to af_alg.
>
> The reason we use output IVs in general is to support chaining
> which is required by algif_skcipher to break up large requests
> into smaller ones.
>
> For CTS and XTS that simply doesn't work.  So we should fix this
> by changing algif_skcipher to not do chaining (and hence drop
> support for large requests like algif_aead) for algorithms like
> CTS/XTS.
>

The reason we return output IVs for CBC is because our generic
implementation of CTS can wrap any CBC implementation, and relies on
this output IV rather than grabbing it from the ciphertext directly
(which may be tricky and is best left up to the CBC code)

For CTS itself, as well as XTS, returning an output IV is meaningless,
given that
a) the implementations rely on the skcipher_walk infrastructure to
present all input except the last bit in chunks that are a multiple of
the block size,
b) neither specification defines how chaining of inputs should work,
regardless of whether the preceding input was a multiple of the block
size or not.

The CS3 mode that we implement for CTS swaps the final two blocks
unconditionally. So even if the input is a whole multiple of the block
size, the preceding ciphertext will turn out differently if any output
happens to follow.

For XTS, the IV is encrypted before processing the first block, so
even if you do return an output IV, the subsequent invocations of the
skcipher need to omit the encryption, which we don't implement
currently.

So if you are saying that we should never split up algif_skcipher
requests into multiple calls into the underlying skcipher, then I
agree with you. Even if the generic CTS seems to work more or less as
expected by, e.g., the NIST validation tool, this is unspecified
behavior, and definitely broken in various other places.

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Herbert Xu]

On Thu, May 28, 2020 at 10:33:25AM +0200, Ard Biesheuvel wrote:
>
> The reason we return output IVs for CBC is because our generic
> implementation of CTS can wrap any CBC implementation, and relies on
> this output IV rather than grabbing it from the ciphertext directly
> (which may be tricky and is best left up to the CBC code)

No that's not the main reason.  The main user of chaining is
algif_skcipher.

> So if you are saying that we should never split up algif_skcipher
> requests into multiple calls into the underlying skcipher, then I
> agree with you. Even if the generic CTS seems to work more or less as
> expected by, e.g., the NIST validation tool, this is unspecified
> behavior, and definitely broken in various other places.

I was merely suggesting that requests to CTS/XTS shouldn't be
split up.  Doing it for others would be a serious regression.

However, having looked at this it would seem that the effort
in marking CTS/XTS is not that different to just adding support
to hold the last two blocks of data so that CTS/XTS can support
chaining.

I'll work on this.

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

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Ard Biesheuvel]

On Fri, 29 May 2020 at 10:05, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Thu, May 28, 2020 at 10:33:25AM +0200, Ard Biesheuvel wrote:
> >
> > The reason we return output IVs for CBC is because our generic
> > implementation of CTS can wrap any CBC implementation, and relies on
> > this output IV rather than grabbing it from the ciphertext directly
> > (which may be tricky and is best left up to the CBC code)
>
> No that's not the main reason.  The main user of chaining is
> algif_skcipher.
>

But many implementation do not return an output IV at all. The only
mode that requires it (for the selftests to pass) is CBC.

> > So if you are saying that we should never split up algif_skcipher
> > requests into multiple calls into the underlying skcipher, then I
> > agree with you. Even if the generic CTS seems to work more or less as
> > expected by, e.g., the NIST validation tool, this is unspecified
> > behavior, and definitely broken in various other places.
>
> I was merely suggesting that requests to CTS/XTS shouldn't be
> split up.  Doing it for others would be a serious regression.
>

Given that in these cases, doing so will give incorrect results even
if the input size is a whole multiple of the block size, I agree that
adding this restriction will not break anything that is working
consistently at the moment.

But could you elaborate on the serious regression for other cases? Do
you have anything particular in mind?

> However, having looked at this it would seem that the effort
> in marking CTS/XTS is not that different to just adding support
> to hold the last two blocks of data so that CTS/XTS can support
> chaining.
>

For XTS, we would have to carry some metadata around that tells you
whether the initial encryption of the IV has occurred or not. In the
CTS case, you need two swap the last two blocks of ciphertext at the
very end.

So does that mean some kind of init/update/final model for skcipher? I
can see how that could address these issues (init() would encrypt the
IV for XTS, and final() would do the final block handling for CTS).
Just holding two blocks of data does not seem sufficient to me to
handle these issues.

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Herbert Xu]

On Fri, May 29, 2020 at 10:20:27AM +0200, Ard Biesheuvel wrote:
>
> But many implementation do not return an output IV at all. The only
> mode that requires it (for the selftests to pass) is CBC.

Most modes can be chained, e.g., CBC, PCBC, OFB, CFB and CTR.
As it stands algif_skcipher requres all algorithms to support
chaining.
 
> For XTS, we would have to carry some metadata around that tells you
> whether the initial encryption of the IV has occurred or not. In the

You're right, XTS in its current form cannot be chained.  So we
do need a way to mark that for algif_skcipher.

> CTS case, you need two swap the last two blocks of ciphertext at the
> very end.

CTS can be easily chained.  You just need to always keep two blocks
from being processed until you reach the end.

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

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Ard Biesheuvel]

On Fri, 29 May 2020 at 13:51, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Fri, May 29, 2020 at 10:20:27AM +0200, Ard Biesheuvel wrote:
> >
> > But many implementation do not return an output IV at all. The only
> > mode that requires it (for the selftests to pass) is CBC.
>
> Most modes can be chained, e.g., CBC, PCBC, OFB, CFB and CTR.
> As it stands algif_skcipher requres all algorithms to support
> chaining.
>

Even if this is the case, it requires that an skcipher implementation
stores an output IV in the buffer that skcipher request's IV field
points to. Currently, we only check whether this is the case for CBC
implementations, and so it is quite likely that lots of h/w
accelerators or arch code don't adhere to this today.


> > For XTS, we would have to carry some metadata around that tells you
> > whether the initial encryption of the IV has occurred or not. In the
>
> You're right, XTS in its current form cannot be chained.  So we
> do need a way to mark that for algif_skcipher.
>
> > CTS case, you need two swap the last two blocks of ciphertext at the
> > very end.
>
> CTS can be easily chained.  You just need to always keep two blocks
> from being processed until you reach the end.
>

This might be feasible for the generic CTS driver wrapping h/w
accelerated CBC. But how is this supposed to work, e.g., for the two
existing h/w implementations of cts(cbc(aes)) that currently ignore
this?

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Herbert Xu]

On Fri, May 29, 2020 at 02:00:14PM +0200, Ard Biesheuvel wrote:
>
> Even if this is the case, it requires that an skcipher implementation
> stores an output IV in the buffer that skcipher request's IV field
> points to. Currently, we only check whether this is the case for CBC
> implementations, and so it is quite likely that lots of h/w
> accelerators or arch code don't adhere to this today.

They are and have always been broken because algif_skcipher has
always relied on this.

> This might be feasible for the generic CTS driver wrapping h/w
> accelerated CBC. But how is this supposed to work, e.g., for the two
> existing h/w implementations of cts(cbc(aes)) that currently ignore
> this?

They'll have to disable chaining.

The way I'm doing this would allow some implementations to allow
chaining while others of the same algorithm can disable chaining
and require the whole request to be presented together.

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

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Ard Biesheuvel]

On Fri, 29 May 2020 at 14:02, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Fri, May 29, 2020 at 02:00:14PM +0200, Ard Biesheuvel wrote:
> >
> > Even if this is the case, it requires that an skcipher implementation
> > stores an output IV in the buffer that skcipher request's IV field
> > points to. Currently, we only check whether this is the case for CBC
> > implementations, and so it is quite likely that lots of h/w
> > accelerators or arch code don't adhere to this today.
>
> They are and have always been broken because algif_skcipher has
> always relied on this.
>

OK, so the undocumented assumption is that algif_skcipher requests are
delineated by ALG_SET_IV commands, and that anything that gets sent to
the socket in between should be treated as a single request, right? I
think that makes sense, but do note that this deviates from Stephan's
use case, where the ciphertext stealing block swap was applied after
every call into af_alg, with the IV being inherited from one request
to the next. I think that case was invalid to begin with, I just hope
no other use cases exist where this unspecified behavior is being
relied upon.

> > This might be feasible for the generic CTS driver wrapping h/w
> > accelerated CBC. But how is this supposed to work, e.g., for the two
> > existing h/w implementations of cts(cbc(aes)) that currently ignore
> > this?
>
> They'll have to disable chaining.
>
> The way I'm doing this would allow some implementations to allow
> chaining while others of the same algorithm can disable chaining
> and require the whole request to be presented together.
>

Fair enough.

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Herbert Xu]

On Fri, May 29, 2020 at 03:10:43PM +0200, Ard Biesheuvel wrote:
>
> OK, so the undocumented assumption is that algif_skcipher requests are
> delineated by ALG_SET_IV commands, and that anything that gets sent to
> the socket in between should be treated as a single request, right? I

Correct.

> think that makes sense, but do note that this deviates from Stephan's
> use case, where the ciphertext stealing block swap was applied after
> every call into af_alg, with the IV being inherited from one request
> to the next. I think that case was invalid to begin with, I just hope
> no other use cases exist where this unspecified behavior is being
> relied upon.

That does indeed sound broken.

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

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Ard Biesheuvel]

On Fri, 29 May 2020 at 15:19, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Fri, May 29, 2020 at 03:10:43PM +0200, Ard Biesheuvel wrote:
> >
> > OK, so the undocumented assumption is that algif_skcipher requests are
> > delineated by ALG_SET_IV commands, and that anything that gets sent to
> > the socket in between should be treated as a single request, right? I
>
> Correct.
>

So what about the final request? At which point do you decide to
return the final chunk of data that you have been holding back in
order to ensure that you can perform the final processing correctly if
it is not being followed by a ALG_SET_IV command?

Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

[Herbert Xu]

On Fri, May 29, 2020 at 03:41:08PM +0200, Ard Biesheuvel wrote:
>
> So what about the final request? At which point do you decide to
> return the final chunk of data that you have been holding back in
> order to ensure that you can perform the final processing correctly if
> it is not being followed by a ALG_SET_IV command?

When the MORE flag is unset.

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

[PATCH 0/3] crypto: skcipher - Add support for no chaining and partial chaining

[Herbert Xu]

This patch-set adds support to the Crypto API and algif_skcipher
for algorithms that cannot be chained, as well as ones that can
be chained if you withhold a certain number of blocks at the end.

It only modifies one algorithm to utilise this, namely cts-generic.
Changing others should be fairly straightforward.  In particular,
we should mark all the ones that don't support chaining (e.g., most
stream ciphers).

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

[PATCH 1/3] crypto: skcipher - Add final chunk size field for chaining

[Herbert Xu]

Crypto skcipher algorithms in general allow chaining to break
large operations into smaller ones based on multiples of the chunk
size.  However, some algorithms don't support chaining while others
(such as cts) only support chaining for the leading blocks.

This patch adds the necessary API support for these algorithms.  In
particular, a new request flag CRYPTO_TFM_REQ_MORE is added to allow
chaining for algorithms such as cts that cannot otherwise be chained.

A new algorithm attribute fcsize has also been added to indicate
how many blocks at the end of a request that cannot be chained and
therefore must be withheld if chaining is attempted.

This attribute can also be used to indicate that no chaining is
allowed.  Its value should be set to -1 in that case.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 include/crypto/skcipher.h |   24 ++++++++++++++++++++++++
 include/linux/crypto.h    |    1 +
 2 files changed, 25 insertions(+)

diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h
index 141e7690f9c31..8b864222e6ce4 100644
--- a/include/crypto/skcipher.h
+++ b/include/crypto/skcipher.h
@@ -97,6 +97,8 @@ struct crypto_sync_skcipher {
  * @walksize: Equal to the chunk size except in cases where the algorithm is
  * 	      considerably more efficient if it can operate on multiple chunks
  * 	      in parallel. Should be a multiple of chunksize.
+ * @fcsize: Number of bytes that must be processed together at the end.
+ *	     If set to -1 then chaining is not possible.
  * @base: Definition of a generic crypto algorithm.
  *
  * All fields except @ivsize are mandatory and must be filled.
@@ -114,6 +116,7 @@ struct skcipher_alg {
 	unsigned int ivsize;
 	unsigned int chunksize;
 	unsigned int walksize;
+	int fcsize;
 
 	struct crypto_alg base;
 };
@@ -279,6 +282,11 @@ static inline unsigned int crypto_skcipher_alg_chunksize(
 	return alg->chunksize;
 }
 
+static inline int crypto_skcipher_alg_fcsize(struct skcipher_alg *alg)
+{
+	return alg->fcsize;
+}
+
 /**
  * crypto_skcipher_chunksize() - obtain chunk size
  * @tfm: cipher handle
@@ -296,6 +304,22 @@ static inline unsigned int crypto_skcipher_chunksize(
 	return crypto_skcipher_alg_chunksize(crypto_skcipher_alg(tfm));
 }
 
+/**
+ * crypto_skcipher_fcsize() - obtain number of final bytes
+ * @tfm: cipher handle
+ *
+ * For algorithms such as CTS the final chunks cannot be chained.
+ * This returns the number of final bytes that must be withheld
+ * when chaining.
+ *
+ * Return: number of final bytes
+ */
+static inline unsigned int crypto_skcipher_fcsize(
+	struct crypto_skcipher *tfm)
+{
+	return crypto_skcipher_alg_fcsize(crypto_skcipher_alg(tfm));
+}
+
 static inline unsigned int crypto_sync_skcipher_blocksize(
 	struct crypto_sync_skcipher *tfm)
 {
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 763863dbc079a..d80dccf472595 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -110,6 +110,7 @@
 #define CRYPTO_TFM_REQ_FORBID_WEAK_KEYS	0x00000100
 #define CRYPTO_TFM_REQ_MAY_SLEEP	0x00000200
 #define CRYPTO_TFM_REQ_MAY_BACKLOG	0x00000400
+#define CRYPTO_TFM_REQ_MORE		0x00000800
 
 /*
  * Miscellaneous stuff.

Re: [PATCH 1/3] crypto: skcipher - Add final chunk size field for chaining

[Stephan Mueller]

Am Freitag, 12. Juni 2020, 14:07:36 CEST schrieb Herbert Xu:

Hi Herbert,

> Crypto skcipher algorithms in general allow chaining to break
> large operations into smaller ones based on multiples of the chunk
> size.  However, some algorithms don't support chaining while others
> (such as cts) only support chaining for the leading blocks.
> 
> This patch adds the necessary API support for these algorithms.  In
> particular, a new request flag CRYPTO_TFM_REQ_MORE is added to allow
> chaining for algorithms such as cts that cannot otherwise be chained.
> 
> A new algorithm attribute fcsize has also been added to indicate
> how many blocks at the end of a request that cannot be chained and
> therefore must be withheld if chaining is attempted.
> 
> This attribute can also be used to indicate that no chaining is
> allowed.  Its value should be set to -1 in that case.

Thanks for the patch set.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> ---
> 
>  include/crypto/skcipher.h |   24 ++++++++++++++++++++++++
>  include/linux/crypto.h    |    1 +
>  2 files changed, 25 insertions(+)
> 
> diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h
> index 141e7690f9c31..8b864222e6ce4 100644
> --- a/include/crypto/skcipher.h
> +++ b/include/crypto/skcipher.h
> @@ -97,6 +97,8 @@ struct crypto_sync_skcipher {
>   * @walksize: Equal to the chunk size except in cases where the algorithm
> is * 	      considerably more efficient if it can operate on multiple
> chunks * 	      in parallel. Should be a multiple of chunksize.
> + * @fcsize: Number of bytes that must be processed together at the end.
> + *	     If set to -1 then chaining is not possible.
>   * @base: Definition of a generic crypto algorithm.
>   *
>   * All fields except @ivsize are mandatory and must be filled.
> @@ -114,6 +116,7 @@ struct skcipher_alg {
>  	unsigned int ivsize;
>  	unsigned int chunksize;
>  	unsigned int walksize;
> +	int fcsize;
> 
>  	struct crypto_alg base;
>  };
> @@ -279,6 +282,11 @@ static inline unsigned int
> crypto_skcipher_alg_chunksize( return alg->chunksize;
>  }
> 
> +static inline int crypto_skcipher_alg_fcsize(struct skcipher_alg *alg)
> +{
> +	return alg->fcsize;
> +}
> +
>  /**
>   * crypto_skcipher_chunksize() - obtain chunk size
>   * @tfm: cipher handle
> @@ -296,6 +304,22 @@ static inline unsigned int crypto_skcipher_chunksize(
>  	return crypto_skcipher_alg_chunksize(crypto_skcipher_alg(tfm));
>  }
> 
> +/**
> + * crypto_skcipher_fcsize() - obtain number of final bytes
> + * @tfm: cipher handle
> + *
> + * For algorithms such as CTS the final chunks cannot be chained.
> + * This returns the number of final bytes that must be withheld
> + * when chaining.
> + *
> + * Return: number of final bytes
> + */
> +static inline unsigned int crypto_skcipher_fcsize(
> +	struct crypto_skcipher *tfm)
> +{
> +	return crypto_skcipher_alg_fcsize(crypto_skcipher_alg(tfm));

Don't we have an implicit signedness conversion here? int -> unsigned int?
> +}
> +
>  static inline unsigned int crypto_sync_skcipher_blocksize(
>  	struct crypto_sync_skcipher *tfm)
>  {
> diff --git a/include/linux/crypto.h b/include/linux/crypto.h
> index 763863dbc079a..d80dccf472595 100644
> --- a/include/linux/crypto.h
> +++ b/include/linux/crypto.h
> @@ -110,6 +110,7 @@
>  #define CRYPTO_TFM_REQ_FORBID_WEAK_KEYS	0x00000100
>  #define CRYPTO_TFM_REQ_MAY_SLEEP	0x00000200
>  #define CRYPTO_TFM_REQ_MAY_BACKLOG	0x00000400
> +#define CRYPTO_TFM_REQ_MORE		0x00000800
> 
>  /*
>   * Miscellaneous stuff.


Ciao
Stephan

Re: [PATCH 1/3] crypto: skcipher - Add final chunk size field for chaining

[Herbert Xu]

On Fri, Jun 12, 2020 at 02:15:52PM +0200, Stephan Mueller wrote:
>
> > +static inline unsigned int crypto_skcipher_fcsize(
> > +	struct crypto_skcipher *tfm)
> > +{
> > +	return crypto_skcipher_alg_fcsize(crypto_skcipher_alg(tfm));
> 
> Don't we have an implicit signedness conversion here? int -> unsigned int?

Good catch.  It's supposed to be int.  Let me fix 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

[v2 PATCH 0/3] crypto: skcipher - Add support for no chaining and partial chaining

[Herbert Xu]

v2

Fixed return type of crypto_skcipher_fcsize.

--

This patch-set adds support to the Crypto API and algif_skcipher
for algorithms that cannot be chained, as well as ones that can
be chained if you withhold a certain number of blocks at the end.

It only modifies one algorithm to utilise this, namely cts-generic.
Changing others should be fairly straightforward.  In particular,
we should mark all the ones that don't support chaining (e.g., most
stream ciphers).

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

[v2 PATCH 1/3] crypto: skcipher - Add final chunk size field for chaining

[Herbert Xu]

Crypto skcipher algorithms in general allow chaining to break
large operations into smaller ones based on multiples of the chunk
size.  However, some algorithms don't support chaining while others
(such as cts) only support chaining for the leading blocks.

This patch adds the necessary API support for these algorithms.  In
particular, a new request flag CRYPTO_TFM_REQ_MORE is added to allow
chaining for algorithms such as cts that cannot otherwise be chained.

A new algorithm attribute fcsize has also been added to indicate
how many blocks at the end of a request that cannot be chained and
therefore must be withheld if chaining is attempted.

This attribute can also be used to indicate that no chaining is
allowed.  Its value should be set to -1 in that case.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 include/crypto/skcipher.h |   23 +++++++++++++++++++++++
 include/linux/crypto.h    |    1 +
 2 files changed, 24 insertions(+)

diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h
index 141e7690f9c31..ae6abc44463b7 100644
--- a/include/crypto/skcipher.h
+++ b/include/crypto/skcipher.h
@@ -97,6 +97,8 @@ struct crypto_sync_skcipher {
  * @walksize: Equal to the chunk size except in cases where the algorithm is
  * 	      considerably more efficient if it can operate on multiple chunks
  * 	      in parallel. Should be a multiple of chunksize.
+ * @fcsize: Number of bytes that must be processed together at the end.
+ *	     If set to -1 then chaining is not possible.
  * @base: Definition of a generic crypto algorithm.
  *
  * All fields except @ivsize are mandatory and must be filled.
@@ -114,6 +116,7 @@ struct skcipher_alg {
 	unsigned int ivsize;
 	unsigned int chunksize;
 	unsigned int walksize;
+	int fcsize;
 
 	struct crypto_alg base;
 };
@@ -279,6 +282,11 @@ static inline unsigned int crypto_skcipher_alg_chunksize(
 	return alg->chunksize;
 }
 
+static inline int crypto_skcipher_alg_fcsize(struct skcipher_alg *alg)
+{
+	return alg->fcsize;
+}
+
 /**
  * crypto_skcipher_chunksize() - obtain chunk size
  * @tfm: cipher handle
@@ -296,6 +304,21 @@ static inline unsigned int crypto_skcipher_chunksize(
 	return crypto_skcipher_alg_chunksize(crypto_skcipher_alg(tfm));
 }
 
+/**
+ * crypto_skcipher_fcsize() - obtain number of final bytes
+ * @tfm: cipher handle
+ *
+ * For algorithms such as CTS the final chunks cannot be chained.
+ * This returns the number of final bytes that must be withheld
+ * when chaining.
+ *
+ * Return: number of final bytes
+ */
+static inline int crypto_skcipher_fcsize(struct crypto_skcipher *tfm)
+{
+	return crypto_skcipher_alg_fcsize(crypto_skcipher_alg(tfm));
+}
+
 static inline unsigned int crypto_sync_skcipher_blocksize(
 	struct crypto_sync_skcipher *tfm)
 {
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 763863dbc079a..d80dccf472595 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -110,6 +110,7 @@
 #define CRYPTO_TFM_REQ_FORBID_WEAK_KEYS	0x00000100
 #define CRYPTO_TFM_REQ_MAY_SLEEP	0x00000200
 #define CRYPTO_TFM_REQ_MAY_BACKLOG	0x00000400
+#define CRYPTO_TFM_REQ_MORE		0x00000800
 
 /*
  * Miscellaneous stuff.

[v2 PATCH 2/3] crypto: algif_skcipher - Add support for fcsize

[Herbert Xu]

As it stands algif_skcipher assumes all algorithms support chaining.
This patch teaches it about the new fcsize attribute which can be
used to disable chaining on a given algorithm.  It can also be used
to support chaining on algorithms such as cts that cannot otherwise
do chaining.  For that case algif_skcipher will also now set the
request flag CRYPTO_TFM_REQ_MORE when needed.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 crypto/algif_skcipher.c |   30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index f8a7fca3203eb..7fc873d28f4a7 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -57,12 +57,18 @@ static int _skcipher_recvmsg(struct socket *sock, struct msghdr *msg,
 	struct af_alg_ctx *ctx = ask->private;
 	struct crypto_skcipher *tfm = pask->private;
 	unsigned int bs = crypto_skcipher_chunksize(tfm);
+	unsigned int rflags = CRYPTO_TFM_REQ_MAY_SLEEP;
+	int fc = crypto_skcipher_fcsize(tfm);
 	struct af_alg_async_req *areq;
+	unsigned int min = bs;
 	int err = 0;
 	size_t len = 0;
 
-	if (!ctx->init || (ctx->more && ctx->used < bs)) {
-		err = af_alg_wait_for_data(sk, flags, bs);
+	if (fc >= 0)
+		min += fc;
+
+	if (!ctx->init || (ctx->more && ctx->used < min)) {
+		err = af_alg_wait_for_data(sk, flags, min);
 		if (err)
 			return err;
 	}
@@ -78,13 +84,22 @@ static int _skcipher_recvmsg(struct socket *sock, struct msghdr *msg,
 	if (err)
 		goto free;
 
+	err = -EINVAL;
+
 	/*
 	 * If more buffers are to be expected to be processed, process only
-	 * full block size buffers.
+	 * full block size buffers and withhold data according to fcsize.
 	 */
-	if (ctx->more || len < ctx->used)
+	if (ctx->more || len < ctx->used) {
+		if (fc < 0)
+			goto free;
+
+		len -= fc;
 		len -= len % bs;
 
+		rflags |= CRYPTO_TFM_REQ_MORE;
+	}
+
 	/*
 	 * Create a per request TX SGL for this request which tracks the
 	 * SG entries from the global TX SGL.
@@ -116,8 +131,7 @@ static int _skcipher_recvmsg(struct socket *sock, struct msghdr *msg,
 		areq->outlen = len;
 
 		skcipher_request_set_callback(&areq->cra_u.skcipher_req,
-					      CRYPTO_TFM_REQ_MAY_SLEEP,
-					      af_alg_async_cb, areq);
+					      rflags, af_alg_async_cb, areq);
 		err = ctx->enc ?
 			crypto_skcipher_encrypt(&areq->cra_u.skcipher_req) :
 			crypto_skcipher_decrypt(&areq->cra_u.skcipher_req);
@@ -129,9 +143,9 @@ static int _skcipher_recvmsg(struct socket *sock, struct msghdr *msg,
 		sock_put(sk);
 	} else {
 		/* Synchronous operation */
+		rflags |= CRYPTO_TFM_REQ_MAY_BACKLOG;
 		skcipher_request_set_callback(&areq->cra_u.skcipher_req,
-					      CRYPTO_TFM_REQ_MAY_SLEEP |
-					      CRYPTO_TFM_REQ_MAY_BACKLOG,
+					      rflags,
 					      crypto_req_done, &ctx->wait);
 		err = crypto_wait_req(ctx->enc ?
 			crypto_skcipher_encrypt(&areq->cra_u.skcipher_req) :

[v2 PATCH 3/3] crypto: cts - Add support for chaining

[Herbert Xu]

As it stands cts cannot do chaining.  That is, it always performs
the cipher-text stealing at the end of a request.  This patch adds
support for chaining when the CRYPTO_TM_REQ_MORE flag is set.

It also sets fcsize so that data can be withheld by the caller to
enable correct processing at the true end of a request.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 crypto/cts.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/crypto/cts.c b/crypto/cts.c
index 5e005c4f02215..3cca3112e7dca 100644
--- a/crypto/cts.c
+++ b/crypto/cts.c
@@ -100,7 +100,7 @@ static int cts_cbc_encrypt(struct skcipher_request *req)
 	struct crypto_cts_reqctx *rctx = skcipher_request_ctx(req);
 	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
 	struct skcipher_request *subreq = &rctx->subreq;
-	int bsize = crypto_skcipher_blocksize(tfm);
+	int bsize = crypto_skcipher_chunksize(tfm);
 	u8 d[MAX_CIPHER_BLOCKSIZE * 2] __aligned(__alignof__(u32));
 	struct scatterlist *sg;
 	unsigned int offset;
@@ -146,7 +146,7 @@ static int crypto_cts_encrypt(struct skcipher_request *req)
 	struct crypto_cts_reqctx *rctx = skcipher_request_ctx(req);
 	struct crypto_cts_ctx *ctx = crypto_skcipher_ctx(tfm);
 	struct skcipher_request *subreq = &rctx->subreq;
-	int bsize = crypto_skcipher_blocksize(tfm);
+	int bsize = crypto_skcipher_chunksize(tfm);
 	unsigned int nbytes = req->cryptlen;
 	unsigned int offset;
 
@@ -155,7 +155,7 @@ static int crypto_cts_encrypt(struct skcipher_request *req)
 	if (nbytes < bsize)
 		return -EINVAL;
 
-	if (nbytes == bsize) {
+	if (nbytes == bsize || req->base.flags & CRYPTO_TFM_REQ_MORE) {
 		skcipher_request_set_callback(subreq, req->base.flags,
 					      req->base.complete,
 					      req->base.data);
@@ -181,7 +181,7 @@ static int cts_cbc_decrypt(struct skcipher_request *req)
 	struct crypto_cts_reqctx *rctx = skcipher_request_ctx(req);
 	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
 	struct skcipher_request *subreq = &rctx->subreq;
-	int bsize = crypto_skcipher_blocksize(tfm);
+	int bsize = crypto_skcipher_chunksize(tfm);
 	u8 d[MAX_CIPHER_BLOCKSIZE * 2] __aligned(__alignof__(u32));
 	struct scatterlist *sg;
 	unsigned int offset;
@@ -240,7 +240,7 @@ static int crypto_cts_decrypt(struct skcipher_request *req)
 	struct crypto_cts_reqctx *rctx = skcipher_request_ctx(req);
 	struct crypto_cts_ctx *ctx = crypto_skcipher_ctx(tfm);
 	struct skcipher_request *subreq = &rctx->subreq;
-	int bsize = crypto_skcipher_blocksize(tfm);
+	int bsize = crypto_skcipher_chunksize(tfm);
 	unsigned int nbytes = req->cryptlen;
 	unsigned int offset;
 	u8 *space;
@@ -250,7 +250,7 @@ static int crypto_cts_decrypt(struct skcipher_request *req)
 	if (nbytes < bsize)
 		return -EINVAL;
 
-	if (nbytes == bsize) {
+	if (nbytes == bsize || req->base.flags & CRYPTO_TFM_REQ_MORE) {
 		skcipher_request_set_callback(subreq, req->base.flags,
 					      req->base.complete,
 					      req->base.data);
@@ -297,7 +297,7 @@ static int crypto_cts_init_tfm(struct crypto_skcipher *tfm)
 	ctx->child = cipher;
 
 	align = crypto_skcipher_alignmask(tfm);
-	bsize = crypto_skcipher_blocksize(cipher);
+	bsize = crypto_skcipher_chunksize(cipher);
 	reqsize = ALIGN(sizeof(struct crypto_cts_reqctx) +
 			crypto_skcipher_reqsize(cipher),
 			crypto_tfm_ctx_alignment()) +
@@ -366,11 +366,12 @@ static int crypto_cts_create(struct crypto_template *tmpl, struct rtattr **tb)
 
 	inst->alg.base.cra_flags = alg->base.cra_flags & CRYPTO_ALG_ASYNC;
 	inst->alg.base.cra_priority = alg->base.cra_priority;
-	inst->alg.base.cra_blocksize = alg->base.cra_blocksize;
+	inst->alg.base.cra_blocksize = 1;
 	inst->alg.base.cra_alignmask = alg->base.cra_alignmask;
 
 	inst->alg.ivsize = alg->base.cra_blocksize;
-	inst->alg.chunksize = crypto_skcipher_alg_chunksize(alg);
+	inst->alg.chunksize = alg->base.cra_blocksize;
+	inst->alg.fcsize = inst->alg.chunksize * 2;
 	inst->alg.min_keysize = crypto_skcipher_alg_min_keysize(alg);
 	inst->alg.max_keysize = crypto_skcipher_alg_max_keysize(alg);
 

Re: [v2 PATCH 0/3] crypto: skcipher - Add support for no chaining and partial chaining

[Ard Biesheuvel]

On Fri, 12 Jun 2020 at 14:21, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> v2
>
> Fixed return type of crypto_skcipher_fcsize.
>
> --
>
> This patch-set adds support to the Crypto API and algif_skcipher
> for algorithms that cannot be chained, as well as ones that can
> be chained if you withhold a certain number of blocks at the end.
>
> It only modifies one algorithm to utilise this, namely cts-generic.
> Changing others should be fairly straightforward.  In particular,
> we should mark all the ones that don't support chaining (e.g., most
> stream ciphers).
>

I understand that there is an oversight here that we need to address,
but I am not crazy about this approach, tbh.

First of all, the default fcsize for all existing XTS implementations
should be -1 as well, given that chaining is currently not supported
at all at the sckipher interface layer for any of them (due to the
fact that the IV gets encrypted with a different key at the start of
the operation). This also means it is going to be rather tricky to
implement for h/w accelerated XTS implementations, and it seems to me
that the only way to deal with this is to decrypt the IV in software
before chaining the next operation, which is rather horrid and needs
to be implemented by all of them.

Given that

a) this is wholly an AF_ALG issue, as there are no in-kernel users
currently suffering from this afaik,
b) using AF_ALG to get access to software implementations is rather
pointless in general, given that userspace can simply issue the same
instructions directly
c) fixing all XTS and CTS implementation on all arches and all
accelerators is not a small task

wouldn't it be better to special case XTS and CBC-CTS in
algif_skcipher instead, rather than polluting the skipcher API this
way?

Re: [v2 PATCH 0/3] crypto: skcipher - Add support for no chaining and partial chaining

[Herbert Xu]

On Fri, Jun 12, 2020 at 06:10:57PM +0200, Ard Biesheuvel wrote:
>
> First of all, the default fcsize for all existing XTS implementations
> should be -1 as well, given that chaining is currently not supported
> at all at the sckipher interface layer for any of them (due to the
> fact that the IV gets encrypted with a different key at the start of

Sure.  I was just too lazy to actually set the -1 everywhere.  I'll
try to do that before I repost again.

> the operation). This also means it is going to be rather tricky to
> implement for h/w accelerated XTS implementations, and it seems to me
> that the only way to deal with this is to decrypt the IV in software
> before chaining the next operation, which is rather horrid and needs
> to be implemented by all of them.

I don't think we should support chaining for XTS at all so I don't
see why we need to worry about the hardware accelerated XTS code.

> Given that
> 
> a) this is wholly an AF_ALG issue, as there are no in-kernel users
> currently suffering from this afaik,
> b) using AF_ALG to get access to software implementations is rather
> pointless in general, given that userspace can simply issue the same
> instructions directly
> c) fixing all XTS and CTS implementation on all arches and all
> accelerators is not a small task
> 
> wouldn't it be better to special case XTS and CBC-CTS in
> algif_skcipher instead, rather than polluting the skipcher API this
> way?

As I said we need to be able to differentiate between the ones
that can chain vs. the ones that can't.  Putting this knowledge
directly into algif_skcipher is just too horrid.

The alternative is to add this marker into the algorithms.  My
point was that if you're going to do that you might as well go
a step further and allow cts to chain as it is so straightforward.

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

Re: [v2 PATCH 0/3] crypto: skcipher - Add support for no chaining and partial chaining

[Ard Biesheuvel]

On Mon, 15 Jun 2020 at 09:30, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Fri, Jun 12, 2020 at 06:10:57PM +0200, Ard Biesheuvel wrote:
> >
> > First of all, the default fcsize for all existing XTS implementations
> > should be -1 as well, given that chaining is currently not supported
> > at all at the sckipher interface layer for any of them (due to the
> > fact that the IV gets encrypted with a different key at the start of
>
> Sure.  I was just too lazy to actually set the -1 everywhere.  I'll
> try to do that before I repost again.
>

Fair enough

> > the operation). This also means it is going to be rather tricky to
> > implement for h/w accelerated XTS implementations, and it seems to me
> > that the only way to deal with this is to decrypt the IV in software
> > before chaining the next operation, which is rather horrid and needs
> > to be implemented by all of them.
>
> I don't think we should support chaining for XTS at all so I don't
> see why we need to worry about the hardware accelerated XTS code.
>

I would prefer that. But if it is fine to disallow chaining altogether
for XTS, why can't we do the same for cbc-cts? In both cases, user
space cannot be relying on it today, since the output is incorrect,
even for inputs that are a round multiple of the block size but are
broken up and chained.

> > Given that
> >
> > a) this is wholly an AF_ALG issue, as there are no in-kernel users
> > currently suffering from this afaik,
> > b) using AF_ALG to get access to software implementations is rather
> > pointless in general, given that userspace can simply issue the same
> > instructions directly
> > c) fixing all XTS and CTS implementation on all arches and all
> > accelerators is not a small task
> >
> > wouldn't it be better to special case XTS and CBC-CTS in
> > algif_skcipher instead, rather than polluting the skipcher API this
> > way?
>
> As I said we need to be able to differentiate between the ones
> that can chain vs. the ones that can't.  Putting this knowledge
> directly into algif_skcipher is just too horrid.
>

No disagreement on the horrid. But polluting the API for an issue that
only affects AF_ALG, which can't possibly be working as expected right
now is not a great thing either.

> The alternative is to add this marker into the algorithms.  My
> point was that if you're going to do that you might as well go
> a step further and allow cts to chain as it is so straightforward.
>

Given the fact that algos that require chaining are broken today and
nobody noticed until Stephan started relying on the skcipher request
object's IV field magically retaining its value on subsequent reuse, I
would prefer it if we could simply mark all of them as non-chainable
and be done with it. (Note that Stephan's case was invalid to begin
with)

Re: [v2 PATCH 0/3] crypto: skcipher - Add support for no chaining and partial chaining

[Eric Biggers]

On Mon, Jun 15, 2020 at 09:50:50AM +0200, Ard Biesheuvel wrote:
> On Mon, 15 Jun 2020 at 09:30, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> >
> > On Fri, Jun 12, 2020 at 06:10:57PM +0200, Ard Biesheuvel wrote:
> > >
> > > First of all, the default fcsize for all existing XTS implementations
> > > should be -1 as well, given that chaining is currently not supported
> > > at all at the sckipher interface layer for any of them (due to the
> > > fact that the IV gets encrypted with a different key at the start of
> >
> > Sure.  I was just too lazy to actually set the -1 everywhere.  I'll
> > try to do that before I repost again.
> >
> 
> Fair enough
> 
> > > the operation). This also means it is going to be rather tricky to
> > > implement for h/w accelerated XTS implementations, and it seems to me
> > > that the only way to deal with this is to decrypt the IV in software
> > > before chaining the next operation, which is rather horrid and needs
> > > to be implemented by all of them.
> >
> > I don't think we should support chaining for XTS at all so I don't
> > see why we need to worry about the hardware accelerated XTS code.
> >
> 
> I would prefer that. But if it is fine to disallow chaining altogether
> for XTS, why can't we do the same for cbc-cts? In both cases, user
> space cannot be relying on it today, since the output is incorrect,
> even for inputs that are a round multiple of the block size but are
> broken up and chained.
> 
> > > Given that
> > >
> > > a) this is wholly an AF_ALG issue, as there are no in-kernel users
> > > currently suffering from this afaik,
> > > b) using AF_ALG to get access to software implementations is rather
> > > pointless in general, given that userspace can simply issue the same
> > > instructions directly
> > > c) fixing all XTS and CTS implementation on all arches and all
> > > accelerators is not a small task
> > >
> > > wouldn't it be better to special case XTS and CBC-CTS in
> > > algif_skcipher instead, rather than polluting the skipcher API this
> > > way?
> >
> > As I said we need to be able to differentiate between the ones
> > that can chain vs. the ones that can't.  Putting this knowledge
> > directly into algif_skcipher is just too horrid.
> >
> 
> No disagreement on the horrid. But polluting the API for an issue that
> only affects AF_ALG, which can't possibly be working as expected right
> now is not a great thing either.
> 
> > The alternative is to add this marker into the algorithms.  My
> > point was that if you're going to do that you might as well go
> > a step further and allow cts to chain as it is so straightforward.
> >
> 
> Given the fact that algos that require chaining are broken today and
> nobody noticed until Stephan started relying on the skcipher request
> object's IV field magically retaining its value on subsequent reuse, I
> would prefer it if we could simply mark all of them as non-chainable
> and be done with it. (Note that Stephan's case was invalid to begin
> with)

Wouldn't it make a lot more sense to make skcipher algorithms non-chainable by
default, and only opt-in the ones where chaining is actually working?  At the
moment we only test iv_out for CBC and CTR, so we can expect that all the others
are broken.

Note that wide-block modes such as Adiantum don't support chaining either.

Also, please use a better name than "fcsize".

- Eric

Re: [v2 PATCH 0/3] crypto: skcipher - Add support for no chaining and partial chaining

[Ard Biesheuvel]

On Mon, 15 Jun 2020 at 20:50, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Mon, Jun 15, 2020 at 09:50:50AM +0200, Ard Biesheuvel wrote:
> > On Mon, 15 Jun 2020 at 09:30, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > >
> > > On Fri, Jun 12, 2020 at 06:10:57PM +0200, Ard Biesheuvel wrote:
> > > >
> > > > First of all, the default fcsize for all existing XTS implementations
> > > > should be -1 as well, given that chaining is currently not supported
> > > > at all at the sckipher interface layer for any of them (due to the
> > > > fact that the IV gets encrypted with a different key at the start of
> > >
> > > Sure.  I was just too lazy to actually set the -1 everywhere.  I'll
> > > try to do that before I repost again.
> > >
> >
> > Fair enough
> >
> > > > the operation). This also means it is going to be rather tricky to
> > > > implement for h/w accelerated XTS implementations, and it seems to me
> > > > that the only way to deal with this is to decrypt the IV in software
> > > > before chaining the next operation, which is rather horrid and needs
> > > > to be implemented by all of them.
> > >
> > > I don't think we should support chaining for XTS at all so I don't
> > > see why we need to worry about the hardware accelerated XTS code.
> > >
> >
> > I would prefer that. But if it is fine to disallow chaining altogether
> > for XTS, why can't we do the same for cbc-cts? In both cases, user
> > space cannot be relying on it today, since the output is incorrect,
> > even for inputs that are a round multiple of the block size but are
> > broken up and chained.
> >
> > > > Given that
> > > >
> > > > a) this is wholly an AF_ALG issue, as there are no in-kernel users
> > > > currently suffering from this afaik,
> > > > b) using AF_ALG to get access to software implementations is rather
> > > > pointless in general, given that userspace can simply issue the same
> > > > instructions directly
> > > > c) fixing all XTS and CTS implementation on all arches and all
> > > > accelerators is not a small task
> > > >
> > > > wouldn't it be better to special case XTS and CBC-CTS in
> > > > algif_skcipher instead, rather than polluting the skipcher API this
> > > > way?
> > >
> > > As I said we need to be able to differentiate between the ones
> > > that can chain vs. the ones that can't.  Putting this knowledge
> > > directly into algif_skcipher is just too horrid.
> > >
> >
> > No disagreement on the horrid. But polluting the API for an issue that
> > only affects AF_ALG, which can't possibly be working as expected right
> > now is not a great thing either.
> >
> > > The alternative is to add this marker into the algorithms.  My
> > > point was that if you're going to do that you might as well go
> > > a step further and allow cts to chain as it is so straightforward.
> > >
> >
> > Given the fact that algos that require chaining are broken today and
> > nobody noticed until Stephan started relying on the skcipher request
> > object's IV field magically retaining its value on subsequent reuse, I
> > would prefer it if we could simply mark all of them as non-chainable
> > and be done with it. (Note that Stephan's case was invalid to begin
> > with)
>
> Wouldn't it make a lot more sense to make skcipher algorithms non-chainable by
> default, and only opt-in the ones where chaining is actually working?  At the
> moment we only test iv_out for CBC and CTR, so we can expect that all the others
> are broken.
>

Agreed. But there is a difference, though. XTS and CBC-CTS are
guaranteed not to be used in a chaining manner today, given that there
is no possible way you could get the right output for a AF_ALG request
that has been split into several skcipher requests: XTS has the IV
encryption that occurs only once, and CBC-CTS has the unconditional
swapping of the last two blocks, which occurs even if the output is a
whole multiple of the block size. Doing either of these more than once
will necessarily result in corrupt output.

For cases where chaining is more straight-forward, we may have users
that we are unaware of, so it is trickier. But that only means that we
may need to require iv_out support for other modes than CBC and CTR,
not that we need to add complexity like this series is doing.

For now, I would prefer to simply introduce a 'permits chaining' flag
that only gets set for CBC and CTR, and without any special handling
to support chaining for modes where doing so is non-trivial and known
to be broken and thus unused anyway. algif_skcipher can then attempt
to allocate the skcipher with the 'permits chaining' flag, and fall
back to allocating without, and deal with the difference accordingly.
Then, we can start adding support for this where necessary, i.e.,
start with any generic skcipher templates that are needed for chaining
support, and add support to other implementations gradually. We should
also start adding iv_out test cases and conditionalize the test on
whether the skcipher has the 'permits chaining' flag set.


> Note that wide-block modes such as Adiantum don't support chaining either.
>
> Also, please use a better name than "fcsize".
>

I don't think we need this at all.

Re: [v2 PATCH 0/3] crypto: skcipher - Add support for no chaining and partial chaining

[Herbert Xu]

On Mon, Jun 15, 2020 at 11:50:28AM -0700, Eric Biggers wrote:
>
> Wouldn't it make a lot more sense to make skcipher algorithms non-chainable by
> default, and only opt-in the ones where chaining is actually working?  At the
> moment we only test iv_out for CBC and CTR, so we can expect that all the others
> are broken.

Yes, I'm working through all the algorithms marking them.  If it
turns out that defaulting to off would result in a smaller patch
then I'm certainly going to do that.

> Note that wide-block modes such as Adiantum don't support chaining either.
> 
> Also, please use a better name than "fcsize".

Any suggestions?

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

Re: [v2 PATCH 0/3] crypto: skcipher - Add support for no chaining and partial chaining

[Eric Biggers]

On Tue, Jun 16, 2020 at 09:04:44PM +1000, Herbert Xu wrote:
> On Mon, Jun 15, 2020 at 11:50:28AM -0700, Eric Biggers wrote:
> >
> > Wouldn't it make a lot more sense to make skcipher algorithms non-chainable by
> > default, and only opt-in the ones where chaining is actually working?  At the
> > moment we only test iv_out for CBC and CTR, so we can expect that all the others
> > are broken.
> 
> Yes, I'm working through all the algorithms marking them.  If it
> turns out that defaulting to off would result in a smaller patch
> then I'm certainly going to do that.
> 
> > Note that wide-block modes such as Adiantum don't support chaining either.
> > 
> > Also, please use a better name than "fcsize".
> 
> Any suggestions?
> 

Just spelling it out as final_chunksize would be much clearer.
But maybe there's a better name.

- Eric

[PATCH 2/3] crypto: algif_skcipher - Add support for fcsize

[Herbert Xu]

As it stands algif_skcipher assumes all algorithms support chaining.
This patch teaches it about the new fcsize attribute which can be
used to disable chaining on a given algorithm.  It can also be used
to support chaining on algorithms such as cts that cannot otherwise
do chaining.  For that case algif_skcipher will also now set the
request flag CRYPTO_TFM_REQ_MORE when needed.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 crypto/algif_skcipher.c |   30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index f8a7fca3203eb..7fc873d28f4a7 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -57,12 +57,18 @@ static int _skcipher_recvmsg(struct socket *sock, struct msghdr *msg,
 	struct af_alg_ctx *ctx = ask->private;
 	struct crypto_skcipher *tfm = pask->private;
 	unsigned int bs = crypto_skcipher_chunksize(tfm);
+	unsigned int rflags = CRYPTO_TFM_REQ_MAY_SLEEP;
+	int fc = crypto_skcipher_fcsize(tfm);
 	struct af_alg_async_req *areq;
+	unsigned int min = bs;
 	int err = 0;
 	size_t len = 0;
 
-	if (!ctx->init || (ctx->more && ctx->used < bs)) {
-		err = af_alg_wait_for_data(sk, flags, bs);
+	if (fc >= 0)
+		min += fc;
+
+	if (!ctx->init || (ctx->more && ctx->used < min)) {
+		err = af_alg_wait_for_data(sk, flags, min);
 		if (err)
 			return err;
 	}
@@ -78,13 +84,22 @@ static int _skcipher_recvmsg(struct socket *sock, struct msghdr *msg,
 	if (err)
 		goto free;
 
+	err = -EINVAL;
+
 	/*
 	 * If more buffers are to be expected to be processed, process only
-	 * full block size buffers.
+	 * full block size buffers and withhold data according to fcsize.
 	 */
-	if (ctx->more || len < ctx->used)
+	if (ctx->more || len < ctx->used) {
+		if (fc < 0)
+			goto free;
+
+		len -= fc;
 		len -= len % bs;
 
+		rflags |= CRYPTO_TFM_REQ_MORE;
+	}
+
 	/*
 	 * Create a per request TX SGL for this request which tracks the
 	 * SG entries from the global TX SGL.
@@ -116,8 +131,7 @@ static int _skcipher_recvmsg(struct socket *sock, struct msghdr *msg,
 		areq->outlen = len;
 
 		skcipher_request_set_callback(&areq->cra_u.skcipher_req,
-					      CRYPTO_TFM_REQ_MAY_SLEEP,
-					      af_alg_async_cb, areq);
+					      rflags, af_alg_async_cb, areq);
 		err = ctx->enc ?
 			crypto_skcipher_encrypt(&areq->cra_u.skcipher_req) :
 			crypto_skcipher_decrypt(&areq->cra_u.skcipher_req);
@@ -129,9 +143,9 @@ static int _skcipher_recvmsg(struct socket *sock, struct msghdr *msg,
 		sock_put(sk);
 	} else {
 		/* Synchronous operation */
+		rflags |= CRYPTO_TFM_REQ_MAY_BACKLOG;
 		skcipher_request_set_callback(&areq->cra_u.skcipher_req,
-					      CRYPTO_TFM_REQ_MAY_SLEEP |
-					      CRYPTO_TFM_REQ_MAY_BACKLOG,
+					      rflags,
 					      crypto_req_done, &ctx->wait);
 		err = crypto_wait_req(ctx->enc ?
 			crypto_skcipher_encrypt(&areq->cra_u.skcipher_req) :

[PATCH 3/3] crypto: cts - Add support for chaining

[Herbert Xu]

As it stands cts cannot do chaining.  That is, it always performs
the cipher-text stealing at the end of a request.  This patch adds
support for chaining when the CRYPTO_TM_REQ_MORE flag is set.

It also sets fcsize so that data can be withheld by the caller to
enable correct processing at the true end of a request.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 crypto/cts.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/crypto/cts.c b/crypto/cts.c
index 5e005c4f02215..3cca3112e7dca 100644
--- a/crypto/cts.c
+++ b/crypto/cts.c
@@ -100,7 +100,7 @@ static int cts_cbc_encrypt(struct skcipher_request *req)
 	struct crypto_cts_reqctx *rctx = skcipher_request_ctx(req);
 	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
 	struct skcipher_request *subreq = &rctx->subreq;
-	int bsize = crypto_skcipher_blocksize(tfm);
+	int bsize = crypto_skcipher_chunksize(tfm);
 	u8 d[MAX_CIPHER_BLOCKSIZE * 2] __aligned(__alignof__(u32));
 	struct scatterlist *sg;
 	unsigned int offset;
@@ -146,7 +146,7 @@ static int crypto_cts_encrypt(struct skcipher_request *req)
 	struct crypto_cts_reqctx *rctx = skcipher_request_ctx(req);
 	struct crypto_cts_ctx *ctx = crypto_skcipher_ctx(tfm);
 	struct skcipher_request *subreq = &rctx->subreq;
-	int bsize = crypto_skcipher_blocksize(tfm);
+	int bsize = crypto_skcipher_chunksize(tfm);
 	unsigned int nbytes = req->cryptlen;
 	unsigned int offset;
 
@@ -155,7 +155,7 @@ static int crypto_cts_encrypt(struct skcipher_request *req)
 	if (nbytes < bsize)
 		return -EINVAL;
 
-	if (nbytes == bsize) {
+	if (nbytes == bsize || req->base.flags & CRYPTO_TFM_REQ_MORE) {
 		skcipher_request_set_callback(subreq, req->base.flags,
 					      req->base.complete,
 					      req->base.data);
@@ -181,7 +181,7 @@ static int cts_cbc_decrypt(struct skcipher_request *req)
 	struct crypto_cts_reqctx *rctx = skcipher_request_ctx(req);
 	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
 	struct skcipher_request *subreq = &rctx->subreq;
-	int bsize = crypto_skcipher_blocksize(tfm);
+	int bsize = crypto_skcipher_chunksize(tfm);
 	u8 d[MAX_CIPHER_BLOCKSIZE * 2] __aligned(__alignof__(u32));
 	struct scatterlist *sg;
 	unsigned int offset;
@@ -240,7 +240,7 @@ static int crypto_cts_decrypt(struct skcipher_request *req)
 	struct crypto_cts_reqctx *rctx = skcipher_request_ctx(req);
 	struct crypto_cts_ctx *ctx = crypto_skcipher_ctx(tfm);
 	struct skcipher_request *subreq = &rctx->subreq;
-	int bsize = crypto_skcipher_blocksize(tfm);
+	int bsize = crypto_skcipher_chunksize(tfm);
 	unsigned int nbytes = req->cryptlen;
 	unsigned int offset;
 	u8 *space;
@@ -250,7 +250,7 @@ static int crypto_cts_decrypt(struct skcipher_request *req)
 	if (nbytes < bsize)
 		return -EINVAL;
 
-	if (nbytes == bsize) {
+	if (nbytes == bsize || req->base.flags & CRYPTO_TFM_REQ_MORE) {
 		skcipher_request_set_callback(subreq, req->base.flags,
 					      req->base.complete,
 					      req->base.data);
@@ -297,7 +297,7 @@ static int crypto_cts_init_tfm(struct crypto_skcipher *tfm)
 	ctx->child = cipher;
 
 	align = crypto_skcipher_alignmask(tfm);
-	bsize = crypto_skcipher_blocksize(cipher);
+	bsize = crypto_skcipher_chunksize(cipher);
 	reqsize = ALIGN(sizeof(struct crypto_cts_reqctx) +
 			crypto_skcipher_reqsize(cipher),
 			crypto_tfm_ctx_alignment()) +
@@ -366,11 +366,12 @@ static int crypto_cts_create(struct crypto_template *tmpl, struct rtattr **tb)
 
 	inst->alg.base.cra_flags = alg->base.cra_flags & CRYPTO_ALG_ASYNC;
 	inst->alg.base.cra_priority = alg->base.cra_priority;
-	inst->alg.base.cra_blocksize = alg->base.cra_blocksize;
+	inst->alg.base.cra_blocksize = 1;
 	inst->alg.base.cra_alignmask = alg->base.cra_alignmask;
 
 	inst->alg.ivsize = alg->base.cra_blocksize;
-	inst->alg.chunksize = crypto_skcipher_alg_chunksize(alg);
+	inst->alg.chunksize = alg->base.cra_blocksize;
+	inst->alg.fcsize = inst->alg.chunksize * 2;
 	inst->alg.min_keysize = crypto_skcipher_alg_min_keysize(alg);
 	inst->alg.max_keysize = crypto_skcipher_alg_max_keysize(alg);