[PATCH v4 0/2] let kexec_file_load use platform keyring to verify the kernel image

[PATCH v4 0/2] let kexec_file_load use platform keyring to verify the kernel image

[Kairui Song]

This patch series adds a .platform_trusted_keys in system_keyring as the
reference to .platform keyring in integrity subsystem, when platform
keyring is being initialized it will be updated. So other component could
use this keyring as well.

This patch series also let kexec_file_load use platform keyring as fall
back if it failed to verify the image against secondary keyring, make it
possible to load kernel signed by keys provides by firmware.

After this patch kexec_file_load will be able to verify a signed PE
bzImage using keys in platform keyring.

Tested in a VM with locally signed kernel with pesign and imported the
cert to EFI's MokList variable.

To test this patch series on latest kernel, you need to ensure this commit
is applied as there is an regression bug in sanity_check_segment_list():

https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=993a110319a4a60aadbd02f6defdebe048f7773b

Update from V3:
  - Tweak and simplify commit message as suggested by Mimi Zohar

Update from V2:
  - Use IS_ENABLED in kexec_file_load to judge if platform_trusted_keys
    should be used for verifying image as suggested by Mimi Zohar

Update from V1:
  - Make platform_trusted_keys static, and update commit message as suggested
    by Mimi Zohar
  - Always check if platform keyring is initialized before use it

Kairui Song (2):
  integrity, KEYS: add a reference to platform keyring
  kexec, KEYS: Make use of platform keyring for signature verify

 arch/x86/kernel/kexec-bzimage64.c | 13 ++++++++++---
 certs/system_keyring.c            | 22 +++++++++++++++++++++-
 include/keys/system_keyring.h     |  5 +++++
 include/linux/verification.h      |  1 +
 security/integrity/digsig.c       |  6 ++++++
 5 files changed, 43 insertions(+), 4 deletions(-)

-- 
2.20.1

[PATCH v4 1/2] integrity, KEYS: add a reference to platform keyring

[Kairui Song]

commit 9dc92c45177a ('integrity: Define a trusted platform keyring')
introduced a .platform keyring for storing preboot keys, used for
verifying kernel images' signature. Currently only IMA-appraisal is able
to use the keyring to verify kernel images that have their signature
stored in xattr.

This patch exposes the .platform keyring, making it accessible for
verifying PE signed kernel images as well.

Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Kairui Song <kasong@redhat.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Tested-by: Mimi Zohar <zohar@linux.ibm.com>
---
 certs/system_keyring.c        | 9 +++++++++
 include/keys/system_keyring.h | 5 +++++
 security/integrity/digsig.c   | 6 ++++++
 3 files changed, 20 insertions(+)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 81728717523d..4690ef9cda8a 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -24,6 +24,9 @@ static struct key *builtin_trusted_keys;
 #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
 static struct key *secondary_trusted_keys;
 #endif
+#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
+static struct key *platform_trusted_keys;
+#endif
 
 extern __initconst const u8 system_certificate_list[];
 extern __initconst const unsigned long system_certificate_list_size;
@@ -265,4 +268,10 @@ int verify_pkcs7_signature(const void *data, size_t len,
 }
 EXPORT_SYMBOL_GPL(verify_pkcs7_signature);
 
+#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
+void __init set_platform_trusted_keys(struct key *keyring) {
+	platform_trusted_keys = keyring;
+}
+#endif
+
 #endif /* CONFIG_SYSTEM_DATA_VERIFICATION */
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index 359c2f936004..9e1b7849b6aa 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -61,5 +61,10 @@ static inline struct key *get_ima_blacklist_keyring(void)
 }
 #endif /* CONFIG_IMA_BLACKLIST_KEYRING */
 
+#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
+
+extern void __init set_platform_trusted_keys(struct key* keyring);
+
+#endif /* CONFIG_INTEGRITY_PLATFORM_KEYRING */
 
 #endif /* _KEYS_SYSTEM_KEYRING_H */
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index f45d6edecf99..bfabc2a8111d 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -89,6 +89,12 @@ static int __integrity_init_keyring(const unsigned int id, key_perm_t perm,
 		keyring[id] = NULL;
 	}
 
+#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
+	if (id == INTEGRITY_KEYRING_PLATFORM) {
+		set_platform_trusted_keys(keyring[id]);
+	}
+#endif
+
 	return err;
 }
 
