verifiers: fix double close on pgp's sig file descriptor
PATCH v2 1/1 verifiers: fix double close on pgp's sig file descriptor
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.
v1 -> v2:
- drop change in grub_verify_signature_init()
--- grub-core/commands/pgp.c | 30 +++++++++++++++--------------- include/grub/pubkey.h | 2 +- 2 files changed, 16 insertions(+), 16 deletions(-)
grub-core/commands/pgp.c +15 -15
@@ -684,15 +684,26 @@ grub_pubkey_close (void *ctxt) }grub_err_t -grub_verify_signature (grub_file_t f,grub_file_t sig, +grub_verify_signature (grub_file_t f,const char *fsig,struct grub_public_key *pkey) { +grub_file_t sig;grub_err_t err;struct grub_pubkey_context ctxt;grub_uint8_t *readbuf =NULL ; + + sig =grub_file_open (fsig, +GRUB_FILE_TYPE_SIGNATURE + |GRUB_FILE_TYPE_NO_DECOMPRESS ); +if (!sig) +return grub_errno; + err =grub_verify_signature_init (&ctxt, sig);if (err) -return err; + { +grub_file_close (sig); +return err; + } readbuf =grub_zalloc (READBUF_SIZE );if (!readbuf)
@@ -806,7 +817,7 @@ static grub_err_t grub_cmd_verify_signature (grub_extcmd_context_t ctxt, int argc, char **args) { - grub_file_t f = NULL, sig = NULL; + grub_file_t f = NULL;grub_err_t err =GRUB_ERR_NONE ;struct grub_public_key *pk =NULL ;
@@ -844,19 +855,8 @@ grub_cmd_verify_signature (grub_extcmd_context_t ctxt, goto fail; } - sig =grub_file_open (args[1 ], -GRUB_FILE_TYPE_SIGNATURE - |GRUB_FILE_TYPE_NO_DECOMPRESS ); -if (!sig) - { - err = grub_errno; - goto fail; - } - - err =grub_verify_signature (f, sig, pk); + err =grub_verify_signature (f, args[1 ], pk); fail: -if (sig) -grub_file_close (sig);if (f)grub_file_close (f);if (pk)
include/grub/pubkey.h +1 -1
@@ -25,7 +25,7 @@ struct grub_public_key * grub_load_public_key (grub_file_t f);grub_err_t -grub_verify_signature (grub_file_t f,grub_file_t sig, +grub_verify_signature (grub_file_t f,const char *fsig,struct grub_public_key *pk);
Signature · 1 line
2.19.1
Quoted 19 lines · Michael Chang
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.sigalloc 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 ingrub_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.
Quoted 8 lines
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 togrub_verify_signature()with full tracing to opened file descriptor by itself.Signed-off-by Michael Chang <[email protected]>
Sadly patch does not apply. Could you rebase it on latest master?
Quoted 3 lines
v1 -> v2:
- drop change in
grub_verify_signature_init()
If you add a blurb to the commit message why it is needed then I am not against it. Even to some extent I am happy with it because it makes code a bit nicer.
Daniel
Quoted 27 lines · Daniel Kiper
Quoted 26 lines · Michael Chang
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.sigalloc 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 ingrub_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 togrub_verify_signature()with full tracing to opened file descriptor by itself.
Quoted 3 lines
Quoted 1 line
Signed-off-by Michael Chang <[email protected]>Sadly patch does not apply. Could you rebase it on latest master?
OK. The conflict is caused by new blank line inserted by new commit which is harmless.
Quoted 8 lines
Quoted 4 lines
v1 -> v2:
- drop change in
grub_verify_signature_init()If you add a blurb to the commit message why it is needed then I am not against it. Even to some extent I am happy with it because it makes code a bit nicer.
OK. I'll add that in upcoming v3 patch. :)
Thanks,
Michael
Quoted 7 lines
Daniel
_______________________________________________
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel