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

[Daniel Kiper]

On Thu, Apr 16, 2020 at 02:36:10PM +0200, Patrick Steinhardt wrote:
> 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>

Much better. However, after seeing this I think that this patch should
be split into two separate ones. If you do that and split the commit
message accordingly feel free to add Reviewed-by: Daniel Kiper
<address@hidden>.

Daniel