-- 
2.20.1

Re: [PATCH v4 1/2] integrity, KEYS: add a reference to platform keyring

[Nayna]

On 01/18/2019 04:17 AM, Kairui Song wrote:
> commit 9dc92c45177a ('integrity: Define a trusted platform keyring')
> introduced a .platform keyring for storing preboot keys, used for
> verifying kernel images' signature. Currently only IMA-appraisal is able
> to use the keyring to verify kernel images that have their signature
> stored in xattr.
>
> This patch exposes the .platform keyring, making it accessible for
> verifying PE signed kernel images as well.
>
> Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> Signed-off-by: Kairui Song <kasong@redhat.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> Tested-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   certs/system_keyring.c        | 9 +++++++++
>   include/keys/system_keyring.h | 5 +++++
>   security/integrity/digsig.c   | 6 ++++++
>   3 files changed, 20 insertions(+)
>
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 81728717523d..4690ef9cda8a 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -24,6 +24,9 @@ static struct key *builtin_trusted_keys;
>   #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
>   static struct key *secondary_trusted_keys;
>   #endif
> +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> +static struct key *platform_trusted_keys;
> +#endif
>   
>   extern __initconst const u8 system_certificate_list[];
>   extern __initconst const unsigned long system_certificate_list_size;
> @@ -265,4 +268,10 @@ int verify_pkcs7_signature(const void *data, size_t len,
>   }
>   EXPORT_SYMBOL_GPL(verify_pkcs7_signature);
>   
> +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> +void __init set_platform_trusted_keys(struct key *keyring) {
> +	platform_trusted_keys = keyring;
> +}
> +#endif
> +
>   #endif /* CONFIG_SYSTEM_DATA_VERIFICATION */
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index 359c2f936004..9e1b7849b6aa 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -61,5 +61,10 @@ static inline struct key *get_ima_blacklist_keyring(void)
>   }
>   #endif /* CONFIG_IMA_BLACKLIST_KEYRING */
>   
> +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> +
> +extern void __init set_platform_trusted_keys(struct key* keyring);
> +
> +#endif /* CONFIG_INTEGRITY_PLATFORM_KEYRING */
>   
>   #endif /* _KEYS_SYSTEM_KEYRING_H */
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index f45d6edecf99..bfabc2a8111d 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -89,6 +89,12 @@ static int __integrity_init_keyring(const unsigned int id, key_perm_t perm,
>   		keyring[id] = NULL;
>   	}
>   
> +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> +	if (id == INTEGRITY_KEYRING_PLATFORM) {

Shouldn't it also check that keyring[id] is not NULL ?

Thanks & Regards,
     - Nayna

> +		set_platform_trusted_keys(keyring[id]);
> +	}
> +#endif
> +
>   	return err;
>   }
>   

Re: [PATCH v4 1/2] integrity, KEYS: add a reference to platform keyring

[Kairui Song]

