Re: [PATCH V34 08/29] kexec_file: split KEXEC_VERIFY_SIG into KEXEC_SIG and KEXEC_SIG_FORCE
From: Dave Young
Date: Mon Jun 24 2019 - 22:35:26 EST
On 06/24/19 at 10:01am, Dave Young wrote:
> On 06/21/19 at 05:03pm, Matthew Garrett wrote:
> > From: Jiri Bohac <jbohac@xxxxxxx>
> >
> > This is a preparatory patch for kexec_file_load() lockdown. A locked down
> > kernel needs to prevent unsigned kernel images from being loaded with
> > kexec_file_load(). Currently, the only way to force the signature
> > verification is compiling with KEXEC_VERIFY_SIG. This prevents loading
> > usigned images even when the kernel is not locked down at runtime.
> >
> > This patch splits KEXEC_VERIFY_SIG into KEXEC_SIG and KEXEC_SIG_FORCE.
> > Analogous to the MODULE_SIG and MODULE_SIG_FORCE for modules, KEXEC_SIG
> > turns on the signature verification but allows unsigned images to be
> > loaded. KEXEC_SIG_FORCE disallows images without a valid signature.
> >
> > [Modified by David Howells such that:
> >
> > (1) verify_pefile_signature() differentiates between no-signature and
> > sig-didn't-match in its returned errors.
> >
> > (2) kexec fails with EKEYREJECTED if there is a signature for which we
> > have a key, but signature doesn't match - even if in non-forcing mode.
> >
> > (3) kexec fails with EBADMSG or some other error if there is a signature
> > which cannot be parsed - even if in non-forcing mode.
> >
> > (4) kexec fails with ELIBBAD if the PE file cannot be parsed to extract
> > the signature - even if in non-forcing mode.
> >
> > ]
>
> Seems I do not see EBADMSG and ELIBBAD in this patch, also kexec fails
> with proper errno instead of EKEYREJECTED only.
>
> I may missed something? Other than the patch log issue:
>
> Reviewed-by: Dave Young <dyoung@xxxxxxxxxx>
Hold on :) Noticed another issue, please see comment inline..
>
> >
> > Signed-off-by: Jiri Bohac <jbohac@xxxxxxx>
> > Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
> > Signed-off-by: Matthew Garrett <mjg59@xxxxxxxxxx>
> > Reviewed-by: Jiri Bohac <jbohac@xxxxxxx>
> > cc: kexec@xxxxxxxxxxxxxxxxxxx
> > ---
> > arch/x86/Kconfig | 20 ++++++++---
> > crypto/asymmetric_keys/verify_pefile.c | 4 ++-
> > include/linux/kexec.h | 4 +--
> > kernel/kexec_file.c | 47 ++++++++++++++++++++++----
> > 4 files changed, 60 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index c1f9b3cf437c..84381dd60760 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -2012,20 +2012,30 @@ config KEXEC_FILE
> > config ARCH_HAS_KEXEC_PURGATORY
> > def_bool KEXEC_FILE
> >
> > -config KEXEC_VERIFY_SIG
> > +config KEXEC_SIG
> > bool "Verify kernel signature during kexec_file_load() syscall"
> > depends on KEXEC_FILE
> > ---help---
> > - This option makes kernel signature verification mandatory for
> > - the kexec_file_load() syscall.
> >
> > - In addition to that option, you need to enable signature
> > + This option makes the kexec_file_load() syscall check for a valid
> > + signature of the kernel image. The image can still be loaded without
> > + a valid signature unless you also enable KEXEC_SIG_FORCE, though if
> > + there's a signature that we can check, then it must be valid.
> > +
> > + In addition to this option, you need to enable signature
> > verification for the corresponding kernel image type being
> > loaded in order for this to work.
> >
> > +config KEXEC_SIG_FORCE
> > + bool "Require a valid signature in kexec_file_load() syscall"
> > + depends on KEXEC_SIG
> > + ---help---
> > + This option makes kernel signature verification mandatory for
> > + the kexec_file_load() syscall.
> > +
> > config KEXEC_BZIMAGE_VERIFY_SIG
> > bool "Enable bzImage signature verification support"
> > - depends on KEXEC_VERIFY_SIG
> > + depends on KEXEC_SIG
> > depends on SIGNED_PE_FILE_VERIFICATION
> > select SYSTEM_TRUSTED_KEYRING
> > ---help---
> > diff --git a/crypto/asymmetric_keys/verify_pefile.c b/crypto/asymmetric_keys/verify_pefile.c
> > index d178650fd524..4473cea1e877 100644
> > --- a/crypto/asymmetric_keys/verify_pefile.c
> > +++ b/crypto/asymmetric_keys/verify_pefile.c
> > @@ -100,7 +100,7 @@ static int pefile_parse_binary(const void *pebuf, unsigned int pelen,
> >
> > if (!ddir->certs.virtual_address || !ddir->certs.size) {
> > pr_debug("Unsigned PE binary\n");
> > - return -EKEYREJECTED;
> > + return -ENODATA;
> > }
> >
> > chkaddr(ctx->header_size, ddir->certs.virtual_address,
> > @@ -408,6 +408,8 @@ static int pefile_digest_pe(const void *pebuf, unsigned int pelen,
> > * (*) 0 if at least one signature chain intersects with the keys in the trust
> > * keyring, or:
> > *
> > + * (*) -ENODATA if there is no signature present.
> > + *
> > * (*) -ENOPKG if a suitable crypto module couldn't be found for a check on a
> > * chain.
> > *
> > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > index b9b1bc5f9669..58b27c7bdc2b 100644
> > --- a/include/linux/kexec.h
> > +++ b/include/linux/kexec.h
> > @@ -125,7 +125,7 @@ typedef void *(kexec_load_t)(struct kimage *image, char *kernel_buf,
> > unsigned long cmdline_len);
> > typedef int (kexec_cleanup_t)(void *loader_data);
> >
> > -#ifdef CONFIG_KEXEC_VERIFY_SIG
> > +#ifdef CONFIG_KEXEC_SIG
> > typedef int (kexec_verify_sig_t)(const char *kernel_buf,
> > unsigned long kernel_len);
> > #endif
> > @@ -134,7 +134,7 @@ struct kexec_file_ops {
> > kexec_probe_t *probe;
> > kexec_load_t *load;
> > kexec_cleanup_t *cleanup;
> > -#ifdef CONFIG_KEXEC_VERIFY_SIG
> > +#ifdef CONFIG_KEXEC_SIG
> > kexec_verify_sig_t *verify_sig;
> > #endif
> > };
> > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > index f1d0e00a3971..eec7e5bb2a08 100644
> > --- a/kernel/kexec_file.c
> > +++ b/kernel/kexec_file.c
> > @@ -90,7 +90,7 @@ int __weak arch_kimage_file_post_load_cleanup(struct kimage *image)
> > return kexec_image_post_load_cleanup_default(image);
> > }
> >
> > -#ifdef CONFIG_KEXEC_VERIFY_SIG
> > +#ifdef CONFIG_KEXEC_SIG
> > static int kexec_image_verify_sig_default(struct kimage *image, void *buf,
> > unsigned long buf_len)
> > {
> > @@ -188,7 +188,8 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
> > const char __user *cmdline_ptr,
> > unsigned long cmdline_len, unsigned flags)
> > {
> > - int ret = 0;
> > + const char *reason;
> > + int ret;
> > void *ldata;
> > loff_t size;
> >
> > @@ -207,15 +208,47 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
> > if (ret)
> > goto out;
> >
> > -#ifdef CONFIG_KEXEC_VERIFY_SIG
> > +#ifdef CONFIG_KEXEC_SIG
> > ret = arch_kexec_kernel_verify_sig(image, image->kernel_buf,
> > image->kernel_buf_len);
> > - if (ret) {
> > - pr_debug("kernel signature verification failed.\n");
> > +#else
> > + ret = -ENODATA;
Use -ENODATA for above case looks not correct, please just remove the #else and
move the #endif to the end of the switch chunk.
> > +#endif
> > +
> > + switch (ret) {
> > + case 0:
> > + break;
> > +
> > + /* Certain verification errors are non-fatal if we're not
> > + * checking errors, provided we aren't mandating that there
> > + * must be a valid signature.
> > + */
> > + case -ENODATA:
> > + reason = "kexec of unsigned image";
> > + goto decide;
> > + case -ENOPKG:
> > + reason = "kexec of image with unsupported crypto";
> > + goto decide;
> > + case -ENOKEY:
> > + reason = "kexec of image with unavailable key";
> > + decide:
> > + if (IS_ENABLED(CONFIG_KEXEC_SIG_FORCE)) {
> > + pr_notice("%s rejected\n", reason);
> > + goto out;
> > + }
> > +
> > + ret = 0;
> > + break;
> > +
> > + /* All other errors are fatal, including nomem, unparseable
> > + * signatures and signature check failures - even if signatures
> > + * aren't required.
> > + */
> > + default:
> > + pr_notice("kernel signature verification failed (%d).\n", ret);
> > goto out;
> > }
> > - pr_debug("kernel signature verification successful.\n");
> > -#endif
> > +
> > /* It is possible that there no initramfs is being loaded */
> > if (!(flags & KEXEC_FILE_NO_INITRAMFS)) {
> > ret = kernel_read_file_from_fd(initrd_fd, &image->initrd_buf,
> > --
> > 2.22.0.410.gd8fdbe21b5-goog
> >
Thanks
Dave