Re: [PATCH] luks2: Improve error reporting when decrypting/verifying key

[Patrick Steinhardt]

On Thu, Apr 16, 2020 at 02:27:02PM +0200, Daniel Kiper wrote:
> On Thu, Apr 16, 2020 at 12:19:55PM +0200, Patrick Steinhardt wrote:
> > While we already set up error messages in both `luks2_verify_key()` and
> > `luks2_decrypt_key()`, we do not ever print them. This makes it really
> > hard to discover why a given key actually failed to decrypt a disk.
> >
> > Improve this by including the error message in the user-visible output.
> >
> > Signed-off-by: Patrick Steinhardt <address@hidden>
> > ---
> >  grub-core/disk/luks2.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > index 65c4f0aac..a48bddf5d 100644
> > --- a/grub-core/disk/luks2.c
> > +++ b/grub-core/disk/luks2.c
> > @@ -487,7 +487,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> >    ret = grub_disk_read (disk, 0, k->area.offset, k->area.size, split_key);
> >    if (ret)
> >      {
> > -      grub_dprintf ("luks2", "Read error: %s\n", grub_errmsg);
> > +      grub_error (GRUB_ERR_IO, "Read error: %s\n", grub_errmsg);
> >        goto err;
> >      }
> 
> AIUI the commit message says about this change but...
> 
> > @@ -610,14 +610,16 @@ luks2_recover_key (grub_disk_t disk,
> >                            (const grub_uint8_t *) passphrase, grub_strlen 
> > (passphrase));
> >        if (ret)
> >     {
> > -     grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" 
> > failed\n", i);
> > +     grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" 
> > failed: %s\n",
> > +                   i, grub_errmsg);
> >       continue;
> >     }
> >
> >        ret = luks2_verify_key (&digest, candidate_key, keyslot.key_size);
> >        if (ret)
> >     {
> > -     grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE"\n", 
> > i);
> > +     grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE": 
> > %s\n",
> > +                   i, grub_errmsg);
> >       continue;
> 
> ...it does not say anything about these changes. If you update commit
> message you can add Reviewed-by: Daniel Kiper <address@hidden>
> 
> Daniel

Does the following commit message clear things up?

    luks2: Improve error reporting when recovering keys

    While we already set up error messages in both `luks2_verify_key()` and
    `luks2_decrypt_key()`, we do not ever print them in the calling function
    `luks2_recover_key()`. This makes it really hard to discover why a given
    key actually failed to decrypt a disk.

    Improve this by including the error message in the user-visible output.
    While at it, fix one error path in `luks2_decrypt_key()` that printed
    the error directly instead of returning it.

    Signed-off-by: Patrick Steinhardt <address@hidden>

Patrick

signature.asc
Description: PGP signature