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

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

[Daniel P. Berrangé]

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

 crypto/block-luks.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

-- 
2.56.0

[PATCH 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 | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 0926ad28f0c..8a1df846a14 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -1152,22 +1152,29 @@ 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
      * update failed
      */
     for (i = 0; i < QCRYPTO_BLOCK_LUKS_ERASE_ITERATIONS; i++) {
         if (qcrypto_random_bytes(garbagesplitkey,
-                                 splitkeylen, &local_err) < 0) {
+                                 splitkeylen,
+                                 errp && *errp ? NULL : &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 (local_err) {
+                error_propagate(errp, local_err);
+            }
 
             if (i > 0) {
                 return -1;
+            } else {
+                local_err = NULL;
             }
         }
         if (writefunc(block,
@@ -1175,8 +1182,10 @@ qcrypto_block_luks_erase_key(QCryptoBlock *block,
                       garbagesplitkey,
                       splitkeylen,
                       opaque,
-                      &local_err) < 0) {
-            error_propagate(errp, local_err);
+                      errp && *errp ? NULL : &local_err) < 0) {
+            if (local_err) {
+                error_propagate(errp, local_err);
+            }
             return -1;
         }
     }
-- 
2.56.0

[PATCH 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 8a1df846a14..bdfc85d15ca 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -1681,7 +1681,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;
     }
 
@@ -1694,7 +1694,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;
@@ -1792,7 +1792,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;
         }
 
@@ -1845,7 +1845,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 2/2] crypto: add newline in error hint messages

[marcandre.lureau]

On Fri, 23 Oct 2026 15:13:03 +0100, 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 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: <20261023141303.163893-2-berrange@redhat.com>
> ---
>  crypto/block-luks.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index 0926ad28f0cd..8a1df846a146 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -1152,22 +1152,29 @@ 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
>       * update failed
>       */
>      for (i = 0; i < QCRYPTO_BLOCK_LUKS_ERASE_ITERATIONS; i++) {
>          if (qcrypto_random_bytes(garbagesplitkey,
> -                                 splitkeylen, &local_err) < 0) {
> +                                 splitkeylen,
> +                                 errp && *errp ? NULL : &local_err) < 0) {

I think the current code was simpler. error_propagate() will simpliy discard local_err if errp is already set. It is just missing "local_err = NULL;"

>              /*
>               * 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 (local_err) {
> +                error_propagate(errp, local_err);
> +            }
>  
>              if (i > 0) {
>                  return -1;
> +            } else {
> +                local_err = NULL;
>              }
>          }
>          if (writefunc(block,
> @@ -1175,8 +1182,10 @@ qcrypto_block_luks_erase_key(QCryptoBlock *block,
>                        garbagesplitkey,
>                        splitkeylen,
>                        opaque,
> -                      &local_err) < 0) {
> -            error_propagate(errp, local_err);
> +                      errp && *errp ? NULL : &local_err) < 0) {
> +            if (local_err) {
> +                error_propagate(errp, local_err);
> +            }

Same, I dont see why that's necessary

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

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

[Markus Armbruster]

marcandre.lureau@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>
>> Message-ID: <20261023141303.163893-2-berrange@redhat.com>
>> ---
>>  crypto/block-luks.c | 17 +++++++++++++----
>>  1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
>> index 0926ad28f0cd..8a1df846a146 100644
>> --- a/crypto/block-luks.c
>> +++ b/crypto/block-luks.c
>> @@ -1152,22 +1152,29 @@ 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
>>       * update failed
>>       */
>>      for (i = 0; i < QCRYPTO_BLOCK_LUKS_ERASE_ITERATIONS; i++) {
>>          if (qcrypto_random_bytes(garbagesplitkey,
>> -                                 splitkeylen, &local_err) < 0) {
>> +                                 splitkeylen,
>> +                                 errp && *errp ? NULL : &local_err) < 0) {
>
> I think the current code was simpler. error_propagate() will simpliy discard local_err if errp is already set. It is just missing "local_err = NULL;"

Moreover, *errp is wrong without ERRP_GUARD().

[...]