| Message ID | E1k0Jsl-0006Ho-Gf@fornost.hmeau.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: skcipher - Add support for no chaining and partial chaining | expand |
On Tue, Jul 28, 2020 at 05:18:39PM +1000, Herbert Xu wrote: > 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 final_chunksize 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. Shouldn't chaining be disabled by default? This is inviting bugs where drivers don't implement chaining, but leave final_chunksize unset (0) which apparently indicates that chaining is supported. - Eric
On Tue, Jul 28, 2020 at 10:15:12AM -0700, Eric Biggers wrote: > > Shouldn't chaining be disabled by default? This is inviting bugs where drivers > don't implement chaining, but leave final_chunksize unset (0) which apparently > indicates that chaining is supported. I've gone through everything and the majority of algorithms do support chaining so I think defaulting to on makes more sense. For now we have some algorithms that can be chained but the drivers do not allow it, this is not something that I'd like to see in new drivers. Cheers,
On Tue, 28 Jul 2020 at 20:22, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Tue, Jul 28, 2020 at 10:15:12AM -0700, Eric Biggers wrote: > > > > Shouldn't chaining be disabled by default? This is inviting bugs where drivers > > don't implement chaining, but leave final_chunksize unset (0) which apparently > > indicates that chaining is supported. > > I've gone through everything and the majority of algorithms do > support chaining so I think defaulting to on makes more sense. > > For now we have some algorithms that can be chained but the drivers > do not allow it, this is not something that I'd like to see in > new drivers. > So how does one allocate a tfm that supports chaining if their use case requires it? Having different implementations of the same algo where one does support it while the other one doesn't means we will need some flag to request this at alloc time. > 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
On Tue, Jul 28, 2020 at 08:26:38PM +0300, Ard Biesheuvel wrote: > > So how does one allocate a tfm that supports chaining if their use > case requires it? Having different implementations of the same algo > where one does support it while the other one doesn't means we will > need some flag to request this at alloc time. Yes we could add a flag for it. However, for the two users that I'm looking at right now (algif_skcipher and sunrpc) this is not required. For algif_skcipher it'll simply fall back to the current behaviour if chaining is not supported, while sunrpc would only use chaining with cts where it is always supported. Cheers,
On Tue, 28 Jul 2020 at 20:30, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Tue, Jul 28, 2020 at 08:26:38PM +0300, Ard Biesheuvel wrote: > > > > So how does one allocate a tfm that supports chaining if their use > > case requires it? Having different implementations of the same algo > > where one does support it while the other one doesn't means we will > > need some flag to request this at alloc time. > > Yes we could add a flag for it. However, for the two users that > I'm looking at right now (algif_skcipher and sunrpc) this is not > required. For algif_skcipher it'll simply fall back to the current > behaviour if chaining is not supported, while sunrpc would only > use chaining with cts where it is always supported. > Ok, now I'm confused again: if falling back to the current behavior is acceptable for algif_skcipher, why do we need all these changes? > 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
On Tue, Jul 28, 2020 at 08:46:42PM +0300, Ard Biesheuvel wrote: > > > Yes we could add a flag for it. However, for the two users that > > I'm looking at right now (algif_skcipher and sunrpc) this is not > > required. For algif_skcipher it'll simply fall back to the current > > behaviour if chaining is not supported, while sunrpc would only > > use chaining with cts where it is always supported. > > Ok, now I'm confused again: if falling back to the current behavior is > acceptable for algif_skcipher, why do we need all these changes? The current behaviour isn't quite the right phrase. What happens now is that algif_skcipher will try to chain everything which would obviously fail with such a driver. With the patch-set it won't try to chain and will instead return -EINVAL. Cheers,
diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h index 5663f71198b37..fb90c3e1c26ba 100644 --- a/include/crypto/skcipher.h +++ b/include/crypto/skcipher.h @@ -97,6 +97,9 @@ 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. + * @final_chunksize: 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 +117,7 @@ struct skcipher_alg { unsigned int ivsize; unsigned int chunksize; unsigned int walksize; + int final_chunksize; struct crypto_alg base; }; @@ -279,6 +283,11 @@ static inline unsigned int crypto_skcipher_alg_chunksize( return alg->chunksize; } +static inline int crypto_skcipher_alg_final_chunksize(struct skcipher_alg *alg) +{ + return alg->final_chunksize; +} + /** * crypto_skcipher_chunksize() - obtain chunk size * @tfm: cipher handle @@ -296,6 +305,21 @@ static inline unsigned int crypto_skcipher_chunksize( return crypto_skcipher_alg_chunksize(crypto_skcipher_alg(tfm)); } +/** + * crypto_skcipher_final_chunksize() - 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_final_chunksize(struct crypto_skcipher *tfm) +{ + return crypto_skcipher_alg_final_chunksize(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 ef90e07c9635c..2e624a1d4f832 100644 --- a/include/linux/crypto.h +++ b/include/linux/crypto.h @@ -141,6 +141,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.
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 final_chunksize 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(+)