[PATCH 0/3] crypto: robustness fixes to handling of SM3 hash

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH 0/3] crypto: robustness fixes to handling of SM3 hash
@ 2026-10-30 15:04 Daniel P. Berrangé
  2026-10-30 15:04 ` [PATCH 1/3] crypto: add glib initialization for " Daniel P. Berrangé
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Daniel P. Berrangé @ 2026-10-30 15:04 UTC (permalink / raw)
  To: qemu-devel; +Cc: Daniel P. Berrangé



Daniel P. Berrangé (3):
  crypto: add glib initialization for SM3 hash
  crypto: unconditionally report SM3 hash length
  crypto: add preventative measures wrt uninitialized hash info

 crypto/hash-glib.c | 19 +++++++++++++++++--
 crypto/hash.c      | 16 ++++++++++++----
 crypto/hmac-glib.c | 19 +++++++++++++++++--
 3 files changed, 46 insertions(+), 8 deletions(-)

-- 
2.56.0



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/3] crypto: add glib initialization for SM3 hash
  2026-10-30 15:04 [PATCH 0/3] crypto: robustness fixes to handling of SM3 hash Daniel P. Berrangé
@ 2026-10-30 15:04 ` Daniel P. Berrangé
  2026-10-30 16:16   ` Philippe Mathieu-Daudé
  2026-10-30 15:04 ` [PATCH 2/3] crypto: unconditionally report SM3 hash length Daniel P. Berrangé
  2026-10-30 15:04 ` [PATCH 3/3] crypto: add preventative measures wrt uninitialized hash info Daniel P. Berrangé
  2 siblings, 1 reply; 6+ messages in thread
From: Daniel P. Berrangé @ 2026-10-30 15:04 UTC (permalink / raw)
  To: qemu-devel; +Cc: Daniel P. Berrangé

The glib backend for the hmac/hash code must have an explicit
initializer for every element, as the default static initializer
of zero maps to G_CHECKSUM_MD5 and will lead to incorrect
behaviour. This could potentially be a security through use of
the wrong hashing algorithm.

Fixes: https://gitlab.com/qemu-project/qemu/-/work_items/4786
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/hash-glib.c | 1 +
 crypto/hmac-glib.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/crypto/hash-glib.c b/crypto/hash-glib.c
index 809cef98aee..39040f1856e 100644
--- a/crypto/hash-glib.c
+++ b/crypto/hash-glib.c
@@ -33,6 +33,7 @@ static int qcrypto_hash_alg_map[QCRYPTO_HASH_ALGO__MAX] = {
     [QCRYPTO_HASH_ALGO_SHA384] = G_CHECKSUM_SHA384,
     [QCRYPTO_HASH_ALGO_SHA512] = G_CHECKSUM_SHA512,
     [QCRYPTO_HASH_ALGO_RIPEMD160] = -1,
+    [QCRYPTO_HASH_ALGO_SM3] = -1,
 };
 
 gboolean qcrypto_hash_supports(QCryptoHashAlgo alg)
diff --git a/crypto/hmac-glib.c b/crypto/hmac-glib.c
index 1f17769c1c7..8f652c5064e 100644
--- a/crypto/hmac-glib.c
+++ b/crypto/hmac-glib.c
@@ -25,6 +25,7 @@ static int qcrypto_hmac_alg_map[QCRYPTO_HASH_ALGO__MAX] = {
     [QCRYPTO_HASH_ALGO_SHA224] = -1,
     [QCRYPTO_HASH_ALGO_SHA384] = -1,
     [QCRYPTO_HASH_ALGO_RIPEMD160] = -1,
+    [QCRYPTO_HASH_ALGO_SM3] = -1,
 };
 
 typedef struct QCryptoHmacGlib QCryptoHmacGlib;
-- 
2.56.0



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/3] crypto: add glib initialization for SM3 hash
  2026-10-30 15:04 ` [PATCH 1/3] crypto: add glib initialization for " Daniel P. Berrangé
@ 2026-10-30 16:16   ` Philippe Mathieu-Daudé
  2026-10-30 16:23     ` Daniel P. Berrangé
  0 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2026-10-30 16:16 UTC (permalink / raw)
  To: Daniel P. Berrangé, qemu-devel

