Messages in this thread | | | Subject | Re: [PATCH v2] integrity: support including firmware ".platform" keys at build time | From | Nayna <> | Date | Sat, 18 Sep 2021 15:37:06 -0400 |
| |
On 9/16/21 10:46 AM, Jarkko Sakkinen wrote: > On Thu, 2021-09-16 at 08:57 -0400, Nayna Jain wrote: >> Some firmware support secureboot by embedding static keys to verify the >> Linux kernel during boot. However, these firmware do not expose an >> interface for the kernel to load firmware keys onto ".platform" keyring. >> This would prevent kernel signature verification on kexec. For those >> environments, allow firmware keys to be compiled into the kernel and >> loaded onto the ".platform" keyring. > "allow" means absolutely nothing. Just tell what your patch does, > and approach taken. Already the patch description should roughly > give idea what and why of code changes. There's nothing here. > >> Reported-by: kernel test robot <lkp@intel.com> > I don't get this reported-by here. > >> Signed-off-by: Nayna Jain <nayna@linux.ibm.com> >> --- >> >> v2: >> * Fixed the error reported by kernel test robot >> * Updated patch description based on Jarkko's feedback. >> >> certs/Makefile | 3 ++- >> certs/blacklist.c | 1 - >> certs/common.c | 2 +- >> certs/common.h | 9 ------- >> certs/system_keyring.c | 1 - >> include/keys/system_keyring.h | 3 +++ >> security/integrity/Kconfig | 10 +++++++ >> security/integrity/Makefile | 17 +++++++++++- >> security/integrity/digsig.c | 2 +- >> security/integrity/integrity.h | 6 +++++ >> .../integrity/platform_certs/platform_cert.S | 23 ++++++++++++++++ >> .../platform_certs/platform_keyring.c | 26 +++++++++++++++++++ >> 12 files changed, 88 insertions(+), 15 deletions(-) >> delete mode 100644 certs/common.h >> create mode 100644 security/integrity/platform_certs/platform_cert.S >> >> diff --git a/certs/Makefile b/certs/Makefile >> index 279433783b10..64ee37f38b85 100644 >> --- a/certs/Makefile >> +++ b/certs/Makefile >> @@ -3,7 +3,8 @@ >> # Makefile for the linux kernel signature checking certificates. >> # >> >> -obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o system_certificates.o common.o >> +obj-$(CONFIG_KEYS) += common.o >> +obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o system_certificates.o >> obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist.o common.o >> obj-$(CONFIG_SYSTEM_REVOCATION_LIST) += revocation_certificates.o >> ifneq ($(CONFIG_SYSTEM_BLACKLIST_HASH_LIST),"") >> diff --git a/certs/blacklist.c b/certs/blacklist.c >> index c9a435b15af4..b95e9b19c42f 100644 >> --- a/certs/blacklist.c >> +++ b/certs/blacklist.c >> @@ -17,7 +17,6 @@ >> #include <linux/uidgid.h> >> #include <keys/system_keyring.h> >> #include "blacklist.h" >> -#include "common.h" >> >> static struct key *blacklist_keyring; >> >> diff --git a/certs/common.c b/certs/common.c >> index 16a220887a53..41f763415a00 100644 >> --- a/certs/common.c >> +++ b/certs/common.c >> @@ -2,7 +2,7 @@ >> >> #include <linux/kernel.h> >> #include <linux/key.h> >> -#include "common.h" > Why this include is removed? > > You should include to your commit message *also* the approach > you are taking. If you export a function, you should mention > it explicitly.
Thanks Jarrko for the review.
Do you think it would be better to split this patch into two ?
Patch 1: Export load_certificate_list() to be called outside certs/
Patch 2: Add and load compiled-in certificates in ".platform" keyring.
Thanks & Regards,
- Nayna
| |