On Fri, Jan 18, 2019 at 10:36 PM Nayna <nayna@linux.vnet.ibm.com> wrote:
> On 01/18/2019 04:17 AM, Kairui Song wrote:
> > commit 9dc92c45177a ('integrity: Define a trusted platform keyring')
> > introduced a .platform keyring for storing preboot keys, used for
> > verifying kernel images' signature. Currently only IMA-appraisal is able
> > to use the keyring to verify kernel images that have their signature
> > stored in xattr.
> >
> > This patch exposes the .platform keyring, making it accessible for
> > verifying PE signed kernel images as well.
> >
> > Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> > Signed-off-by: Kairui Song <kasong@redhat.com>
> > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > Tested-by: Mimi Zohar <zohar@linux.ibm.com>
> > ---
> >   certs/system_keyring.c        | 9 +++++++++
> >   include/keys/system_keyring.h | 5 +++++
> >   security/integrity/digsig.c   | 6 ++++++
> >   3 files changed, 20 insertions(+)
> >
> > diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> > index 81728717523d..4690ef9cda8a 100644
> > --- a/certs/system_keyring.c
> > +++ b/certs/system_keyring.c
> > @@ -24,6 +24,9 @@ static struct key *builtin_trusted_keys;
> >   #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> >   static struct key *secondary_trusted_keys;
> >   #endif
> > +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> > +static struct key *platform_trusted_keys;
> > +#endif
> >
> >   extern __initconst const u8 system_certificate_list[];
> >   extern __initconst const unsigned long system_certificate_list_size;
> > @@ -265,4 +268,10 @@ int verify_pkcs7_signature(const void *data, size_t len,
> >   }
> >   EXPORT_SYMBOL_GPL(verify_pkcs7_signature);
> >
> > +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> > +void __init set_platform_trusted_keys(struct key *keyring) {
> > +     platform_trusted_keys = keyring;
> > +}
> > +#endif
> > +
> >   #endif /* CONFIG_SYSTEM_DATA_VERIFICATION */
> > diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> > index 359c2f936004..9e1b7849b6aa 100644
> > --- a/include/keys/system_keyring.h
> > +++ b/include/keys/system_keyring.h
> > @@ -61,5 +61,10 @@ static inline struct key *get_ima_blacklist_keyring(void)
> >   }
> >   #endif /* CONFIG_IMA_BLACKLIST_KEYRING */
> >
> > +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> > +
> > +extern void __init set_platform_trusted_keys(struct key* keyring);
> > +
> > +#endif /* CONFIG_INTEGRITY_PLATFORM_KEYRING */
> >
> >   #endif /* _KEYS_SYSTEM_KEYRING_H */
> > diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> > index f45d6edecf99..bfabc2a8111d 100644
> > --- a/security/integrity/digsig.c
> > +++ b/security/integrity/digsig.c
> > @@ -89,6 +89,12 @@ static int __integrity_init_keyring(const unsigned int id, key_perm_t perm,
> >               keyring[id] = NULL;
> >       }
> >
> > +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> > +     if (id == INTEGRITY_KEYRING_PLATFORM) {
>
> Shouldn't it also check that keyring[id] is not NULL ?

Good catch, if it's NULL then platform_trusted_keyring will be set to
NULL as well, which will work just fine as in this case
platform_trusted_keyring is still considered not initialized. I'll add
a sanity check here to check err value just in case.
Thanks for your suggestion!

>
> Thanks & Regards,
>      - Nayna
>
> > +             set_platform_trusted_keys(keyring[id]);
> > +     }
> > +#endif
> > +
> >       return err;
> >   }
> >
>
>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec



-- 
Best Regards,
Kairui Song

[PATCH v4 2/2] kexec, KEYS: Make use of platform keyring for signature verify

[Kairui Song]

This patch let kexec_file_load makes use of .platform keyring as fall
back if it failed to verify a PE signed image against secondary or
builtin keyring, make it possible to verify kernel image signed with
preboot keys as well.

This commit adds a VERIFY_USE_PLATFORM_KEYRING similar to previous
VERIFY_USE_SECONDARY_KEYRING indicating that verify_pkcs7_signature
should verify the signature using platform keyring. Also, decrease
the error message log level when verification failed with -ENOKEY,
so that if called tried multiple time with different keyring it
won't generate extra noises.

Signed-off-by: Kairui Song <kasong@redhat.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Tested-by: Mimi Zohar <zohar@linux.ibm.com>
---
 arch/x86/kernel/kexec-bzimage64.c | 13 ++++++++++---
 certs/system_keyring.c            | 13 ++++++++++++-
 include/linux/verification.h      |  1 +
 3 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
index 7d97e432cbbc..2c007abd3d40 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -534,9 +534,16 @@ static int bzImage64_cleanup(void *loader_data)
 #ifdef CONFIG_KEXEC_BZIMAGE_VERIFY_SIG
 static int bzImage64_verify_sig(const char *kernel, unsigned long kernel_len)
 {
-	return verify_pefile_signature(kernel, kernel_len,
-				       VERIFY_USE_SECONDARY_KEYRING,
-				       VERIFYING_KEXEC_PE_SIGNATURE);
+	int ret;
+	ret = verify_pefile_signature(kernel, kernel_len,
+				      VERIFY_USE_SECONDARY_KEYRING,
+				      VERIFYING_KEXEC_PE_SIGNATURE);
+	if (ret == -ENOKEY && IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING)) {
+		ret = verify_pefile_signature(kernel, kernel_len,
+					      VERIFY_USE_PLATFORM_KEYRING,
+					      VERIFYING_KEXEC_PE_SIGNATURE);
+	}
+	return ret;
 }
 #endif
 
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 4690ef9cda8a..7085c286f4bd 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -240,11 +240,22 @@ int verify_pkcs7_signature(const void *data, size_t len,
 #else
 		trusted_keys = builtin_trusted_keys;
 #endif
+	} else if (trusted_keys == VERIFY_USE_PLATFORM_KEYRING) {
+#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
+		trusted_keys = platform_trusted_keys;
+#else
+		trusted_keys = NULL;
+#endif
+		if (!trusted_keys) {
+			ret = -ENOKEY;
+			pr_devel("PKCS#7 platform keyring is not available\n");
+			goto error;
+		}
 	}
 	ret = pkcs7_validate_trust(pkcs7, trusted_keys);
 	if (ret < 0) {
 		if (ret == -ENOKEY)
-			pr_err("PKCS#7 signature not signed with a trusted key\n");
+			pr_devel("PKCS#7 signature not signed with a trusted key\n");
 		goto error;
 	}
 
