| Message ID | 20190109164824.19708-3-kasong@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | let kexec_file_load use platform keyring to verify the kernel image | expand |
On 01/10/19 at 12:48am, Kairui Song wrote: > kexec_file_load will need to verify the kernel signed with third part > keys, and the keys could be stored in firmware, then got loaded into > the .platform keyring. Now we have a .platform_trusted_keyring > as the reference to .platform keyring, this patch makes use if it and > allow kexec_file_load to verify the image against keys in .platform > keyring. > > 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> > --- > arch/x86/kernel/kexec-bzimage64.c | 13 ++++++++++--- > certs/system_keyring.c | 7 ++++++- > include/linux/verification.h | 1 + > 3 files changed, 17 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c > index 7d97e432cbbc..a8a5c1773ccc 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) { > + 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 a61b95390b80..7514e69e719f 100644 > --- a/certs/system_keyring.c > +++ b/certs/system_keyring.c > @@ -18,6 +18,7 @@ > #include <linux/verification.h> > #include <keys/asymmetric-type.h> > #include <keys/system_keyring.h> > +#include <keys/platform_keyring.h> > #include <crypto/pkcs7.h> > > static struct key *builtin_trusted_keys; > @@ -239,12 +240,16 @@ int verify_pkcs7_signature(const void *data, size_t len, > trusted_keys = secondary_trusted_keys; > #else > trusted_keys = builtin_trusted_keys; > +#endif > +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING > + } else if (trusted_keys == VERIFY_USE_PLATFORM_KEYRING) { > + trusted_keys = platform_trusted_keys; > #endif > } > 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 > Personally I would like to see platform key separated from integrity. But for the kexec_file part I think it is good at least it works with this fix. Acked-by: Dave Young <dyoung@redhat.com> Thanks Dave
On Fri, 2019-01-11 at 21:43 +0800, Dave Young wrote: [snip] > Personally I would like to see platform key separated from integrity. > But for the kexec_file part I think it is good at least it works with > this fix. > > Acked-by: Dave Young <dyoung@redhat.com> The original "platform" keyring patches that Nayna posted multiple times were in the certs directory, but nobody commented/responded. So she reworked the patches, moving them to the integrity directory and posted them (cc'ing the kexec mailing list). It's a bit late to be asking to move it, isn't it? Mimi
Hi, On 01/11/19 at 11:13am, Mimi Zohar wrote: > On Fri, 2019-01-11 at 21:43 +0800, Dave Young wrote: > [snip] > > > Personally I would like to see platform key separated from integrity. > > But for the kexec_file part I think it is good at least it works with > > this fix. > > > > Acked-by: Dave Young <dyoung@redhat.com> > > The original "platform" keyring patches that Nayna posted multiple > times were in the certs directory, but nobody commented/responded. So > she reworked the patches, moving them to the integrity directory and > posted them (cc'ing the kexec mailing list). It's a bit late to be > asking to move it, isn't it? Hmm, apologize for being late, I did not get chance to have a look the old series. Since we have the needs now, it should be still fine Maybe Kairui can check Nayna's old series, see if he can do something again? > > Mimi > Thanks Dave
Hi, Mimi, Dave I checked the previous patches: https://www.spinics.net/lists/keyrings/msg03518.html https://www.spinics.net/lists/keyrings/msg03517.html https://www.spinics.net/lists/keyrings/msg03516.html That the latest patched I could found that placed the platform keyring in certs/ However it didn't cc kexec list, and so I think Dave didn't receive them. I could compose a patch to use the previous design, how do you think? On Sun, Jan 13, 2019 at 9:40 AM Dave Young <dyoung@redhat.com> wrote: > > Hi, > > On 01/11/19 at 11:13am, Mimi Zohar wrote: > > On Fri, 2019-01-11 at 21:43 +0800, Dave Young wrote: > > [snip] > > > > > Personally I would like to see platform key separated from integrity. > > > But for the kexec_file part I think it is good at least it works with > > > this fix. > > > > > > Acked-by: Dave Young <dyoung@redhat.com> > > > > The original "platform" keyring patches that Nayna posted multiple > > times were in the certs directory, but nobody commented/responded. So > > she reworked the patches, moving them to the integrity directory and > > posted them (cc'ing the kexec mailing list). It's a bit late to be > > asking to move it, isn't it? > > Hmm, apologize for being late, I did not get chance to have a look the > old series. Since we have the needs now, it should be still fine > > Maybe Kairui can check Nayna's old series, see if he can do something > again? > > > > > Mimi > > > > Thanks > Dave -- Best Regards, Kairui Song
On Sun, 2019-01-13 at 09:39 +0800, Dave Young wrote: > Hi, > > On 01/11/19 at 11:13am, Mimi Zohar wrote: > > On Fri, 2019-01-11 at 21:43 +0800, Dave Young wrote: > > [snip] > > > > > Personally I would like to see platform key separated from integrity. > > > But for the kexec_file part I think it is good at least it works with > > > this fix. > > > > > > Acked-by: Dave Young <dyoung@redhat.com> > > > > The original "platform" keyring patches that Nayna posted multiple > > times were in the certs directory, but nobody commented/responded. So > > she reworked the patches, moving them to the integrity directory and > > posted them (cc'ing the kexec mailing list). It's a bit late to be > > asking to move it, isn't it? > > Hmm, apologize for being late, I did not get chance to have a look the > old series. Since we have the needs now, it should be still fine > > Maybe Kairui can check Nayna's old series, see if he can do something > again? Whether the platform keyring is defined in certs/ or in integrity/ the keyring id needs to be accessible to the other, without making the keyring id global. Moving where the platform keyring is defined is not the problem. Commit a210fd32a46b ("kexec: add call to LSM hook in original kexec_load syscall") introduced a new LSM hook. Assuming CONFIG_KEXEC_VERIFY_SIG is enabled, with commit b5ca117365d9 ("ima: prevent kexec_load syscall based on runtime secureboot flag") we can now block the kexec_load syscall. Without being able to block the kexec_load syscall, verifying the kexec image signature via the kexec_file_load syscall is kind of pointless. Unless you're planning on writing an LSM to prevent the kexec_load syscall, I assume you'll want to enable integrity anyway. Mimi
On 01/14/19 at 11:10am, Mimi Zohar wrote: > On Sun, 2019-01-13 at 09:39 +0800, Dave Young wrote: > > Hi, > > > > On 01/11/19 at 11:13am, Mimi Zohar wrote: > > > On Fri, 2019-01-11 at 21:43 +0800, Dave Young wrote: > > > [snip] > > > > > > > Personally I would like to see platform key separated from integrity. > > > > But for the kexec_file part I think it is good at least it works with > > > > this fix. > > > > > > > > Acked-by: Dave Young <dyoung@redhat.com> > > > > > > The original "platform" keyring patches that Nayna posted multiple > > > times were in the certs directory, but nobody commented/responded. So > > > she reworked the patches, moving them to the integrity directory and > > > posted them (cc'ing the kexec mailing list). It's a bit late to be > > > asking to move it, isn't it? > > > > Hmm, apologize for being late, I did not get chance to have a look the > > old series. Since we have the needs now, it should be still fine > > > > Maybe Kairui can check Nayna's old series, see if he can do something > > again? > > Whether the platform keyring is defined in certs/ or in integrity/ the > keyring id needs to be accessible to the other, without making the > keyring id global. Moving where the platform keyring is defined is > not the problem. Agreed, but just feel kexec depends on IMA sounds not good. > > Commit a210fd32a46b ("kexec: add call to LSM hook in original > kexec_load syscall") introduced a new LSM hook. Assuming > CONFIG_KEXEC_VERIFY_SIG is enabled, with commit b5ca117365d9 ("ima: > prevent kexec_load syscall based on runtime secureboot flag") we can > now block the kexec_load syscall. Without being able to block the > kexec_load syscall, verifying the kexec image signature via the > kexec_file_load syscall is kind of pointless. > > Unless you're planning on writing an LSM to prevent the kexec_load > syscall, I assume you'll want to enable integrity anyway. User can disable kexec_load in kernel config, and only allow kexec_file_load. But yes, this can be improved separately in case no IMA enabled. For the time being we can leave with it and fix like this series do. > > Mimi > Thanks Dave
On Tue, Jan 15, 2019 at 10:42 AM Dave Young <dyoung@redhat.com> wrote: > > On 01/14/19 at 11:10am, Mimi Zohar wrote: > > On Sun, 2019-01-13 at 09:39 +0800, Dave Young wrote: > > > Hi, > > > > > > On 01/11/19 at 11:13am, Mimi Zohar wrote: > > > > On Fri, 2019-01-11 at 21:43 +0800, Dave Young wrote: > > > > [snip] > > > > > > > > > Personally I would like to see platform key separated from integrity. > > > > > But for the kexec_file part I think it is good at least it works with > > > > > this fix. > > > > > > > > > > Acked-by: Dave Young <dyoung@redhat.com> > > > > > > > > The original "platform" keyring patches that Nayna posted multiple > > > > times were in the certs directory, but nobody commented/responded. So > > > > she reworked the patches, moving them to the integrity directory and > > > > posted them (cc'ing the kexec mailing list). It's a bit late to be > > > > asking to move it, isn't it? > > > > > > Hmm, apologize for being late, I did not get chance to have a look the > > > old series. Since we have the needs now, it should be still fine > > > > > > Maybe Kairui can check Nayna's old series, see if he can do something > > > again? > > > > Whether the platform keyring is defined in certs/ or in integrity/ the > > keyring id needs to be accessible to the other, without making the > > keyring id global. Moving where the platform keyring is defined is > > not the problem. > > Agreed, but just feel kexec depends on IMA sounds not good. > > > > > Commit a210fd32a46b ("kexec: add call to LSM hook in original > > kexec_load syscall") introduced a new LSM hook. Assuming > > CONFIG_KEXEC_VERIFY_SIG is enabled, with commit b5ca117365d9 ("ima: > > prevent kexec_load syscall based on runtime secureboot flag") we can > > now block the kexec_load syscall. Without being able to block the > > kexec_load syscall, verifying the kexec image signature via the > > kexec_file_load syscall is kind of pointless. > > > > Unless you're planning on writing an LSM to prevent the kexec_load > > syscall, I assume you'll want to enable integrity anyway. > > User can disable kexec_load in kernel config, and only allow > kexec_file_load. But yes, this can be improved separately in case no > IMA enabled. > > For the time being we can leave with it and fix like this series do. > > > > > Mimi > > > > Thanks > Dave Yes, for now, I think it's good to fix the problem by following this patch series and get kexec_file_load work with platform keyring first. Will adopt suggestion from Mimi in the previous reply and update the patch series. For other remaining potential issues, kexec_load not being protected, it could be disabled by config, and the improvement may require more discussion. And issues like where the keyring is located, dependency to making the keyring available for more general use could be discussed later. -- Best Regards, Kairui Song
On 2019-01-14 21:42, Dave Young wrote: > On 01/14/19 at 11:10am, Mimi Zohar wrote: >> On Sun, 2019-01-13 at 09:39 +0800, Dave Young wrote: >> > Hi, >> > >> > On 01/11/19 at 11:13am, Mimi Zohar wrote: >> > > On Fri, 2019-01-11 at 21:43 +0800, Dave Young wrote: >> > > [snip] >> > > >> > > > Personally I would like to see platform key separated from integrity. >> > > > But for the kexec_file part I think it is good at least it works with >> > > > this fix. >> > > > >> > > > Acked-by: Dave Young <dyoung@redhat.com> >> > > >> > > The original "platform" keyring patches that Nayna posted multiple >> > > times were in the certs directory, but nobody commented/responded. So >> > > she reworked the patches, moving them to the integrity directory and >> > > posted them (cc'ing the kexec mailing list). It's a bit late to be >> > > asking to move it, isn't it? >> > >> > Hmm, apologize for being late, I did not get chance to have a look the >> > old series. Since we have the needs now, it should be still fine >> > >> > Maybe Kairui can check Nayna's old series, see if he can do something >> > again? >> >> Whether the platform keyring is defined in certs/ or in integrity/ the >> keyring id needs to be accessible to the other, without making the >> keyring id global. Moving where the platform keyring is defined is >> not the problem. > > Agreed, but just feel kexec depends on IMA sounds not good. The platform keyring is not dependent on IMA, it is dependent on "integrity" - CONFIG_INTEGRITY_ASYMMETRIC_KEYS. Other CONFIGS which it needs are CONFIG_SYSTEM_BLACKLIST_KEYRING, CONFIG_EFI. Thanks & Regards, - Nayna
diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c index 7d97e432cbbc..a8a5c1773ccc 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) { + 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 a61b95390b80..7514e69e719f 100644 --- a/certs/system_keyring.c +++ b/certs/system_keyring.c @@ -18,6 +18,7 @@ #include <linux/verification.h> #include <keys/asymmetric-type.h> #include <keys/system_keyring.h> +#include <keys/platform_keyring.h> #include <crypto/pkcs7.h> static struct key *builtin_trusted_keys; @@ -239,12 +240,16 @@ int verify_pkcs7_signature(const void *data, size_t len, trusted_keys = secondary_trusted_keys; #else trusted_keys = builtin_trusted_keys; +#endif +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING + } else if (trusted_keys == VERIFY_USE_PLATFORM_KEYRING) { + trusted_keys = platform_trusted_keys; #endif } 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.
kexec_file_load will need to verify the kernel signed with third part keys, and the keys could be stored in firmware, then got loaded into the .platform keyring. Now we have a .platform_trusted_keyring as the reference to .platform keyring, this patch makes use if it and allow kexec_file_load to verify the image against keys in .platform keyring. 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> --- arch/x86/kernel/kexec-bzimage64.c | 13 ++++++++++--- certs/system_keyring.c | 7 ++++++- include/linux/verification.h | 1 + 3 files changed, 17 insertions(+), 4 deletions(-)