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 <address@hidden>
> > ---
> >  crypto/hash.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> Reviewed-by: Eric Blake <address@hidden>

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 :|