Re: [Qemu-devel] [PATCH] crypto: afalg: fix a NULL pointer dereference

["Longpeng (Mike)" <longpeng2@huawei.com>]

On 2017/11/7 1:00, Eric Blake wrote:

> On 11/06/2017 12:21 AM, Longpeng(Mike) wrote:
>> Test-crypto-hash calls qcrypto_hash_bytesv/digest/base64 with
>> errp=NULL, this will cause a NULL poniter deference if afalg_driver
> 
> s/poniter deference/pointer dereference/
> 

OK.

>> doesn't support requested algos:
>>     ret = qcrypto_hash_afalg_driver.hash_bytesv(alg, iov, niov,
>>                                                 result, resultlen,
>>                                                 errp);
>>     if (ret == 0) {
>>         return ret;
>>     }
>>
>>     error_free(*errp);  // <--- here
>>
>> So we must check 'errp & *errp' before dereference.
> 
> No, if we are going to blindly ignore the error from the hash_bytesv()
> call, then we should pass NULL rather than errp.
> 

The 'errp' in this palce is convenient for debug, it can tell us the reason for
failure without stepping into afalg_driver's hash_bytesv().

>>
>> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
>> ---
>>  crypto/hash.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/crypto/hash.c b/crypto/hash.c
>> index ac59c63..c464c78 100644
>> --- a/crypto/hash.c
>> +++ b/crypto/hash.c
>> @@ -60,7 +60,9 @@ int qcrypto_hash_bytesv(QCryptoHashAlgorithm alg,
>>       * TODO:
>>       * Maybe we should treat some afalg errors as fatal
>>       */
>> -    error_free(*errp);
>> +    if (errp && *errp) {
>> +        error_free(*errp);
>> +    }
> 
> Any code that uses 'if (errp && *errp)' is suspicious; I think you need

I see, thanks a lot.

> a v2 that doesn't try to set errp in the first place, rather than
> cleaning it up to ignore it after the fact.
> 

I answered above.

How about the following approach?

int qcrypto_hash_bytesv(...Error **errp)
{
#ifdef CONFIG_AF_ALG
    int ret;
    Error *err2 = NULL;

    ret = qcrypto_hash_afalg_driver.hash_bytesv(alg, iov, niov,
                                                result, resultlen,
                                                &err2);
    if (ret == 0) {
        return ret;
    }

    error_free(*err2);
#endif

    return qcrypto_hash_lib_driver.hash_bytesv(alg, iov, niov,
                                               result, resultlen,
                                               errp);
}

I'll send a V2 if there's no objection, otherwise I'll pick your suggested
approach. :)

-- 
Regards,
Longpeng(Mike)