[PATCH v2 0/2] crypto: fix handling & reporting of errors when erasing keyslots

[PATCH v2 0/2] crypto: fix handling & reporting of errors when erasing keyslots

[Daniel P. Berrangé]

Changed in v2:

 - Simplify first patch to rely error_propagate auto
   discarding dupe errors.

Daniel P. Berrangé (2):
  crypto: avoid overwriting Error object erasing keyslot
  crypto: add newline in error hint messages

 crypto/block-luks.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

-- 
2.56.0

[PATCH v2 1/2] crypto: avoid overwriting Error object erasing keyslot

[Daniel P. Berrangé]

When erasing a keyslot we deliberately ignore some errors and
carry on trying to erase as much as possible. This results in
overwriting Error objects and triggering an assertion. Don't
pass in the &local_err, if errp is already set.

Fixes: https://gitlab.com/qemu-project/qemu/-/work_items/4697
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/block-luks.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 0926ad28f0c..277a5110607 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -1152,6 +1152,7 @@ qcrypto_block_luks_erase_key(QCryptoBlock *block,
 
     if (ret < 0) {
         error_propagate(errp, local_err);
+        local_err = NULL;
     }
     /*
      * Now try to erase the key material, even if the header
@@ -1169,6 +1170,7 @@ qcrypto_block_luks_erase_key(QCryptoBlock *block,
             if (i > 0) {
                 return -1;
             }
+            local_err = NULL;
         }
         if (writefunc(block,
                       slot->key_offset_sector * QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
-- 
2.56.0

[PATCH v2 2/2] crypto: add newline in error hint messages

[Daniel P. Berrangé]

The error_append_hint() API contract requires messages to end with
a newline character.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/block-luks.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 277a5110607..f7217ad7b77 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -1674,7 +1674,7 @@ qcrypto_block_luks_amend_add_keyslot(QCryptoBlock *block,
 
     if (qcrypto_block_luks_find_key(block, old_password, master_key,
                                     readfunc, opaque, errp) < 0) {
-        error_append_hint(errp, "Failed to retrieve the master key");
+        error_append_hint(errp, "Failed to retrieve the master key\n");
         return -1;
     }
 
@@ -1687,7 +1687,7 @@ qcrypto_block_luks_amend_add_keyslot(QCryptoBlock *block,
     /* Now set the new keyslots */
     if (qcrypto_block_luks_store_key(block, keyslot, new_password, master_key,
                                      iter_time, writefunc, opaque, errp)) {
-        error_append_hint(errp, "Failed to write to keyslot %i", keyslot);
+        error_append_hint(errp, "Failed to write to keyslot %i\n", keyslot);
         return -1;
     }
     return 0;
@@ -1785,7 +1785,7 @@ qcrypto_block_luks_amend_erase_keyslots(QCryptoBlock *block,
 
         if (qcrypto_block_luks_erase_key(block, keyslot,
                                          writefunc, opaque, errp)) {
-            error_append_hint(errp, "Failed to erase keyslot %i", keyslot);
+            error_append_hint(errp, "Failed to erase keyslot %i\n", keyslot);
             return -1;
         }
 
@@ -1838,7 +1838,7 @@ qcrypto_block_luks_amend_erase_keyslots(QCryptoBlock *block,
             }
             if (qcrypto_block_luks_erase_key(block, i, writefunc,
                 opaque, errp)) {
-                error_append_hint(errp, "Failed to erase keyslot %zu", i);
+                error_append_hint(errp, "Failed to erase keyslot %zu\n", i);
                 return -1;
             }
         }
-- 
2.56.0

Re: [PATCH v2 1/2] crypto: avoid overwriting Error object erasing keyslot

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> When erasing a keyslot we deliberately ignore some errors and
> carry on trying to erase as much as possible. This results in
> overwriting Error objects and triggering an assertion. Don't
> pass in the &local_err, if errp is already set.
>
> Fixes: https://gitlab.com/qemu-project/qemu/-/work_items/4697
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  crypto/block-luks.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index 0926ad28f0c..277a5110607 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -1152,6 +1152,7 @@ qcrypto_block_luks_erase_key(QCryptoBlock *block,
   {
       QCryptoBlockLUKS *luks = block->opaque;
       QCryptoBlockLUKSKeySlot *slot;
       g_autofree uint8_t *garbagesplitkey = NULL;
       size_t splitkeylen;
       size_t i;
       Error *local_err = NULL;
       int ret;

       assert(slot_idx < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
       slot = &luks->header.key_slots[slot_idx];

       splitkeylen = luks->header.master_key_len * slot->stripes;
       assert(splitkeylen > 0);

       garbagesplitkey = g_new0(uint8_t, splitkeylen);

       /* Reset the key slot header */
       memset(slot->salt, 0, QCRYPTO_BLOCK_LUKS_SALT_LEN);
       slot->iterations = 0;
       slot->active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;

       ret = qcrypto_block_luks_store_header(block, writefunc,
                                             opaque, &local_err);

Why not pass @errp here, and ...

>  
>      if (ret < 0) {
>          error_propagate(errp, local_err);

... drop this line?

> +        local_err = NULL;
>      }

Suggest a blank line here.


Note for later: @errp may be non-null now.

>      /*
>       * Now try to erase the key material, even if the header
> @@ -1169,6 +1170,7 @@ qcrypto_block_luks_erase_key(QCryptoBlock *block,
        * update failed
        */
       for (i = 0; i < QCRYPTO_BLOCK_LUKS_ERASE_ITERATIONS; i++) {
           if (qcrypto_random_bytes(garbagesplitkey,
                                    splitkeylen, &local_err) < 0) {
               /*
                * If we failed to get the random data, still write
                * at least zeros to the key slot at least once
                */
               error_propagate(errp, local_err);

If @errp is still null, store this error, else ignore it.

>              if (i > 0) {
>                  return -1;

Stop when erasing non-first key slot failed.

>              }
> +            local_err = NULL;
>          }
>          if (writefunc(block,
>                        slot->key_offset_sector * QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
                         garbagesplitkey,
                         splitkeylen,
                         opaque,
                         &local_err) < 0) {
               error_propagate(errp, local_err);

We do need error_propagate() here, because @errp can be non-null.

               return -1;
           }
       }
       return ret;
   }

Nothing is wrong, so
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v2 2/2] crypto: add newline in error hint messages

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> The error_append_hint() API contract requires messages to end with
> a newline character.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v2 2/2] crypto: add newline in error hint messages

[marcandre.lureau]

On Mon, 26 Oct 2026 11:12:37 +0000, Daniel P. Berrangé <berrange@redhat.com> wrote:
> The error_append_hint() API contract requires messages to end with
> a newline character.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [PATCH v2 1/2] crypto: avoid overwriting Error object erasing keyslot

[marcandre.lureau]

> When erasing a keyslot we deliberately ignore some errors and
> carry on trying to erase as much as possible. This results in
> overwriting Error objects and triggering an assertion. Don't
> pass in the &local_err, if errp is already set.
> 
> Fixes: https://gitlab.com/qemu-project/qemu/-/work_items/4697
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> Message-ID: <20261026111237.660633-2-berrange@redhat.com>
> ---
>  crypto/block-luks.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index 0926ad28f0cd..277a51106072 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -1152,6 +1152,7 @@ qcrypto_block_luks_erase_key(QCryptoBlock *block,
>  
>      if (ret < 0) {
>          error_propagate(errp, local_err);
> +        local_err = NULL;
>      }
>      /*
>       * Now try to erase the key material, even if the header
> @@ -1169,6 +1170,7 @@ qcrypto_block_luks_erase_key(QCryptoBlock *block,
>              if (i > 0) {
>                  return -1;
>              }
> +            local_err = NULL;

This is the recoverable path: the comment says we still write zeros and
carry on, so a first-iteration random failure is not meant to be fatal. But
error_propagate(errp, local_err) has just stored that error in *errp.

Keep the first-iteration failure as non-fatal but do not hand it to errp?
(e.g. error_free(local_err) instead of error_propagate() when i == 0)

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>