[Qemu-devel] [PATCH] crypto: assert cipher algorithm is always valid

[Qemu-devel] [PATCH] crypto: assert cipher algorithm is always valid

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

Crypto routines 'qcrypto_cipher_get_block_len' and
'qcrypto_cipher_get_key_len' return non-zero cipher block and key
lengths from static arrays 'alg_block_len[]' and 'alg_key_len[]'
respectively. Returning 'zero(0)' value from either of them would
likely lead to an error condition.

Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 crypto/cipher.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/crypto/cipher.c b/crypto/cipher.c
index 9ecaff7..5a96489 100644
--- a/crypto/cipher.c
+++ b/crypto/cipher.c
@@ -63,18 +63,14 @@ static bool mode_need_iv[QCRYPTO_CIPHER_MODE__MAX] = {
 
 size_t qcrypto_cipher_get_block_len(QCryptoCipherAlgorithm alg)
 {
-    if (alg >= G_N_ELEMENTS(alg_key_len)) {
-        return 0;
-    }
+    assert(alg < G_N_ELEMENTS(alg_key_len));
     return alg_block_len[alg];
 }
 
 
 size_t qcrypto_cipher_get_key_len(QCryptoCipherAlgorithm alg)
 {
-    if (alg >= G_N_ELEMENTS(alg_key_len)) {
-        return 0;
-    }
+    assert(alg < G_N_ELEMENTS(alg_key_len));
     return alg_key_len[alg];
 }
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH] crypto: assert cipher algorithm is always valid

[Daniel P. Berrange]

On Mon, Feb 20, 2017 at 04:53:07PM +0530, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> Crypto routines 'qcrypto_cipher_get_block_len' and
> 'qcrypto_cipher_get_key_len' return non-zero cipher block and key
> lengths from static arrays 'alg_block_len[]' and 'alg_key_len[]'
> respectively. Returning 'zero(0)' value from either of them would
> likely lead to an error condition.

Well callers are supposed to check for 0 condition and report an
error really. In practice none of them do, and the alg parameters
they pass in all come from constants. So we're not validating user
input here - we're catching programming bugs and thus assert makes
sense.

> 
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  crypto/cipher.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>

> 
> diff --git a/crypto/cipher.c b/crypto/cipher.c
> index 9ecaff7..5a96489 100644
> --- a/crypto/cipher.c
> +++ b/crypto/cipher.c
> @@ -63,18 +63,14 @@ static bool mode_need_iv[QCRYPTO_CIPHER_MODE__MAX] = {
>  
>  size_t qcrypto_cipher_get_block_len(QCryptoCipherAlgorithm alg)
>  {
> -    if (alg >= G_N_ELEMENTS(alg_key_len)) {
> -        return 0;
> -    }
> +    assert(alg < G_N_ELEMENTS(alg_key_len));
>      return alg_block_len[alg];
>  }
>  
>  
>  size_t qcrypto_cipher_get_key_len(QCryptoCipherAlgorithm alg)
>  {
> -    if (alg >= G_N_ELEMENTS(alg_key_len)) {
> -        return 0;
> -    }
> +    assert(alg < G_N_ELEMENTS(alg_key_len));
>      return alg_key_len[alg];
>  }

Adding to the crypto/ queue

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|