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

[Markus Armbruster <armbru@redhat.com>]

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>