[Qemu-devel] [PATCH] crypto: assert that qcrypto_hash_digest_len is in range

[Qemu-devel] [PATCH] crypto: assert that qcrypto_hash_digest_len is in range

[Paolo Bonzini]

Otherwise unintended results could happen.  For example,
Coverity reports a division by zero in qcrypto_afsplit_hash.
While this cannot really happen, it shows that the contract
of qcrypto_hash_digest_len can be improved.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 crypto/hash.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/crypto/hash.c b/crypto/hash.c
index b90af34..2907bff 100644
--- a/crypto/hash.c
+++ b/crypto/hash.c
@@ -36,9 +36,7 @@ static size_t qcrypto_hash_alg_size[QCRYPTO_HASH_ALG__MAX] = {
 
 size_t qcrypto_hash_digest_len(QCryptoHashAlgorithm alg)
 {
-    if (alg >= G_N_ELEMENTS(qcrypto_hash_alg_size)) {
-        return 0;
-    }
+    assert(alg < G_N_ELEMENTS(qcrypto_hash_alg_size));
     return qcrypto_hash_alg_size[alg];
 }
 
-- 
2.5.5

Re: [Qemu-devel] [PATCH] crypto: assert that qcrypto_hash_digest_len is in range

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]

On 05/20/2016 03:09 AM, Paolo Bonzini wrote:
> Otherwise unintended results could happen.  For example,
> Coverity reports a division by zero in qcrypto_afsplit_hash.
> While this cannot really happen, it shows that the contract
> of qcrypto_hash_digest_len can be improved.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  crypto/hash.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/crypto/hash.c b/crypto/hash.c
> index b90af34..2907bff 100644
> --- a/crypto/hash.c
> +++ b/crypto/hash.c
> @@ -36,9 +36,7 @@ static size_t qcrypto_hash_alg_size[QCRYPTO_HASH_ALG__MAX] = {
>  
>  size_t qcrypto_hash_digest_len(QCryptoHashAlgorithm alg)
>  {
> -    if (alg >= G_N_ELEMENTS(qcrypto_hash_alg_size)) {
> -        return 0;
> -    }
> +    assert(alg < G_N_ELEMENTS(qcrypto_hash_alg_size));
>      return qcrypto_hash_alg_size[alg];

The assertion doesn't protect us if QCryptoHashAlgorithm gains another
member but we forget to update qcrypto_hash_alg_size[] to match.  Do you
want an additional assertion that you are returning a non-zero value?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] crypto: assert that qcrypto_hash_digest_len is in range

[Daniel P. Berrange]

On Fri, May 20, 2016 at 09:52:36AM -0600, Eric Blake wrote:
> On 05/20/2016 03:09 AM, Paolo Bonzini wrote:
> > Otherwise unintended results could happen.  For example,
> > Coverity reports a division by zero in qcrypto_afsplit_hash.
> > While this cannot really happen, it shows that the contract
> > of qcrypto_hash_digest_len can be improved.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  crypto/hash.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks, will queue this in my crypto-next branch

> > diff --git a/crypto/hash.c b/crypto/hash.c
> > index b90af34..2907bff 100644
> > --- a/crypto/hash.c
> > +++ b/crypto/hash.c
> > @@ -36,9 +36,7 @@ static size_t qcrypto_hash_alg_size[QCRYPTO_HASH_ALG__MAX] = {
> >  
> >  size_t qcrypto_hash_digest_len(QCryptoHashAlgorithm alg)
> >  {
> > -    if (alg >= G_N_ELEMENTS(qcrypto_hash_alg_size)) {
> > -        return 0;
> > -    }
> > +    assert(alg < G_N_ELEMENTS(qcrypto_hash_alg_size));
> >      return qcrypto_hash_alg_size[alg];
> 
> The assertion doesn't protect us if QCryptoHashAlgorithm gains another
> member but we forget to update qcrypto_hash_alg_size[] to match.  Do you
> want an additional assertion that you are returning a non-zero value?

It will gain more entries with a patch I have pending, but at the same
time the impl of the hash methods will change, so this code won't exist
in its current form. So I'll just queue this patch as is.

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