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 <[email protected]>
---
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.
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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 <[email protected]>
> ---
>
> 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
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Fri, 12 Jun 2020 at 14:21, Herbert Xu <[email protected]> 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?
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Mon, 15 Jun 2020 at 09:30, Herbert Xu <[email protected]> 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)
On Mon, Jun 15, 2020 at 09:50:50AM +0200, Ard Biesheuvel wrote:
> On Mon, 15 Jun 2020 at 09:30, Herbert Xu <[email protected]> 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
On Mon, 15 Jun 2020 at 20:50, Eric Biggers <[email protected]> wrote:
>
> On Mon, Jun 15, 2020 at 09:50:50AM +0200, Ard Biesheuvel wrote:
> > On Mon, 15 Jun 2020 at 09:30, Herbert Xu <[email protected]> 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.
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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