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

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

[Longpeng(Mike)]

From: Longpeng <longpeng2@huawei.com>

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

Because the error message is threw away immediately, so we should
just pass NULL to hash_bytesv(). There is also the same problem in
afalg-backend cipher & hmac, let's fix them together.

Reported-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Longpeng <longpeng2@huawei.com>
---
v2->v1:
 - fix typo [Eric]
 - just pass NULL to hash_bytesv() [Eric, Daniel]
 - remove useless Error object in afalg-backend
   cipher & hmac [Longpeng]

---
 crypto/cipher.c | 5 +----
 crypto/hash.c   | 8 +-------
 crypto/hmac.c   | 4 +---
 3 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/crypto/cipher.c b/crypto/cipher.c
index 0aad9d6..bcbfb3d 100644
--- a/crypto/cipher.c
+++ b/crypto/cipher.c
@@ -164,11 +164,10 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
 {
     QCryptoCipher *cipher;
     void *ctx = NULL;
-    Error *err2 = NULL;
     QCryptoCipherDriver *drv = NULL;
 
 #ifdef CONFIG_AF_ALG
-    ctx = qcrypto_afalg_cipher_ctx_new(alg, mode, key, nkey, &err2);
+    ctx = qcrypto_afalg_cipher_ctx_new(alg, mode, key, nkey, NULL);
     if (ctx) {
         drv = &qcrypto_cipher_afalg_driver;
     }
@@ -177,12 +176,10 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
     if (!ctx) {
         ctx = qcrypto_cipher_ctx_new(alg, mode, key, nkey, errp);
         if (!ctx) {
-            error_free(err2);
             return NULL;
         }
 
         drv = &qcrypto_cipher_lib_driver;
-        error_free(err2);
     }
 
     cipher = g_new0(QCryptoCipher, 1);
diff --git a/crypto/hash.c b/crypto/hash.c
index ac59c63..aeeec97 100644
--- a/crypto/hash.c
+++ b/crypto/hash.c
@@ -51,16 +51,10 @@ int qcrypto_hash_bytesv(QCryptoHashAlgorithm alg,
 
     ret = qcrypto_hash_afalg_driver.hash_bytesv(alg, iov, niov,
                                                 result, resultlen,
-                                                errp);
+                                                NULL);
     if (ret == 0) {
         return ret;
     }
-
-    /*
-     * TODO:
-     * Maybe we should treat some afalg errors as fatal
-     */
-    error_free(*errp);
 #endif
 
     return qcrypto_hash_lib_driver.hash_bytesv(alg, iov, niov,
diff --git a/crypto/hmac.c b/crypto/hmac.c
index 82b0055..f6c2d8d 100644
--- a/crypto/hmac.c
+++ b/crypto/hmac.c
@@ -90,11 +90,10 @@ QCryptoHmac *qcrypto_hmac_new(QCryptoHashAlgorithm alg,
 {
     QCryptoHmac *hmac;
     void *ctx = NULL;
-    Error *err2 = NULL;
     QCryptoHmacDriver *drv = NULL;
 
 #ifdef CONFIG_AF_ALG
-    ctx = qcrypto_afalg_hmac_ctx_new(alg, key, nkey, &err2);
+    ctx = qcrypto_afalg_hmac_ctx_new(alg, key, nkey, NULL);
     if (ctx) {
         drv = &qcrypto_hmac_afalg_driver;
     }
@@ -107,7 +106,6 @@ QCryptoHmac *qcrypto_hmac_new(QCryptoHashAlgorithm alg,
         }
 
         drv = &qcrypto_hmac_lib_driver;
-        error_free(err2);
     }
 
     hmac = g_new0(QCryptoHmac, 1);
-- 
1.8.3.1

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

[Eric Blake]

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

On 11/07/2017 05:32 AM, Longpeng(Mike) wrote:
> From: Longpeng <longpeng2@huawei.com>
> 
> Test-crypto-hash calls qcrypto_hash_bytesv/digest/base64 with
> errp=NULL, this will cause a NULL pointer deference if afalg_driver

s/deference/dereference/

(I called out two typo fixes against v1, but you only fixed one of them).

> 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
> 
> Because the error message is threw away immediately, so we should

s/threw/thrown/
s/so //

> just pass NULL to hash_bytesv(). There is also the same problem in
> afalg-backend cipher & hmac, let's fix them together.
> 
> Reported-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Longpeng <longpeng2@huawei.com>
> ---

> +++ b/crypto/hash.c
> @@ -51,16 +51,10 @@ int qcrypto_hash_bytesv(QCryptoHashAlgorithm alg,
>  
>      ret = qcrypto_hash_afalg_driver.hash_bytesv(alg, iov, niov,
>                                                  result, resultlen,
> -                                                errp);
> +                                                NULL);
>      if (ret == 0) {
>          return ret;
>      }
> -
> -    /*
> -     * TODO:
> -     * Maybe we should treat some afalg errors as fatal
> -     */

The comment is probably still worth keeping (maybe hoisted above the
call to hash_bytesv(, NULL), though).

As moving (instead of deleting) a comment is trivial, I'm okay if a
maintainer does that while applying:

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

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