grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Daniel Kiper
Subject: Re: [PATCH] verifiers: fix double close on pgp's sig file descriptor
Date: Wed, 14 Nov 2018 17:23:58 +0100
User-agent: NeoMutt/20170113 (1.7.2)

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.

Daniel



reply via email to

[Prev in Thread] Current Thread [Next in Thread]