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

[Nayna <nayna@linux.vnet.ibm.com>]

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