diff mbox series

[v7,1/3] certs: export load_certificate_list() to be used outside certs/

Message ID 20220105175410.554444-2-nayna@linux.ibm.com (mailing list archive)
State New
Headers show
Series integrity: support including firmware ".platform" keys at build time | expand

Commit Message

Nayna Jain Jan. 5, 2022, 5:54 p.m. UTC 
load_certificate_list() parses certificates embedded in the kernel
image to load them onto the keyring.

Commit "2565ca7f5ec1 (certs: Move load_system_certificate_list to a common
function)" made load_certificate_list() a common function in the certs/
directory. Now, export load_certificate_list() outside certs/ to be used
by load_platform_certificate_list() which is added later in the patchset.

Reported-by: kernel test robot <lkp@intel.com> (auto build test ERROR)
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 certs/Makefile                | 5 +++--
 certs/blacklist.c             | 1 -
 certs/common.c                | 2 +-
 certs/common.h                | 9 ---------
 certs/system_keyring.c        | 1 -
 include/keys/system_keyring.h | 6 ++++++
 6 files changed, 10 insertions(+), 14 deletions(-)
 delete mode 100644 certs/common.h

Comments

Jarkko Sakkinen Jan. 8, 2022, 1:54 p.m. UTC | #1 
On Wed, Jan 05, 2022 at 12:54:08PM -0500, Nayna Jain wrote:
> load_certificate_list() parses certificates embedded in the kernel
> image to load them onto the keyring.
> 
> Commit "2565ca7f5ec1 (certs: Move load_system_certificate_list to a common
> function)" made load_certificate_list() a common function in the certs/
> directory. Now, export load_certificate_list() outside certs/ to be used
> by load_platform_certificate_list() which is added later in the patchset.
> 
> Reported-by: kernel test robot <lkp@intel.com> (auto build test ERROR)
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>

This lacking fixes tag, if it is a bug, or "reported-by" needs to be
completely removed.

/Jarkko
Jarkko Sakkinen Jan. 8, 2022, 1:55 p.m. UTC | #2 
On Wed, Jan 05, 2022 at 12:54:08PM -0500, Nayna Jain wrote:
> load_certificate_list() parses certificates embedded in the kernel
> image to load them onto the keyring.
> 
> Commit "2565ca7f5ec1 (certs: Move load_system_certificate_list to a common
> function)" made load_certificate_list() a common function in the certs/
> directory. Now, export load_certificate_list() outside certs/ to be used
> by load_platform_certificate_list() which is added later in the patchset.

Also, please describe the specific use it will be used. Patchset is not
a valid cross-reference in this context.

/Jarkko
Mimi Zohar Jan. 9, 2022, 11:49 a.m. UTC | #3 
Hi Jarkko,

On Sat, 2022-01-08 at 15:55 +0200, Jarkko Sakkinen wrote:
> On Wed, Jan 05, 2022 at 12:54:08PM -0500, Nayna Jain wrote:
> > load_certificate_list() parses certificates embedded in the kernel
> > image to load them onto the keyring.
> > 
> > Commit "2565ca7f5ec1 (certs: Move load_system_certificate_list to a common
> > function)" made load_certificate_list() a common function in the certs/
> > directory. Now, export load_certificate_list() outside certs/ to be used
> > by load_platform_certificate_list() which is added later in the patchset.
> 
> Also, please describe the specific use it will be used. Patchset is not
> a valid cross-reference in this context.

The specific usecase scenario is described in the cover letter.  Is
that what you're looking for?

thanks,

Mimi
Nayna Jan. 10, 2022, 4:12 p.m. UTC | #4 
On 1/8/22 08:54, Jarkko Sakkinen wrote:
> On Wed, Jan 05, 2022 at 12:54:08PM -0500, Nayna Jain wrote:
>> load_certificate_list() parses certificates embedded in the kernel
>> image to load them onto the keyring.
>>
>> Commit "2565ca7f5ec1 (certs: Move load_system_certificate_list to a common
>> function)" made load_certificate_list() a common function in the certs/
>> directory. Now, export load_certificate_list() outside certs/ to be used
>> by load_platform_certificate_list() which is added later in the patchset.
>>
>> Reported-by: kernel test robot <lkp@intel.com> (auto build test ERROR)
>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
>> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> This lacking fixes tag, if it is a bug, or "reported-by" needs to be
> completely removed.

When I posted my first version for this patch set, kernel test robot 
reported the build error - 
https://lore.kernel.org/linux-integrity/202109110507.ucpEmrwz-lkp@intel.com/

The Reported-by tag is because of this statement in the reported error - 
" If you fix the issue, kindly add following tag as appropriate 
Reported-by: kernel test robot <lkp@intel.com>"

Do you still think that the tag is not required ? If so, I am fine to 
remove it.

Thanks & Regards,

      - Nayna
Jarkko Sakkinen Jan. 11, 2022, 2:22 a.m. UTC | #5 
On Sun, Jan 09, 2022 at 06:49:23AM -0500, Mimi Zohar wrote:
> Hi Jarkko,
> 
> On Sat, 2022-01-08 at 15:55 +0200, Jarkko Sakkinen wrote:
> > On Wed, Jan 05, 2022 at 12:54:08PM -0500, Nayna Jain wrote:
> > > load_certificate_list() parses certificates embedded in the kernel
> > > image to load them onto the keyring.
> > > 
> > > Commit "2565ca7f5ec1 (certs: Move load_system_certificate_list to a common
> > > function)" made load_certificate_list() a common function in the certs/
> > > directory. Now, export load_certificate_list() outside certs/ to be used
> > > by load_platform_certificate_list() which is added later in the patchset.
> > 
> > Also, please describe the specific use it will be used. Patchset is not
> > a valid cross-reference in this context.
> 
> The specific usecase scenario is described in the cover letter.  Is
> that what you're looking for?

