Re: [PATCH v2] integrity: support including firmware ".platform" keys at build time

[Nayna]

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@xxxxxxxxx>
> I don't get this reported-by here.
>
> > Signed-off-by: Nayna Jain <nayna@xxxxxxxxxxxxx>
> > ---
> >
> > 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