Re: [PATCH 2/2] [FIXED v2] Replace magic for trusting the secondary keyring with #define

From: Dave Young <dyoung@redhat.com>
To: Yannik Sembritzki <yannik@sembritzki.me>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	David Howells <dhowells@redhat.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Peter Anvin <hpa@zytor.com>,
	the arch/x86 maintainers <x86@kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Baoquan He <bhe@redhat.com>, Justin Forbes <jforbes@redhat.com>,
	Peter Jones <pjones@redhat.com>,
	James Bottomley <James.Bottomley@hansenpartnership.com>,
	Matthew Garrett <mjg59@google.com>,
	Vivek Goyal <vgoyal@redhat.com>
Subject: Re: [PATCH 2/2] [FIXED v2] Replace magic for trusting the secondary keyring with #define
Date: Thu, 16 Aug 2018 09:11:06 +0800	[thread overview]
Message-ID: <20180816011106.GC5915@dhcp-128-65.nay.redhat.com> (raw)
In-Reply-To: <fcc28be0-f066-9c7a-7a4e-05d0cc1c0967@sembritzki.me>

On 08/16/18 at 12:07am, Yannik Sembritzki wrote:
> Signed-off-by: Yannik Sembritzki <yannik@sembritzki.me>
> ---
>  arch/x86/kernel/kexec-bzimage64.c       | 2 +-
>  certs/system_keyring.c                  | 3 ++-
>  crypto/asymmetric_keys/pkcs7_key_type.c | 2 +-
>  include/linux/verification.h            | 3 +++
>  4 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/kexec-bzimage64.c
> b/arch/x86/kernel/kexec-bzimage64.c
> index 74628275..97d199a3 100644
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
>  static int bzImage64_verify_sig(const char *kernel, unsigned long
> kernel_len)
>  {
>      return verify_pefile_signature(kernel, kernel_len,
> -                       ((struct key *)1UL),
> +                       TRUST_SECONDARY_KEYRING,

Instead of fix your 1st patch in 2nd patch, I would suggest to
switch the patch order.  In 1st patch change the common code to use
the new macro and in 2nd patch you can directly fix the kexec code
with TRUST_SECONDARY_KEYRING.

>                         VERIFYING_KEXEC_PE_SIGNATURE);
>  }
>  #endif
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 6251d1b2..777ac7d2 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -15,6 +15,7 @@
>  #include <linux/cred.h>
>  #include <linux/err.h>
>  #include <linux/slab.h>
> +#include <linux/verification.h>
>  #include <keys/asymmetric-type.h>
>  #include <keys/system_keyring.h>
>  #include <crypto/pkcs7.h>
> @@ -230,7 +231,7 @@ int verify_pkcs7_signature(const void *data, size_t len,
>  
>      if (!trusted_keys) {
>          trusted_keys = builtin_trusted_keys;
> -    } else if (trusted_keys == (void *)1UL) {
> +    } else if (trusted_keys == TRUST_SECONDARY_KEYRING) {
>  #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
>          trusted_keys = secondary_trusted_keys;
>  #else
> diff --git a/crypto/asymmetric_keys/pkcs7_key_type.c
> b/crypto/asymmetric_keys/pkcs7_key_type.c
> index e284d9cb..0783e555 100644
> --- a/crypto/asymmetric_keys/pkcs7_key_type.c
> +++ b/crypto/asymmetric_keys/pkcs7_key_type.c
> @@ -63,7 +63,7 @@ static int pkcs7_preparse(struct key_preparsed_payload
> *prep)
>  
>      return verify_pkcs7_signature(NULL, 0,
>                        prep->data, prep->datalen,
> -                      (void *)1UL, usage,
> +                      TRUST_SECONDARY_KEYRING, usage,
>                        pkcs7_view_content, prep);
>  }
>  
> diff --git a/include/linux/verification.h b/include/linux/verification.h
> index a10549a6..c00c1143 100644
> --- a/include/linux/verification.h
> +++ b/include/linux/verification.h
> @@ -12,6 +12,9 @@
>  #ifndef _LINUX_VERIFICATION_H
>  #define _LINUX_VERIFICATION_H
>  
> +// Allow both builtin trusted keys and secondary trusted keys

It would be better to use commenting style /*

> +#define TRUST_SECONDARY_KEYRING ((struct key *)1UL)
> +
>  /*
>   * The use to which an asymmetric key is being put.
>   */
> -- 
> 2.17.1
> 
> 

Thanks
Dave