Re: [PATCH] verifiers: fix double close on pgp's sig file descriptor

[Michael Chang]

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 <address@hidden>
> > ---
> >  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