From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1gNEL1-0007Ho-O8 for mharc-grub-devel@gnu.org; Thu, 15 Nov 2018 04:53:27 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43256) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gNEKz-0007Ff-Uj for grub-devel@gnu.org; Thu, 15 Nov 2018 04:53:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gNEKs-0002sg-Ps for grub-devel@gnu.org; Thu, 15 Nov 2018 04:53:24 -0500 Received: from smtp2.provo.novell.com ([137.65.250.81]:38052) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1gNEKs-0002qm-FB for grub-devel@gnu.org; Thu, 15 Nov 2018 04:53:18 -0500 Received: from mazu (prv-ext-foundry1int.gns.novell.com [137.65.251.240]) by smtp2.provo.novell.com with ESMTP (TLS encrypted); Thu, 15 Nov 2018 02:53:06 -0700 Date: Thu, 15 Nov 2018 17:53:01 +0800 From: Michael Chang To: Daniel Kiper Cc: grub-devel@gnu.org Subject: Re: [PATCH] verifiers: fix double close on pgp's sig file descriptor Message-ID: <20181115095301.GA10734@mazu> Mail-Followup-To: Daniel Kiper , grub-devel@gnu.org References: <20181113063118.GA15556@mazu> <20181114162358.hw7hvdz2bvp7webv@tomti.i.net-space.pl> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181114162358.hw7hvdz2bvp7webv@tomti.i.net-space.pl> User-Agent: Mutt/1.10.1 (2018-07-13) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 137.65.250.81 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 15 Nov 2018 09:53:26 -0000 On Wed, Nov 14, 2018 at 05:23:58PM +0100, Daniel Kiper wrote: > On Tue, Nov 13, 2018 at 02:31:18PM +0800, Michael Chang wrote: > > An error emerged as when I was tesing the verifiers branch, so instead > > of putting it in pgp prefix, the verifiers is used to reflect what the > > patch is based on. > > > > While running verify_detached, grub aborts with error. > > > > verify_detached /@/.snapshots/1/snapshot/boot/grub/grub.cfg > > /@/.snapshots/1/snapshot/boot/grub/grub.cfg.sig > > > > alloc magic is broken at 0x7beea660: 0 > > Aborted. Press any key to exit. > > > > The error is caused by sig file desciptor been closed twice, first time > > in grub_verify_signature() to which it is passed as parameter. Second in > > grub_cmd_verify_signature() or in whichever opens the sig file > > decriptor. The second close is not consider as bug to me either, as in > > common rule of what opens a file has to close it to avoid file > > descriptor leakage. > > > > Afterall the design of grub_verify_signature() makes it diffcult to keep > > a good trace on opened file descriptor from it's caller. Let's refine > > the application interface to accept file path rather than descriptor, in > > this way the caller doesn't have to care about closing the descriptor by > > delegating it to grub_verify_signature() with full tracing to opened > > file descriptor by itself. > > > > Signed-off-by: Michael Chang > > --- > > grub-core/commands/pgp.c | 33 ++++++++++++++++----------------- > > include/grub/pubkey.h | 2 +- > > 2 files changed, 17 insertions(+), 18 deletions(-) > > > > diff --git a/grub-core/commands/pgp.c b/grub-core/commands/pgp.c > > index d5d7c0f0a..f294057b5 100644 > > --- a/grub-core/commands/pgp.c > > +++ b/grub-core/commands/pgp.c > > @@ -495,13 +495,12 @@ grub_verify_signature_init (struct grub_pubkey_context *ctxt, grub_file_t sig) > > > > grub_dprintf ("crypt", "alive\n"); > > > > - ctxt->sig = sig; > > - > > ctxt->hash_context = grub_zalloc (ctxt->hash->contextsize); > > if (!ctxt->hash_context) > > return grub_errno; > > > > ctxt->hash->init (ctxt->hash_context); > > + ctxt->sig = sig; > > This change does not seem to belong to this patch. Otherwise it LGTM. > You can split this patch into two patches or add a blurb about this change > into commit message or drop it at all. I would choose first option. We can drop this. I just wanted to make it clear the sig descriptor is not referenced in error returning path of grub_verify_signature_init() so that we feel more comfortable to close it directly. Thanks, Michael > > Daniel