diff --git a/include/linux/verification.h b/include/linux/verification.h
index cfa4730d607a..018fb5f13d44 100644
--- a/include/linux/verification.h
+++ b/include/linux/verification.h
@@ -17,6 +17,7 @@
  * should be used.
  */
 #define VERIFY_USE_SECONDARY_KEYRING ((struct key *)1UL)
+#define VERIFY_USE_PLATFORM_KEYRING  ((struct key *)2UL)
 
 /*
  * The use to which an asymmetric key is being put.
-- 
2.20.1

Re: [PATCH v4 0/2] let kexec_file_load use platform keyring to verify the kernel image

[Mimi Zohar]

On Fri, 2019-01-18 at 17:17 +0800, Kairui Song wrote:
> This patch series adds a .platform_trusted_keys in system_keyring as the
> reference to .platform keyring in integrity subsystem, when platform
> keyring is being initialized it will be updated. So other component could
> use this keyring as well.

Kairui, when people review patches, the comments could be specific,
but are normally generic.  My review included a couple of generic
suggestions - not to use "#ifdef" in C code (eg. is_enabled), use the
term "preboot" keys, and remove any references to "other components".

After all the wording suggestions I've made, you are still saying, "So
other components could use this keyring as well".  Really?!  How the
platform keyring will be used in the future, is up to you and others
to convince Linus.  At least for now, please limit its usage to
verifying the PE signed kernel image.  If this patch set needs to be
reposted, please remove all references to "other components".

Dave/David, are you ok with Kairui's usage of "#ifdef's"?  Dave, you
Acked the original post.  Can I include it?  Can we get some
additional Ack's on these patches?

thanks!

Mimi


> 
> This patch series also let kexec_file_load use platform keyring as fall
> back if it failed to verify the image against secondary keyring, make it
> possible to load kernel signed by keys provides by firmware.
> 
> After this patch kexec_file_load will be able to verify a signed PE
> bzImage using keys in platform keyring.
> 
> Tested in a VM with locally signed kernel with pesign and imported the
> cert to EFI's MokList variable.
> 
> To test this patch series on latest kernel, you need to ensure this commit
> is applied as there is an regression bug in sanity_check_segment_list():
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=993a110319a4a60aadbd02f6defdebe048f7773b
> 
> Update from V3:
>   - Tweak and simplify commit message as suggested by Mimi Zohar
> 
> Update from V2:
>   - Use IS_ENABLED in kexec_file_load to judge if platform_trusted_keys
>     should be used for verifying image as suggested by Mimi Zohar
> 
> Update from V1:
>   - Make platform_trusted_keys static, and update commit message as suggested
>     by Mimi Zohar
>   - Always check if platform keyring is initialized before use it
> 
> Kairui Song (2):
>   integrity, KEYS: add a reference to platform keyring
>   kexec, KEYS: Make use of platform keyring for signature verify
> 
>  arch/x86/kernel/kexec-bzimage64.c | 13 ++++++++++---
>  certs/system_keyring.c            | 22 +++++++++++++++++++++-
>  include/keys/system_keyring.h     |  5 +++++
>  include/linux/verification.h      |  1 +
>  security/integrity/digsig.c       |  6 ++++++
>  5 files changed, 43 insertions(+), 4 deletions(-)
> 

Re: [PATCH v4 0/2] let kexec_file_load use platform keyring to verify the kernel image

[Kairui Song]

On Fri, Jan 18, 2019, 19:54 Mimi Zohar <zohar@linux.ibm.com wrote:
>
> On Fri, 2019-01-18 at 17:17 +0800, Kairui Song wrote:
> > This patch series adds a .platform_trusted_keys in system_keyring as the
> > reference to .platform keyring in integrity subsystem, when platform
> > keyring is being initialized it will be updated. So other component could
> > use this keyring as well.
>
> Kairui, when people review patches, the comments could be specific,
> but are normally generic.  My review included a couple of generic
> suggestions - not to use "#ifdef" in C code (eg. is_enabled), use the
> term "preboot" keys, and remove any references to "other components".
>
> After all the wording suggestions I've made, you are still saying, "So
> other components could use this keyring as well".  Really?!  How the
> platform keyring will be used in the future, is up to you and others
> to convince Linus.  At least for now, please limit its usage to
> verifying the PE signed kernel image.  If this patch set needs to be
> reposted, please remove all references to "other components".
>
> Dave/David, are you ok with Kairui's usage of "#ifdef's"?  Dave, you
> Acked the original post.  Can I include it?  Can we get some
> additional Ack's on these patches?
>
> thanks!
>
> Mimi
>

Hi, Mimi, thanks for your feedback. My bad I reused the old cover
letter without checking it carefully, hopefully, the commit messages
should have a proper wording now. If the cover letter needs to be
updated I can resend the patch, let me just hold a while before update
again.

Re: [PATCH v4 0/2] let kexec_file_load use platform keyring to verify the kernel image

[Dave Young]

On 01/18/19 at 06:53am, Mimi Zohar wrote:
> On Fri, 2019-01-18 at 17:17 +0800, Kairui Song wrote:
> > This patch series adds a .platform_trusted_keys in system_keyring as the
> > reference to .platform keyring in integrity subsystem, when platform
> > keyring is being initialized it will be updated. So other component could
> > use this keyring as well.
> 
> Kairui, when people review patches, the comments could be specific,
> but are normally generic.  My review included a couple of generic
> suggestions - not to use "#ifdef" in C code (eg. is_enabled), use the
> term "preboot" keys, and remove any references to "other components".
> 
> After all the wording suggestions I've made, you are still saying, "So
> other components could use this keyring as well".  Really?!  How the
> platform keyring will be used in the future, is up to you and others
> to convince Linus.  At least for now, please limit its usage to
> verifying the PE signed kernel image.  If this patch set needs to be
> reposted, please remove all references to "other components".
> 
> Dave/David, are you ok with Kairui's usage of "#ifdef's"?  Dave, you
> Acked the original post.  Can I include it?  Can we get some
> additional Ack's on these patches?

It is better to update patch to use IS_ENABLED in patch 1/2 as well.
Other than that, for kexec part I'm fine with an ack.
 
Thanks
Dave

Re: [PATCH v4 0/2] let kexec_file_load use platform keyring to verify the kernel image

[Dave Young]

On 01/18/19 at 08:34pm, Dave Young wrote:
> On 01/18/19 at 06:53am, Mimi Zohar wrote:
> > On Fri, 2019-01-18 at 17:17 +0800, Kairui Song wrote:
> > > This patch series adds a .platform_trusted_keys in system_keyring as the
> > > reference to .platform keyring in integrity subsystem, when platform
> > > keyring is being initialized it will be updated. So other component could
> > > use this keyring as well.
> > 
> > Kairui, when people review patches, the comments could be specific,
> > but are normally generic.  My review included a couple of generic
> > suggestions - not to use "#ifdef" in C code (eg. is_enabled), use the
> > term "preboot" keys, and remove any references to "other components".
> > 
> > After all the wording suggestions I've made, you are still saying, "So
> > other components could use this keyring as well".  Really?!  How the
> > platform keyring will be used in the future, is up to you and others
> > to convince Linus.  At least for now, please limit its usage to
> > verifying the PE signed kernel image.  If this patch set needs to be
> > reposted, please remove all references to "other components".
> > 
> > Dave/David, are you ok with Kairui's usage of "#ifdef's"?  Dave, you
> > Acked the original post.  Can I include it?  Can we get some
> > additional Ack's on these patches?
> 
> It is better to update patch to use IS_ENABLED in patch 1/2 as well.

Hmm, not only for patch 1/2, patch 2/2 also need an update

> Other than that, for kexec part I'm fine with an ack.
>  
> Thanks
> Dave

Re: [PATCH v4 0/2] let kexec_file_load use platform keyring to verify the kernel image

[Kairui Song]

On Fri, Jan 18, 2019 at 8:37 PM Dave Young <dyoung@redhat.com> wrote:
>
> On 01/18/19 at 08:34pm, Dave Young wrote:
> > On 01/18/19 at 06:53am, Mimi Zohar wrote:
> > > On Fri, 2019-01-18 at 17:17 +0800, Kairui Song wrote:
> > > > This patch series adds a .platform_trusted_keys in system_keyring as the
> > > > reference to .platform keyring in integrity subsystem, when platform
> > > > keyring is being initialized it will be updated. So other component could
> > > > use this keyring as well.
> > >
> > > Kairui, when people review patches, the comments could be specific,
> > > but are normally generic.  My review included a couple of generic
> > > suggestions - not to use "#ifdef" in C code (eg. is_enabled), use the
> > > term "preboot" keys, and remove any references to "other components".
> > >
> > > After all the wording suggestions I've made, you are still saying, "So
> > > other components could use this keyring as well".  Really?!  How the
> > > platform keyring will be used in the future, is up to you and others
> > > to convince Linus.  At least for now, please limit its usage to
> > > verifying the PE signed kernel image.  If this patch set needs to be
> > > reposted, please remove all references to "other components".
> > >
> > > Dave/David, are you ok with Kairui's usage of "#ifdef's"?  Dave, you
> > > Acked the original post.  Can I include it?  Can we get some
> > > additional Ack's on these patches?
> >
> > It is better to update patch to use IS_ENABLED in patch 1/2 as well.
>
> Hmm, not only for patch 1/2, patch 2/2 also need an update
>
> > Other than that, for kexec part I'm fine with an ack.
> >
> > Thanks
> > Dave

Thanks for the review again, will update the patch using IS_ENABLED
along with update the cover letter shortly.

-- 
Best Regards,
Kairui Song

Re: [PATCH v4 0/2] let kexec_file_load use platform keyring to verify the kernel image

[Kairui Song]

On Fri, Jan 18, 2019 at 9:42 PM Kairui Song <kasong@redhat.com> wrote:
>
> On Fri, Jan 18, 2019 at 8:37 PM Dave Young <dyoung@redhat.com> wrote:
> >
> > On 01/18/19 at 08:34pm, Dave Young wrote:
> > > On 01/18/19 at 06:53am, Mimi Zohar wrote:
> > > > On Fri, 2019-01-18 at 17:17 +0800, Kairui Song wrote:
> > > > > This patch series adds a .platform_trusted_keys in system_keyring as the
> > > > > reference to .platform keyring in integrity subsystem, when platform
> > > > > keyring is being initialized it will be updated. So other component could
> > > > > use this keyring as well.
> > > >
> > > > Kairui, when people review patches, the comments could be specific,
> > > > but are normally generic.  My review included a couple of generic
> > > > suggestions - not to use "#ifdef" in C code (eg. is_enabled), use the
> > > > term "preboot" keys, and remove any references to "other components".
> > > >
> > > > After all the wording suggestions I've made, you are still saying, "So
> > > > other components could use this keyring as well".  Really?!  How the
> > > > platform keyring will be used in the future, is up to you and others
> > > > to convince Linus.  At least for now, please limit its usage to
> > > > verifying the PE signed kernel image.  If this patch set needs to be
> > > > reposted, please remove all references to "other components".
> > > >
> > > > Dave/David, are you ok with Kairui's usage of "#ifdef's"?  Dave, you
> > > > Acked the original post.  Can I include it?  Can we get some
> > > > additional Ack's on these patches?
> > >
> > > It is better to update patch to use IS_ENABLED in patch 1/2 as well.
> >
> > Hmm, not only for patch 1/2, patch 2/2 also need an update
> >
> > > Other than that, for kexec part I'm fine with an ack.
> > >
> > > Thanks
> > > Dave
>
> Thanks for the review again, will update the patch using IS_ENABLED
> along with update the cover letter shortly.
>
> --
> Best Regards,
> Kairui Song

Hi, before I update it again, most part of the new
platform_trusted_keyring related code is following how
secondary_trusted_keyring is implemented (surrounded by ifdefs). I
thought this could reduce unused code when the keyring is not enabled.
Else, all ifdef could be simply removed, when platform_keyring is not
enabled, the platform_trusted_keys will always be NULL, and
verify_pkcs7_signature will simply return NOKEY if anyone try to use
platform keyring.

Any suggestions? Or I can just remove the ifdef in
security/integrity/digsig.c and make set_platform_trusted_keys a
inline empty function in system_keyring.h.

-- 
Best Regards,
Kairui Song

Re: [PATCH v4 0/2] let kexec_file_load use platform keyring to verify the kernel image

[Kairui Song]

On Fri, Jan 18, 2019 at 10:28 PM Kairui Song <kasong@redhat.com> wrote:
>
> On Fri, Jan 18, 2019 at 9:42 PM Kairui Song <kasong@redhat.com> wrote:
> >
> > On Fri, Jan 18, 2019 at 8:37 PM Dave Young <dyoung@redhat.com> wrote:
> > >
> > > On 01/18/19 at 08:34pm, Dave Young wrote:
> > > > On 01/18/19 at 06:53am, Mimi Zohar wrote:
> > > > > On Fri, 2019-01-18 at 17:17 +0800, Kairui Song wrote:
> > > > > > This patch series adds a .platform_trusted_keys in system_keyring as the
> > > > > > reference to .platform keyring in integrity subsystem, when platform
> > > > > > keyring is being initialized it will be updated. So other component could
> > > > > > use this keyring as well.
> > > > >
> > > > > Kairui, when people review patches, the comments could be specific,
> > > > > but are normally generic.  My review included a couple of generic
> > > > > suggestions - not to use "#ifdef" in C code (eg. is_enabled), use the
> > > > > term "preboot" keys, and remove any references to "other components".
> > > > >
> > > > > After all the wording suggestions I've made, you are still saying, "So
> > > > > other components could use this keyring as well".  Really?!  How the
> > > > > platform keyring will be used in the future, is up to you and others
> > > > > to convince Linus.  At least for now, please limit its usage to
> > > > > verifying the PE signed kernel image.  If this patch set needs to be
> > > > > reposted, please remove all references to "other components".
> > > > >
> > > > > Dave/David, are you ok with Kairui's usage of "#ifdef's"?  Dave, you
> > > > > Acked the original post.  Can I include it?  Can we get some
> > > > > additional Ack's on these patches?
> > > >
> > > > It is better to update patch to use IS_ENABLED in patch 1/2 as well.
> > >
> > > Hmm, not only for patch 1/2, patch 2/2 also need an update
> > >
> > > > Other than that, for kexec part I'm fine with an ack.
> > > >
> > > > Thanks
> > > > Dave
> >
> > Thanks for the review again, will update the patch using IS_ENABLED
> > along with update the cover letter shortly.
> >
> > --
> > Best Regards,
> > Kairui Song
>
> Hi, before I update it again, most part of the new
> platform_trusted_keyring related code is following how
> secondary_trusted_keyring is implemented (surrounded by ifdefs). I
> thought this could reduce unused code when the keyring is not enabled.
> Else, all ifdef could be simply removed, when platform_keyring is not
> enabled, the platform_trusted_keys will always be NULL, and
> verify_pkcs7_signature will simply return NOKEY if anyone try to use
> platform keyring.
>
> Any suggestions? Or I can just remove the ifdef in
> security/integrity/digsig.c and make set_platform_trusted_keys a
> inline empty function in system_keyring.h.
>
> --
> Best Regards,
> Kairui Song

Hi, after a second thought I'll drop the #ifdef in
security/integrity/digsig.c in PATCH 1/2, and make the
set_platform_trusted_keys function a empty inline function when
CONFIG_INTEGRITY_PLATFORM_KEYRING is undefined.
But for other ifdefs in certs/system_keyring.c I think maybe just keep
then untouched. They were used to strip out the
platform_trusted_keyring variable and related function when
CONFIG_INTEGRITY_PLATFORM_KEYRING is not used, this should help reduce
unused code and prevent compile error, also make code style aligns
with existing code in system_keyring.c.

Will sent v5 with above updates and fix a potential problem found by Nayna.


--
Best Regards,
Kairui Song