You cannot refer to "a patch set" in the long description. It's by all
practical means a dead ref after some years. The commit messages are
meant for commit log to help to understand the history of changes.
This does not do that job. Neither a cover letter helps with this.

/Jarkko
Jarkko Sakkinen Jan. 11, 2022, 2:25 a.m. UTC | #6 
On Mon, Jan 10, 2022 at 11:12:41AM -0500, Nayna wrote:
> 
> On 1/8/22 08:54, Jarkko Sakkinen wrote:
> > On Wed, Jan 05, 2022 at 12:54:08PM -0500, Nayna Jain wrote:
> > > load_certificate_list() parses certificates embedded in the kernel
> > > image to load them onto the keyring.
> > > 
> > > Commit "2565ca7f5ec1 (certs: Move load_system_certificate_list to a common
> > > function)" made load_certificate_list() a common function in the certs/
> > > directory. Now, export load_certificate_list() outside certs/ to be used
> > > by load_platform_certificate_list() which is added later in the patchset.
> > > 
> > > Reported-by: kernel test robot <lkp@intel.com> (auto build test ERROR)
> > > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > > Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> > This lacking fixes tag, if it is a bug, or "reported-by" needs to be
> > completely removed.
> 
> When I posted my first version for this patch set, kernel test robot
> reported the build error -
> https://lore.kernel.org/linux-integrity/202109110507.ucpEmrwz-lkp@intel.com/
> 
> The Reported-by tag is because of this statement in the reported error - "
> If you fix the issue, kindly add following tag as appropriate Reported-by:
> kernel test robot <lkp@intel.com>"
> 
> Do you still think that the tag is not required ? If so, I am fine to remove
> it.

It makes absolutely no sense for anything that is not triggered by the
mainline code.

/Jarkko
Jarkko Sakkinen Jan. 11, 2022, 2:25 a.m. UTC | #7 
On Tue, Jan 11, 2022 at 04:25:09AM +0200, Jarkko Sakkinen wrote:
> On Mon, Jan 10, 2022 at 11:12:41AM -0500, Nayna wrote:
> > 
> > On 1/8/22 08:54, Jarkko Sakkinen wrote:
> > > On Wed, Jan 05, 2022 at 12:54:08PM -0500, Nayna Jain wrote:
> > > > load_certificate_list() parses certificates embedded in the kernel
> > > > image to load them onto the keyring.
> > > > 
> > > > Commit "2565ca7f5ec1 (certs: Move load_system_certificate_list to a common
> > > > function)" made load_certificate_list() a common function in the certs/
> > > > directory. Now, export load_certificate_list() outside certs/ to be used
> > > > by load_platform_certificate_list() which is added later in the patchset.
> > > > 
> > > > Reported-by: kernel test robot <lkp@intel.com> (auto build test ERROR)
> > > > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > > > Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> > > This lacking fixes tag, if it is a bug, or "reported-by" needs to be
> > > completely removed.
> > 
> > When I posted my first version for this patch set, kernel test robot
> > reported the build error -
> > https://lore.kernel.org/linux-integrity/202109110507.ucpEmrwz-lkp@intel.com/
> > 
> > The Reported-by tag is because of this statement in the reported error - "
> > If you fix the issue, kindly add following tag as appropriate Reported-by:
> > kernel test robot <lkp@intel.com>"
> > 
> > Do you still think that the tag is not required ? If so, I am fine to remove
> > it.
> 
> It makes absolutely no sense for anything that is not triggered by the
> mainline code.

I.e. if it is not merged, there is no bug.

/Jarkko
diff mbox series

Patch

diff --git a/certs/Makefile b/certs/Makefile
index 279433783b10..6f26c93ff56b 100644
--- a/certs/Makefile
+++ b/certs/Makefile
@@ -3,8 +3,9 @@ 
 # Makefile for the linux kernel signature checking certificates.
 #
 
-obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o system_certificates.o common.o
-obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist.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
 obj-$(CONFIG_SYSTEM_REVOCATION_LIST) += revocation_certificates.o
 ifneq ($(CONFIG_SYSTEM_BLACKLIST_HASH_LIST),"")
 obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist_hashes.o
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"
+#include <keys/system_keyring.h>
 
 int load_certificate_list(const u8 cert_list[],
 			  const unsigned long list_size,
diff --git a/certs/common.h b/certs/common.h
deleted file mode 100644
index abdb5795936b..000000000000
--- a/certs/common.h
+++ /dev/null
@@ -1,9 +0,0 @@ 
-/* SPDX-License-Identifier: GPL-2.0-or-later */
-
-#ifndef _CERT_COMMON_H
-#define _CERT_COMMON_H
-
-int load_certificate_list(const u8 cert_list[], const unsigned long list_size,
-			  const struct key *keyring);
-
-#endif
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 692365dee2bd..d130d5a96e09 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -16,7 +16,6 @@ 
 #include <keys/asymmetric-type.h>
 #include <keys/system_keyring.h>
 #include <crypto/pkcs7.h>
-#include "common.h"
 
 static struct key *builtin_trusted_keys;
 #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index 6acd3cf13a18..d3f914d9a632 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -10,6 +10,12 @@ 
 
 #include <linux/key.h>
 
+#ifdef CONFIG_KEYS
+int load_certificate_list(const u8 cert_list[],
+			  const unsigned long list_size,
+			  const struct key *keyring);
+#endif
+
 #ifdef CONFIG_SYSTEM_TRUSTED_KEYRING
 
 extern int restrict_link_by_builtin_trusted(struct key *keyring,