From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1jP458-0006Ao-H6 for mharc-grub-devel@gnu.org; Thu, 16 Apr 2020 08:57:26 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:57689) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jP455-000695-C9 for grub-devel@gnu.org; Thu, 16 Apr 2020 08:57:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1jP452-0008SH-OV for grub-devel@gnu.org; Thu, 16 Apr 2020 08:57:23 -0400 Received: from dibed.net-space.pl ([84.10.22.86]:42869) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:24) (Exim 4.71) (envelope-from ) id 1jP452-0008Pd-B6 for grub-devel@gnu.org; Thu, 16 Apr 2020 08:57:20 -0400 Received: from router-fw.i.net-space.pl ([192.168.52.1]:59700 "EHLO tomti.i.net-space.pl") by router-fw-old.i.net-space.pl with ESMTP id S1949783AbgDPM5S (ORCPT ); Thu, 16 Apr 2020 14:57:18 +0200 X-Comment: RFC 2476 MSA function at dibed.net-space.pl logged sender identity as: dkiper Date: Thu, 16 Apr 2020 14:57:14 +0200 From: Daniel Kiper To: Patrick Steinhardt Cc: grub-devel@gnu.org Subject: Re: [PATCH] luks2: Improve error reporting when decrypting/verifying key Message-ID: <20200416125714.pfpfas7zbsyebtrh@tomti.i.net-space.pl> References: <20200416122702.pbr3r67ahrt65zcj@tomti.i.net-space.pl> <20200416123610.GB277885@tanuki.pks.im> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200416123610.GB277885@tanuki.pks.im> User-Agent: NeoMutt/20170113 (1.7.2) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 84.10.22.86 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Apr 2020 12:57:25 -0000 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 > > > --- > > > 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 > > > > 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 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 . Daniel