On 30/10/26 16:04, Daniel P. BerrangÃ© wrote:
> The glib backend for the hmac/hash code must have an explicit
> initializer for every element, as the default static initializer
> of zero maps to G_CHECKSUM_MD5 and will lead to incorrect
> behaviour. This could potentially be a security through use of
> the wrong hashing algorithm.
> 
> Fixes: https://gitlab.com/qemu-project/qemu/-/work_items/4786
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   crypto/hash-glib.c | 1 +
>   crypto/hmac-glib.c | 1 +
>   2 files changed, 2 insertions(+)
> 
> diff --git a/crypto/hash-glib.c b/crypto/hash-glib.c
> index 809cef98aee..39040f1856e 100644
> --- a/crypto/hash-glib.c
> +++ b/crypto/hash-glib.c
> @@ -33,6 +33,7 @@ static int qcrypto_hash_alg_map[QCRYPTO_HASH_ALGO__MAX] = {
>       [QCRYPTO_HASH_ALGO_SHA384] = G_CHECKSUM_SHA384,
>       [QCRYPTO_HASH_ALGO_SHA512] = G_CHECKSUM_SHA512,
>       [QCRYPTO_HASH_ALGO_RIPEMD160] = -1,
> +    [QCRYPTO_HASH_ALGO_SM3] = -1,
>   };

Should we initialize the array with range designator to prevent that?

   [0 ... QCRYPTO_HASH_ALGO__MAX - 1] = -1,



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/3] crypto: add glib initialization for SM3 hash
  2026-10-30 16:16   ` Philippe Mathieu-Daudé
@ 2026-10-30 16:23     ` Daniel P. Berrangé
  0 siblings, 0 replies; 6+ messages in thread
From: Daniel P. Berrangé @ 2026-10-30 16:23 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: qemu-devel

On Fri, Oct 30, 2026 at 05:16:31PM +0100, Philippe Mathieu-Daudé wrote:
> On 30/10/26 16:04, Daniel P. BerrangÃ© wrote:
> > The glib backend for the hmac/hash code must have an explicit
> > initializer for every element, as the default static initializer
> > of zero maps to G_CHECKSUM_MD5 and will lead to incorrect
> > behaviour. This could potentially be a security through use of
> > the wrong hashing algorithm.
> > 
> > Fixes: https://gitlab.com/qemu-project/qemu/-/work_items/4786
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   crypto/hash-glib.c | 1 +
> >   crypto/hmac-glib.c | 1 +
> >   2 files changed, 2 insertions(+)
> > 
> > diff --git a/crypto/hash-glib.c b/crypto/hash-glib.c
> > index 809cef98aee..39040f1856e 100644
> > --- a/crypto/hash-glib.c
> > +++ b/crypto/hash-glib.c
> > @@ -33,6 +33,7 @@ static int qcrypto_hash_alg_map[QCRYPTO_HASH_ALGO__MAX] = {
> >       [QCRYPTO_HASH_ALGO_SHA384] = G_CHECKSUM_SHA384,
> >       [QCRYPTO_HASH_ALGO_SHA512] = G_CHECKSUM_SHA512,
> >       [QCRYPTO_HASH_ALGO_RIPEMD160] = -1,
> > +    [QCRYPTO_HASH_ALGO_SM3] = -1,
> >   };
> 
> Should we initialize the array with range designator to prevent that?
> 
>   [0 ... QCRYPTO_HASH_ALGO__MAX - 1] = -1,

Oh, I didn't know that syntax exists.

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/3] crypto: unconditionally report SM3 hash length
  2026-10-30 15:04 [PATCH 0/3] crypto: robustness fixes to handling of SM3 hash Daniel P. Berrangé
  2026-10-30 15:04 ` [PATCH 1/3] crypto: add glib initialization for " Daniel P. Berrangé
@ 2026-10-30 15:04 ` Daniel P. Berrangé
  2026-10-30 15:04 ` [PATCH 3/3] crypto: add preventative measures wrt uninitialized hash info Daniel P. Berrangé
  2 siblings, 0 replies; 6+ messages in thread
From: Daniel P. Berrangé @ 2026-10-30 15:04 UTC (permalink / raw)
  To: qemu-devel; +Cc: Daniel P. Berrangé

Callers may query the hash digest length before getting to the
code path that performs hashing, which is where the lack of
support for SM3 in the backend will be reported.

Reporting a hash length of 0 is liable to cause divide by zero
errors in callers.

Fixes: https://gitlab.com/qemu-project/qemu/-/work_items/4786
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/hash.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/crypto/hash.c b/crypto/hash.c
index 6ffb88bf541..42a0a8ec56f 100644
--- a/crypto/hash.c
+++ b/crypto/hash.c
@@ -33,9 +33,7 @@ static size_t qcrypto_hash_alg_size[QCRYPTO_HASH_ALGO__MAX] = {
     [QCRYPTO_HASH_ALGO_SHA384]    = QCRYPTO_HASH_DIGEST_LEN_SHA384,
     [QCRYPTO_HASH_ALGO_SHA512]    = QCRYPTO_HASH_DIGEST_LEN_SHA512,
     [QCRYPTO_HASH_ALGO_RIPEMD160] = QCRYPTO_HASH_DIGEST_LEN_RIPEMD160,
-#ifdef CONFIG_CRYPTO_SM3
-    [QCRYPTO_HASH_ALGO_SM3] = QCRYPTO_HASH_DIGEST_LEN_SM3,
-#endif
+    [QCRYPTO_HASH_ALGO_SM3]       = QCRYPTO_HASH_DIGEST_LEN_SM3,
 };
 
 size_t qcrypto_hash_digest_len(QCryptoHashAlgo alg)
-- 
2.56.0



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 3/3] crypto: add preventative measures wrt uninitialized hash info
  2026-10-30 15:04 [PATCH 0/3] crypto: robustness fixes to handling of SM3 hash Daniel P. Berrangé
  2026-10-30 15:04 ` [PATCH 1/3] crypto: add glib initialization for " Daniel P. Berrangé
  2026-10-30 15:04 ` [PATCH 2/3] crypto: unconditionally report SM3 hash length Daniel P. Berrangé
@ 2026-10-30 15:04 ` Daniel P. Berrangé
  2 siblings, 0 replies; 6+ messages in thread
