Re: [RFC PATCH] crypto: skcipher - perform len fits into blocksize check at the top

[Eric Biggers <ebiggers@kernel.org>]

Hi Christian,

On Sat, May 18, 2019 at 11:30:35PM +0200, Christian Lamparter wrote:
> This patch adds early check to test the given data length
> is aligned with the supported ciphers blocksize, or abort
> with -EINVAL in case the supplied chunk size does not fit
> without padding into the blocksize for block ciphers.
> 
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> 
> ---
> 
> This will also work instead of the
> "crypto: crypto4xx - blockciphers should only accept complete blocks"
> It will fix all potential driver issues in other drivers as well as
> break the drivers that don't have the correct blocksize set. it will
> also make the extra checks scattered around in the drivers make
> redundand as well as the extra tests that do send incomplete blocks
> to the hardware drivers.
> ---
>  include/crypto/skcipher.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h
> index e555294ed77f..971294602a41 100644
> --- a/include/crypto/skcipher.h
> +++ b/include/crypto/skcipher.h
> @@ -494,6 +494,8 @@ static inline int crypto_skcipher_encrypt(struct skcipher_request *req)
>  	crypto_stats_get(alg);
>  	if (crypto_skcipher_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
>  		ret = -ENOKEY;
> +	else if (!IS_ALIGNED(cryptlen, crypto_skcipher_blocksize(tfm)))
> +		ret = -EINVAL;
>  	else
>  		ret = tfm->encrypt(req);
>  	crypto_stats_skcipher_encrypt(cryptlen, ret, alg);
> @@ -521,6 +523,8 @@ static inline int crypto_skcipher_decrypt(struct skcipher_request *req)
>  	crypto_stats_get(alg);
>  	if (crypto_skcipher_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
>  		ret = -ENOKEY;
> +	else if (!IS_ALIGNED(cryptlen, crypto_skcipher_blocksize(tfm)))
> +		ret = -EINVAL;
>  	else
>  		ret = tfm->decrypt(req);
>  	crypto_stats_skcipher_decrypt(cryptlen, ret, alg);
> -- 

Unfortunately this doesn't work because there are some skcipher algorithms
("cts" and "adiantum") that set cra_blocksize to their minimum message length
(16) rather than their length alignment requirement (1).  I.e., these algorithms
support all messages with length >= 16 bytes.

So before we can add this check, we need to get this straightened out.

I actually don't think "block size" should be a property of all crypto
algorithms at all, e.g. for skciphers we should instead have "min_msgsize" and
"msgsize_alignment".  The notion of "block size" is not meaningful for
length-preserving encryption algorithms in general, let alone all crypto
algorithms.  But unfortunately "block size" is pretty deeply embedded into the
crypto API, so it doesn't look easy to fix this...  Maybe for skciphers we could
get away with it always meaning the msgsize_alignment, though.

- Eric