diff mbox

[3/6] crypto: api - avoid VLA use

Message ID 1523126303-23205-4-git-send-email-s.mesoraca16@gmail.com (mailing list archive)
State Superseded
Headers show

Commit Message

Salvatore Mesoraca April 7, 2018, 6:38 p.m. UTC
We avoid a VLA[1] by always allocating MAX_BLOCKSIZE +
MAX_ALIGNMASK bytes.
We also check the selected cipher at initialization time, if
it doesn't comply with these limits, the initialization will
fail.

[1] http://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com

Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
---
 crypto/cipher.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Herbert Xu April 8, 2018, 3:16 a.m. UTC | #1
On Sat, Apr 07, 2018 at 08:38:20PM +0200, Salvatore Mesoraca wrote:
>
>  int crypto_init_cipher_ops(struct crypto_tfm *tfm)
>  {
> +	const unsigned long alignmask = crypto_tfm_alg_alignmask(tfm);
> +	const unsigned int size = crypto_tfm_alg_blocksize(tfm);
>  	struct cipher_tfm *ops = &tfm->crt_cipher;
>  	struct cipher_alg *cipher = &tfm->__crt_alg->cra_cipher;
>  
> +	if (size > MAX_BLOCKSIZE || alignmask > MAX_ALIGNMASK)
> +		return -EINVAL;
> +

This check should be done when the algorithm is registered.  Perhaps
crypto_check_alg.

Thanks,
Salvatore Mesoraca April 8, 2018, 9:07 a.m. UTC | #2
2018-04-08 5:16 GMT+02:00 Herbert Xu <herbert@gondor.apana.org.au>:
> On Sat, Apr 07, 2018 at 08:38:20PM +0200, Salvatore Mesoraca wrote:
>>
>>  int crypto_init_cipher_ops(struct crypto_tfm *tfm)
>>  {
>> +     const unsigned long alignmask = crypto_tfm_alg_alignmask(tfm);
>> +     const unsigned int size = crypto_tfm_alg_blocksize(tfm);
>>       struct cipher_tfm *ops = &tfm->crt_cipher;
>>       struct cipher_alg *cipher = &tfm->__crt_alg->cra_cipher;
>>
>> +     if (size > MAX_BLOCKSIZE || alignmask > MAX_ALIGNMASK)
>> +             return -EINVAL;
>> +
>
> This check should be done when the algorithm is registered.  Perhaps
> crypto_check_alg.

Please correct me if I'm wrong:
isn't crypto_check_alg invoked also during hashing algorithm registration?
In this patch-set I'm dealing only with ciphers, because the maximum
block size (16)
is relatively small and it's also the most common block size with
ciphers (maybe I should
have explicitly referenced ciphers in the macro names, my bad).
I don't think that it would be OK to use a similar approach for hashes
too, because some
of them have block size >= 1024 bytes.

Thank you for your time,

Salvatore
Herbert Xu April 8, 2018, 9:22 a.m. UTC | #3
On Sun, Apr 08, 2018 at 11:07:12AM +0200, Salvatore Mesoraca wrote:
>
> > This check should be done when the algorithm is registered.  Perhaps
> > crypto_check_alg.
> 
> Please correct me if I'm wrong:
> isn't crypto_check_alg invoked also during hashing algorithm registration?
> In this patch-set I'm dealing only with ciphers, because the maximum
> block size (16)
> is relatively small and it's also the most common block size with
> ciphers (maybe I should
> have explicitly referenced ciphers in the macro names, my bad).
> I don't think that it would be OK to use a similar approach for hashes
> too, because some
> of them have block size >= 1024 bytes.

Yes we want to make it for ciphers only even if we move it to
crypto_check_alg.

For a legacy type like cipher cou can do it by

	if (!alg->cra_type && (alg->cra_flags & CRYPTO_ALG_TYPE_MASK) ==
			      CRYPTO_ALG_TYPE_CIPHER)
		do_cipher_specific_check();

Cheers,
Salvatore Mesoraca April 9, 2018, 1:27 p.m. UTC | #4
2018-04-08 11:22 GMT+02:00 Herbert Xu <herbert@gondor.apana.org.au>:
> On Sun, Apr 08, 2018 at 11:07:12AM +0200, Salvatore Mesoraca wrote:
>>
>> > This check should be done when the algorithm is registered.  Perhaps
>> > crypto_check_alg.
>>
>> Please correct me if I'm wrong:
>> isn't crypto_check_alg invoked also during hashing algorithm registration?
>> In this patch-set I'm dealing only with ciphers, because the maximum
>> block size (16)
>> is relatively small and it's also the most common block size with
>> ciphers (maybe I should
>> have explicitly referenced ciphers in the macro names, my bad).
>> I don't think that it would be OK to use a similar approach for hashes
>> too, because some
>> of them have block size >= 1024 bytes.
>
> Yes we want to make it for ciphers only even if we move it to
> crypto_check_alg.
>
> For a legacy type like cipher cou can do it by
>
>         if (!alg->cra_type && (alg->cra_flags & CRYPTO_ALG_TYPE_MASK) ==
>                               CRYPTO_ALG_TYPE_CIPHER)
>                 do_cipher_specific_check();
>

Thank you very much for your help.
I'm sending the new version.

Salvatore
diff mbox

Patch

diff --git a/crypto/cipher.c b/crypto/cipher.c
index 94fa355..9cedf23 100644
--- a/crypto/cipher.c
+++ b/crypto/cipher.c
@@ -67,7 +67,7 @@  static void cipher_crypt_unaligned(void (*fn)(struct crypto_tfm *, u8 *,
 {
 	unsigned long alignmask = crypto_tfm_alg_alignmask(tfm);
 	unsigned int size = crypto_tfm_alg_blocksize(tfm);
-	u8 buffer[size + alignmask];
+	u8 buffer[MAX_BLOCKSIZE + MAX_ALIGNMASK];
 	u8 *tmp = (u8 *)ALIGN((unsigned long)buffer, alignmask + 1);
 
 	memcpy(tmp, src, size);
@@ -105,9 +105,14 @@  static void cipher_decrypt_unaligned(struct crypto_tfm *tfm,
 
 int crypto_init_cipher_ops(struct crypto_tfm *tfm)
 {
+	const unsigned long alignmask = crypto_tfm_alg_alignmask(tfm);
+	const unsigned int size = crypto_tfm_alg_blocksize(tfm);
 	struct cipher_tfm *ops = &tfm->crt_cipher;
 	struct cipher_alg *cipher = &tfm->__crt_alg->cra_cipher;
 
+	if (size > MAX_BLOCKSIZE || alignmask > MAX_ALIGNMASK)
+		return -EINVAL;
+
 	ops->cit_setkey = setkey;
 	ops->cit_encrypt_one = crypto_tfm_alg_alignmask(tfm) ?
 		cipher_encrypt_unaligned : cipher->cia_encrypt;