From: Daniel P. Berrangé @ 2026-10-30 15:04 UTC (permalink / raw)
  To: qemu-devel; +Cc: Daniel P. Berrangé

Several static arrays can create incorrect behaviour if a new hash
algorithm is added without them being explicitly initialized. Be
more robust against this problem by using a static assert for the
array size, instead of predeclaring the array size.

When querying the arrays also add further sanity checks for missing
initializers.

Fixes: https://gitlab.com/qemu-project/qemu/-/work_items/4786
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/hash-glib.c | 18 ++++++++++++++++--
 crypto/hash.c      | 12 +++++++++++-
 crypto/hmac-glib.c | 18 ++++++++++++++++--
 3 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/crypto/hash-glib.c b/crypto/hash-glib.c
index 39040f1856e..d83caec8cab 100644
--- a/crypto/hash-glib.c
+++ b/crypto/hash-glib.c
@@ -25,7 +25,7 @@
 #include "hashpriv.h"
 
 
-static int qcrypto_hash_alg_map[QCRYPTO_HASH_ALGO__MAX] = {
+static int qcrypto_hash_alg_map[] = {
     [QCRYPTO_HASH_ALGO_MD5] = G_CHECKSUM_MD5,
     [QCRYPTO_HASH_ALGO_SHA1] = G_CHECKSUM_SHA1,
     [QCRYPTO_HASH_ALGO_SHA224] = -1,
@@ -35,11 +35,25 @@ static int qcrypto_hash_alg_map[QCRYPTO_HASH_ALGO__MAX] = {
     [QCRYPTO_HASH_ALGO_RIPEMD160] = -1,
     [QCRYPTO_HASH_ALGO_SM3] = -1,
 };
+/*
+ * Prefer the assert over declaring the array with
+ * size QCRYPTO_HASH_ALGO__MAX so we get better
+ * detection of uninitialize elements.
+ */
+G_STATIC_ASSERT(G_N_ELEMENTS(qcrypto_hash_alg_map) ==
+                QCRYPTO_HASH_ALGO__MAX);
 
 gboolean qcrypto_hash_supports(QCryptoHashAlgo alg)
 {
+    /*
+     * The default static initializer value of 0
+     * matches G_CHECKSUM_MD5, so protect against
+     * accidentally uninitialized fields
+     */
     if (alg < G_N_ELEMENTS(qcrypto_hash_alg_map) &&
-        qcrypto_hash_alg_map[alg] != -1) {
+        qcrypto_hash_alg_map[alg] != -1 &&
+        (alg == QCRYPTO_HASH_ALGO_MD5 ||
+         qcrypto_hash_alg_map[alg] != G_CHECKSUM_MD5)) {
         return true;
     }
     return false;
diff --git a/crypto/hash.c b/crypto/hash.c
index 42a0a8ec56f..e966fc1a633 100644
--- a/crypto/hash.c
+++ b/crypto/hash.c
@@ -25,7 +25,7 @@
 #include "crypto/hash.h"
 #include "hashpriv.h"
 
-static size_t qcrypto_hash_alg_size[QCRYPTO_HASH_ALGO__MAX] = {
+static size_t qcrypto_hash_alg_size[] = {
     [QCRYPTO_HASH_ALGO_MD5]       = QCRYPTO_HASH_DIGEST_LEN_MD5,
     [QCRYPTO_HASH_ALGO_SHA1]      = QCRYPTO_HASH_DIGEST_LEN_SHA1,
     [QCRYPTO_HASH_ALGO_SHA224]    = QCRYPTO_HASH_DIGEST_LEN_SHA224,
@@ -35,10 +35,20 @@ static size_t qcrypto_hash_alg_size[QCRYPTO_HASH_ALGO__MAX] = {
     [QCRYPTO_HASH_ALGO_RIPEMD160] = QCRYPTO_HASH_DIGEST_LEN_RIPEMD160,
     [QCRYPTO_HASH_ALGO_SM3]       = QCRYPTO_HASH_DIGEST_LEN_SM3,
 };
+/*
+ * Prefer the assert over declaring the array with
+ * size QCRYPTO_HASH_ALGO__MAX so we get better
+ * detection of uninitialized elements. We don't
+ * want to ever return digest lengths of zero, as
+ * that risks a div-by-zero in callers.
+ */
+G_STATIC_ASSERT(G_N_ELEMENTS(qcrypto_hash_alg_size) ==
+                QCRYPTO_HASH_ALGO__MAX);
 
 size_t qcrypto_hash_digest_len(QCryptoHashAlgo alg)
 {
     assert(alg < G_N_ELEMENTS(qcrypto_hash_alg_size));
+    assert(qcrypto_hash_alg_size[alg] != 0);
     return qcrypto_hash_alg_size[alg];
 }
 
diff --git a/crypto/hmac-glib.c b/crypto/hmac-glib.c
index 8f652c5064e..34090444083 100644
--- a/crypto/hmac-glib.c
+++ b/crypto/hmac-glib.c
@@ -17,7 +17,7 @@
 #include "crypto/hmac.h"
 #include "hmacpriv.h"
 
-static int qcrypto_hmac_alg_map[QCRYPTO_HASH_ALGO__MAX] = {
+static int qcrypto_hmac_alg_map[] = {
     [QCRYPTO_HASH_ALGO_MD5] = G_CHECKSUM_MD5,
     [QCRYPTO_HASH_ALGO_SHA1] = G_CHECKSUM_SHA1,
     [QCRYPTO_HASH_ALGO_SHA256] = G_CHECKSUM_SHA256,
@@ -27,6 +27,13 @@ static int qcrypto_hmac_alg_map[QCRYPTO_HASH_ALGO__MAX] = {
     [QCRYPTO_HASH_ALGO_RIPEMD160] = -1,
     [QCRYPTO_HASH_ALGO_SM3] = -1,
 };
+/*
+ * Prefer the assert over declaring the array with
+ * size QCRYPTO_HASH_ALGO__MAX so we get better
+ * detection of uninitialize elements.
+ */
+G_STATIC_ASSERT(G_N_ELEMENTS(qcrypto_hmac_alg_map) ==
+                QCRYPTO_HASH_ALGO__MAX);
 
 typedef struct QCryptoHmacGlib QCryptoHmacGlib;
 struct QCryptoHmacGlib {
@@ -35,8 +42,15 @@ struct QCryptoHmacGlib {
 
 bool qcrypto_hmac_supports(QCryptoHashAlgo alg)
 {
+    /*
+     * The default static initializer value of 0
+     * matches G_CHECKSUM_MD5, so protect against
+     * accidentally uninitialized fields
+     */
     if (alg < G_N_ELEMENTS(qcrypto_hmac_alg_map) &&
-        qcrypto_hmac_alg_map[alg] != -1) {
+        qcrypto_hmac_alg_map[alg] != -1 &&
+        (alg == QCRYPTO_HASH_ALGO_MD5 ||
+         qcrypto_hmac_alg_map[alg] != G_CHECKSUM_MD5)) {
         return true;
     }
 
-- 
2.56.0



^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-10-30 16:23 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-30 15:04 [PATCH 0/3] crypto: robustness fixes to handling of SM3 hash Daniel P. Berrangé
2026-10-30 15:04 ` [PATCH 1/3] crypto: add glib initialization for " Daniel P. Berrangé
2026-10-30 16:16   ` Philippe Mathieu-Daudé
2026-10-30 16:23     ` Daniel P. Berrangé
2026-10-30 15:04 ` [PATCH 2/3] crypto: unconditionally report SM3 hash length Daniel P. Berrangé
2026-10-30 15:04 ` [PATCH 3/3] crypto: add preventative measures wrt uninitialized hash info Daniel P. Berrangé

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.