| Message ID | 20240311161111.3268190-2-eric.snowberg@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clavis LSM | expand |
On Mon Mar 11, 2024 at 6:11 PM EET, Eric Snowberg wrote: > Introduce a new function to allow a keyring to link to a key contained > within one of the system keyrings (builtin, secondary, or platform). > Depending on how the kernel is built, if the machine keyring is > available, it will be checked as well, since it is linked to the secondary > keyring. If the asymmetric key id matches a key within one of these > system keyrings, the matching key is linked into the passed in > keyring. > > Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com> > --- > certs/system_keyring.c | 29 +++++++++++++++++++++++++++++ > include/keys/system_keyring.h | 7 ++++++- > 2 files changed, 35 insertions(+), 1 deletion(-) > > diff --git a/certs/system_keyring.c b/certs/system_keyring.c > index 9de610bf1f4b..b647be49f6e0 100644 > --- a/certs/system_keyring.c > +++ b/certs/system_keyring.c > @@ -426,3 +426,32 @@ void __init set_platform_trusted_keys(struct key *keyring) > platform_trusted_keys = keyring; > } > #endif > + > +/** > + * system_key_link - Link to a system key > + * @keyring: The keyring to link into > + * @id: The asymmetric key id to look for in the system keyring > + */ > +int system_key_link(struct key *keyring, struct asymmetric_key_id *id) > +{ > + struct key *tkey; I'd suggest to replace this with just 'tkey'. Single obscure character does not bring any readability value. > + > +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING > + tkey = find_asymmetric_key(secondary_trusted_keys, id, NULL, NULL, false); > +#else > + tkey = find_asymmetric_key(builtin_trusted_keys, id, NULL, NULL, false); > +#endif I'd have just single call site here and inside ifdef-shenanigans i'd just set helper "keyring" to point to the appropriate keyring. > + if (!IS_ERR(tkey)) > + goto found; > + > + tkey = find_asymmetric_key(platform_trusted_keys, id, NULL, NULL, false); > + Please remove this empty line as the check below is associated with it. > + if (!IS_ERR(tkey)) > + goto found; > + > + return -1; > + > +found: > + key_link(keyring, tkey); > + return 0; > +} > diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h > index 8365adf842ef..b47ac8e2001a 100644 > --- a/include/keys/system_keyring.h > +++ b/include/keys/system_keyring.h > @@ -9,6 +9,7 @@ > #define _KEYS_SYSTEM_KEYRING_H > > #include <linux/key.h> > +struct asymmetric_key_id; > > enum blacklist_hash_type { > /* TBSCertificate hash */ > @@ -28,7 +29,7 @@ int restrict_link_by_digsig_builtin(struct key *dest_keyring, > const union key_payload *payload, > struct key *restriction_key); > extern __init int load_module_cert(struct key *keyring); > - > +extern int system_key_link(struct key *keyring, struct asymmetric_key_id *id); > #else > #define restrict_link_by_builtin_trusted restrict_link_reject > #define restrict_link_by_digsig_builtin restrict_link_reject > @@ -38,6 +39,10 @@ static inline __init int load_module_cert(struct key *keyring) > return 0; > } > > +static inline int system_key_link(struct key *keyring, struct asymmetric_key_id *id) > +{ > + return 0; > +} > #endif > > #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING BR, Jarkko
On Mon Mar 11, 2024 at 6:11 PM EET, Eric Snowberg wrote:
> + return -1;
Missed this one: why a magic number?
BR, Jarkko
> On Mar 11, 2024, at 1:16 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote: > > On Mon Mar 11, 2024 at 6:11 PM EET, Eric Snowberg wrote: >> Introduce a new function to allow a keyring to link to a key contained >> within one of the system keyrings (builtin, secondary, or platform). >> Depending on how the kernel is built, if the machine keyring is >> available, it will be checked as well, since it is linked to the secondary >> keyring. If the asymmetric key id matches a key within one of these >> system keyrings, the matching key is linked into the passed in >> keyring. >> >> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com> >> --- >> certs/system_keyring.c | 29 +++++++++++++++++++++++++++++ >> include/keys/system_keyring.h | 7 ++++++- >> 2 files changed, 35 insertions(+), 1 deletion(-) >> >> diff --git a/certs/system_keyring.c b/certs/system_keyring.c >> index 9de610bf1f4b..b647be49f6e0 100644 >> --- a/certs/system_keyring.c >> +++ b/certs/system_keyring.c >> @@ -426,3 +426,32 @@ void __init set_platform_trusted_keys(struct key *keyring) >> platform_trusted_keys = keyring; >> } >> #endif >> + >> +/** >> + * system_key_link - Link to a system key >> + * @keyring: The keyring to link into >> + * @id: The asymmetric key id to look for in the system keyring >> + */ >> +int system_key_link(struct key *keyring, struct asymmetric_key_id *id) >> +{ >> + struct key *tkey; > > I'd suggest to replace this with just 'tkey'. Single obscure character > does not bring any readability value. I assume you mean replace it with "key"? If so, yes, I'll change that in the next round. >> + >> +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING >> + tkey = find_asymmetric_key(secondary_trusted_keys, id, NULL, NULL, false); >> +#else >> + tkey = find_asymmetric_key(builtin_trusted_keys, id, NULL, NULL, false); >> +#endif > > I'd have just single call site here and inside ifdef-shenanigans i'd > just set helper "keyring" to point to the appropriate keyring. I'll also make this change >> + if (!IS_ERR(tkey)) >> + goto found; >> + >> + tkey = find_asymmetric_key(platform_trusted_keys, id, NULL, NULL, false); >> + > > Please remove this empty line as the check below is associated with it. and remove the empty line. Thanks.
> On Mar 11, 2024, at 1:18 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote: > > On Mon Mar 11, 2024 at 6:11 PM EET, Eric Snowberg wrote: >> + return -1; > > Missed this one: why a magic number? Good point, I'll change this to return -ENOKEY. Thanks.
> -----Original Message----- > From: Eric Snowberg <eric.snowberg@oracle.com> > Sent: Monday, March 11, 2024 9:41 PM > To: linux-security-module@vger.kernel.org > Cc: dhowells@redhat.com; dwmw2@infradead.org; > herbert@gondor.apana.org.au; davem@davemloft.net; ardb@kernel.org; > jarkko@kernel.org; paul@paul-moore.com; jmorris@namei.org; > serge@hallyn.com; zohar@linux.ibm.com; roberto.sassu@huawei.com; > dmitry.kasatkin@gmail.com; mic@digikod.net; casey@schaufler-ca.com; > stefanb@linux.ibm.com; eric.snowberg@oracle.com; linux- > kernel@vger.kernel.org; keyrings@vger.kernel.org; linux- > crypto@vger.kernel.org; linux-efi@vger.kernel.org; linux- > integrity@vger.kernel.org > Subject: [EXTERNAL] [PATCH RFC 1/8] certs: Introduce ability to link to a > system key > > Prioritize security for external emails: Confirm sender and content safety > before clicking links or opening attachments > > ---------------------------------------------------------------------- > Introduce a new function to allow a keyring to link to a key contained > within one of the system keyrings (builtin, secondary, or platform). > Depending on how the kernel is built, if the machine keyring is > available, it will be checked as well, since it is linked to the secondary > keyring. If the asymmetric key id matches a key within one of these > system keyrings, the matching key is linked into the passed in > keyring. > > Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com> > --- > certs/system_keyring.c | 29 +++++++++++++++++++++++++++++ > include/keys/system_keyring.h | 7 ++++++- > 2 files changed, 35 insertions(+), 1 deletion(-) > > diff --git a/certs/system_keyring.c b/certs/system_keyring.c > index 9de610bf1f4b..b647be49f6e0 100644 > --- a/certs/system_keyring.c > +++ b/certs/system_keyring.c > @@ -426,3 +426,32 @@ void __init set_platform_trusted_keys(struct key > *keyring) > platform_trusted_keys = keyring; > } > #endif > + > +/** > + * system_key_link - Link to a system key > + * @keyring: The keyring to link into > + * @id: The asymmetric key id to look for in the system keyring > + */ > +int system_key_link(struct key *keyring, struct asymmetric_key_id *id) > +{ > + struct key *tkey; > + > +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING > + tkey = find_asymmetric_key(secondary_trusted_keys, id, NULL, NULL, > false); > +#else > + tkey = find_asymmetric_key(builtin_trusted_keys, id, NULL, NULL, > false); > +#endif > + if (!IS_ERR(tkey)) > + goto found; > + > + tkey = find_asymmetric_key(platform_trusted_keys, id, NULL, NULL, > false); > + > + if (!IS_ERR(tkey)) > + goto found; > + > + return -1; Please use -ENOKEY. Thanks -Bharat > + > +found: > + key_link(keyring, tkey); > + return 0; > +} > diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h > index 8365adf842ef..b47ac8e2001a 100644 > --- a/include/keys/system_keyring.h > +++ b/include/keys/system_keyring.h > @@ -9,6 +9,7 @@ > #define _KEYS_SYSTEM_KEYRING_H > > #include <linux/key.h> > +struct asymmetric_key_id; > > enum blacklist_hash_type { > /* TBSCertificate hash */ > @@ -28,7 +29,7 @@ int restrict_link_by_digsig_builtin(struct key > *dest_keyring, > const union key_payload *payload, > struct key *restriction_key); > extern __init int load_module_cert(struct key *keyring); > - > +extern int system_key_link(struct key *keyring, struct asymmetric_key_id > *id); > #else > #define restrict_link_by_builtin_trusted restrict_link_reject > #define restrict_link_by_digsig_builtin restrict_link_reject > @@ -38,6 +39,10 @@ static inline __init int load_module_cert(struct key > *keyring) > return 0; > } > > +static inline int system_key_link(struct key *keyring, struct asymmetric_key_id > *id) > +{ > + return 0; > +} > #endif > > #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING > -- > 2.39.3 >
On Mon Mar 11, 2024 at 11:31 PM EET, Eric Snowberg wrote: > > > > On Mar 11, 2024, at 1:18 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote: > > > > On Mon Mar 11, 2024 at 6:11 PM EET, Eric Snowberg wrote: > >> + return -1; > > > > Missed this one: why a magic number? > > Good point, I'll change this to return -ENOKEY. Thanks. Either that or a boolean function, which ever fits to the overall flow better... The upside of error code is less branching in the call sites. The upside of boolean is that caller exactly knows all the values that ever should come out as a result. Your choice ofc. BR, Jarkko
Hi Eric, > diff --git a/certs/system_keyring.c b/certs/system_keyring.c > index 9de610bf1f4b..b647be49f6e0 100644 > --- a/certs/system_keyring.c > +++ b/certs/system_keyring.c > @@ -426,3 +426,32 @@ void __init set_platform_trusted_keys(struct key > *keyring) > platform_trusted_keys = keyring; > } > #endif > + > +/** > + * system_key_link - Link to a system key > + * @keyring: The keyring to link into > + * @id: The asymmetric key id to look for in the system keyring > + */ > +int system_key_link(struct key *keyring, struct asymmetric_key_id *id) > +{ > + struct key *tkey; > + > +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING > + tkey = find_asymmetric_key(secondary_trusted_keys, id, NULL, NULL, > false); > +#else > + tkey = find_asymmetric_key(builtin_trusted_keys, id, NULL, NULL, false); > +#endif > + if (!IS_ERR(tkey)) > + goto found; > + > + tkey = find_asymmetric_key(platform_trusted_keys, id, NULL, NULL, > false); > + > + if (!IS_ERR(tkey)) > + goto found; > + > + return -1; Normally "goto" is for the error. Invert the logic. Mimi > + > +found: > + key_link(keyring, tkey); > + return 0; > +} >
> On Apr 4, 2024, at 4:40 PM, Mimi Zohar <zohar@linux.ibm.com> wrote: > > Hi Eric, > >> diff --git a/certs/system_keyring.c b/certs/system_keyring.c >> index 9de610bf1f4b..b647be49f6e0 100644 >> --- a/certs/system_keyring.c >> +++ b/certs/system_keyring.c >> @@ -426,3 +426,32 @@ void __init set_platform_trusted_keys(struct key >> *keyring) >> platform_trusted_keys = keyring; >> } >> #endif >> + >> +/** >> + * system_key_link - Link to a system key >> + * @keyring: The keyring to link into >> + * @id: The asymmetric key id to look for in the system keyring >> + */ >> +int system_key_link(struct key *keyring, struct asymmetric_key_id *id) >> +{ >> + struct key *tkey; >> + >> +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING >> + tkey = find_asymmetric_key(secondary_trusted_keys, id, NULL, NULL, >> false); >> +#else >> + tkey = find_asymmetric_key(builtin_trusted_keys, id, NULL, NULL, false); >> +#endif >> + if (!IS_ERR(tkey)) >> + goto found; >> + >> + tkey = find_asymmetric_key(platform_trusted_keys, id, NULL, NULL, >> false); >> + >> + if (!IS_ERR(tkey)) >> + goto found; >> + >> + return -1; > > Normally "goto" is for the error. Invert the logic. Ok, I'll change that in the next version. Thanks.
diff --git a/certs/system_keyring.c b/certs/system_keyring.c index 9de610bf1f4b..b647be49f6e0 100644 --- a/certs/system_keyring.c +++ b/certs/system_keyring.c @@ -426,3 +426,32 @@ void __init set_platform_trusted_keys(struct key *keyring) platform_trusted_keys = keyring; } #endif + +/** + * system_key_link - Link to a system key + * @keyring: The keyring to link into + * @id: The asymmetric key id to look for in the system keyring + */ +int system_key_link(struct key *keyring, struct asymmetric_key_id *id) +{ + struct key *tkey; + +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING + tkey = find_asymmetric_key(secondary_trusted_keys, id, NULL, NULL, false); +#else + tkey = find_asymmetric_key(builtin_trusted_keys, id, NULL, NULL, false); +#endif + if (!IS_ERR(tkey)) + goto found; + + tkey = find_asymmetric_key(platform_trusted_keys, id, NULL, NULL, false); + + if (!IS_ERR(tkey)) + goto found; + + return -1; + +found: + key_link(keyring, tkey); + return 0; +} diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h index 8365adf842ef..b47ac8e2001a 100644 --- a/include/keys/system_keyring.h +++ b/include/keys/system_keyring.h @@ -9,6 +9,7 @@ #define _KEYS_SYSTEM_KEYRING_H #include <linux/key.h> +struct asymmetric_key_id; enum blacklist_hash_type { /* TBSCertificate hash */ @@ -28,7 +29,7 @@ int restrict_link_by_digsig_builtin(struct key *dest_keyring, const union key_payload *payload, struct key *restriction_key); extern __init int load_module_cert(struct key *keyring); - +extern int system_key_link(struct key *keyring, struct asymmetric_key_id *id); #else #define restrict_link_by_builtin_trusted restrict_link_reject #define restrict_link_by_digsig_builtin restrict_link_reject @@ -38,6 +39,10 @@ static inline __init int load_module_cert(struct key *keyring) return 0; } +static inline int system_key_link(struct key *keyring, struct asymmetric_key_id *id) +{ + return 0; +} #endif #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
Introduce a new function to allow a keyring to link to a key contained within one of the system keyrings (builtin, secondary, or platform). Depending on how the kernel is built, if the machine keyring is available, it will be checked as well, since it is linked to the secondary keyring. If the asymmetric key id matches a key within one of these system keyrings, the matching key is linked into the passed in keyring. Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com> --- certs/system_keyring.c | 29 +++++++++++++++++++++++++++++ include/keys/system_keyring.h | 7 ++++++- 2 files changed, 35 insertions(+), 1 deletion(-)