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

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

[Longpeng(Mike)]

Test-crypto-hash calls qcrypto_hash_bytesv/digest/base64 with
errp=NULL, this will cause a NULL poniter deference if afalg_driver
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.

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);
+    }
 #endif
 
     return qcrypto_hash_lib_driver.hash_bytesv(alg, iov, niov,
-- 
1.8.3.1

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

[Gonglei (Arei)]

> -----Original Message-----
> From: longpeng
> Sent: Monday, November 06, 2017 2:21 PM
> To: berrange@redhat.com; pbonzini@redhat.com; Gonglei (Arei)
> Cc: longpeng; qemu-devel@nongnu.org
> Subject: [PATCH] crypto: afalg: fix a NULL pointer dereference
> 
> Test-crypto-hash calls qcrypto_hash_bytesv/digest/base64 with
> errp=NULL, this will cause a NULL poniter deference if afalg_driver
> 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.
> 
> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> ---

Reported-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Gonglei <arei.gonglei@huawei.com>

Thanks,
-Gonglei

>  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);
> +    }
>  #endif
> 
>      return qcrypto_hash_lib_driver.hash_bytesv(alg, iov, niov,
> --
> 1.8.3.1
> 

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

[Eric Blake]

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

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/

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

> 
> 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
a v2 that doesn't try to set errp in the first place, rather than
cleaning it up to ignore it after the fact.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

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

[Longpeng (Mike)]

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)

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

[Daniel P. Berrange]

On Tue, Nov 07, 2017 at 10:27:10AM +0800, Longpeng (Mike) wrote:
> 
> 
> 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().

It doesn't do anything useful for debug, because we are just immediately
throwing away the error without printing it anywhere. Just pass NULL into
the hash_bytesv call above.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Longpeng (Mike)]

On 2017/11/7 17:16, Daniel P. Berrange wrote:

> On Tue, Nov 07, 2017 at 10:27:10AM +0800, Longpeng (Mike) wrote:
>>
>>
>> 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().
> 
> It doesn't do anything useful for debug, because we are just immediately
> throwing away the error without printing it anywhere. Just pass NULL into
> the hash_bytesv call above.
> 

OK..

Afalg-backend cipher/hmac has the same usage, so maybe I need to correct all of
them.

> Regards,
> Daniel


-- 
Regards,
Longpeng(Mike)

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

[Stefan Hajnoczi]

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

On Mon, Nov 06, 2017 at 02:21:11PM +0800, 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
> 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.

Only errp needs to be checked.  It's okay to invoke error_free(NULL):

  void error_free(Error *err)
  {
      if (err) {

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Longpeng (Mike)]

On 2017/11/7 1:18, Stefan Hajnoczi wrote:

> On Mon, Nov 06, 2017 at 02:21:11PM +0800, 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
>> 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.
> 
> Only errp needs to be checked.  It's okay to invoke error_free(NULL):
> 
>   void error_free(Error *err)
>   {
>       if (err) {


Ah yes, thanks for your note :)

I'll pick another approach to fix this bug.

-- 
Regards,
Longpeng(Mike)