[RFC,v2,1/2] integrity, KEYS: add a reference to platform keyring
diff mbox series

Message ID 20190115094542.17129-2-kasong@redhat.com
State Superseded
Headers show
Series
  • let kexec_file_load use platform keyring to verify the kernel image
Related show

Commit Message

Kairui Song Jan. 15, 2019, 9:45 a.m. UTC
Currently when loading new kernel via kexec_file_load syscall, it is able
to verify the signed PE bzimage against .builtin_trusted_keys or
.secondary_trusted_keys. But the image could be signed with third part
keys which will be provided by platform or firmware as EFI variable (eg.
stored in MokListRT EFI variable), and the keys won't be available in
keyrings mentioned above.

After commit 9dc92c45177a ('integrity: Define a trusted platform keyring')
a .platform keyring is introduced to store the keys provided by platform
or firmware, this keyring is intended to be used for verifying kernel
images being loaded by kexec_file_load syscall. And with a few following
up commits, keys provided by firmware is being loaded into this keyring,
and IMA-appraisal is able to use the keyring to verify kernel images.
IMA is the currently the only user of that keyring.

This patch exposes the .platform, and makes it useable for other
components. For example, kexec_file_load could use this .platform
keyring to verify the kernel image's image.

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

Comments

Mimi Zohar Jan. 15, 2019, 3:34 p.m. UTC | #1
On Tue, 2019-01-15 at 17:45 +0800, Kairui Song wrote:
[snip]

> 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;
>  }
>  

Any reason for setting it here as opposed to in the caller
platform_keyring_init()?

Mimi
Kairui Song Jan. 15, 2019, 3:47 p.m. UTC | #2
On Tue, Jan 15, 2019 at 11:34 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Tue, 2019-01-15 at 17:45 +0800, Kairui Song wrote:
> [snip]
>
> > 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;
> >  }
> >
>
> Any reason for setting it here as opposed to in the caller
> platform_keyring_init()?
>
> Mimi
>

Yes, "keyring" is static so unless I expose it to other files, it is
only accessible here. And I think there should be no problem to put
the set_platform_trusted_keys here.
Mimi Zohar Jan. 15, 2019, 3:54 p.m. UTC | #3
On Tue, 2019-01-15 at 23:47 +0800, Kairui Song wrote:
> On Tue, Jan 15, 2019 at 11:34 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Tue, 2019-01-15 at 17:45 +0800, Kairui Song wrote:
> > [snip]
> >
> > > 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;
> > >  }
> > >
> >
> > Any reason for setting it here as opposed to in the caller
> > platform_keyring_init()?
> >
> > Mimi
> >
> 
> Yes, "keyring" is static so unless I expose it to other files, it is
> only accessible here. And I think there should be no problem to put
> the set_platform_trusted_keys here.

Right, that's a really good reason.

Mimi

Patch
diff mbox series

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;
 }