[PATCH] certs: Fix wrong kconfig option used for x509_revocation_list

[PATCH] certs: Fix wrong kconfig option used for x509_revocation_list

[Eric Snowberg]

This fixes a build issue when x509_revocation_list is not defined.

$ make ARCH=x86_64 O=build64 all

 EXTRACT_CERTS   ../
At main.c:154:
- SSL error:0909006C:PEM routines:get_name:no start line: crypto/pem/pem_lib.c:745
extract-cert: ../: Is a directory
make[2]: [../certs/Makefile:119: certs/x509_revocation_list] Error 1 (ignored)

When the new CONFIG_SYSTEM_REVOCATION_LIST was added [1], it was not carried
into the code for preloading the revocation certificates [2].  Change from
using the original CONFIG_SYSTEM_BLACKLIST_KEYRING  to the new
CONFIG_SYSTEM_REVOCATION_LIST.

[1] https://lore.kernel.org/keyrings/EDA280F9-F72D-4181-93C7-CDBE95976FF7@oracle.com/T/#m562c1b27bf402190e7bb573ad20eff5b6310d08f
[2] https://lore.kernel.org/keyrings/EDA280F9-F72D-4181-93C7-CDBE95976FF7@oracle.com/T/#m07e258bf019ccbac23820fad5192ceffa74fc6ab

Reported-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 certs/Makefile    | 7 +++++--
 certs/blacklist.c | 4 ++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/certs/Makefile b/certs/Makefile
index e3f4926fd21e..3bc43c88a6d2 100644
--- a/certs/Makefile
+++ b/certs/Makefile
@@ -4,7 +4,10 @@
 #
 
 obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o system_certificates.o common.o
-obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist.o revocation_certificates.o common.o
+obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist.o common.o
+ifeq ($(CONFIG_SYSTEM_REVOCATION_LIST),y)
+obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += revocation_certificates.o
+endif
 ifneq ($(CONFIG_SYSTEM_BLACKLIST_HASH_LIST),"")
 obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist_hashes.o
 else
@@ -105,7 +108,7 @@ $(obj)/signing_key.x509: scripts/extract-cert $(X509_DEP) FORCE
 	$(call if_changed,extract_certs,$(MODULE_SIG_KEY_SRCPREFIX)$(CONFIG_MODULE_SIG_KEY))
 endif # CONFIG_MODULE_SIG
 
-ifeq ($(CONFIG_SYSTEM_BLACKLIST_KEYRING),y)
+ifeq ($(CONFIG_SYSTEM_REVOCATION_LIST),y)
 
 $(eval $(call config_filename,SYSTEM_REVOCATION_KEYS))
 
diff --git a/certs/blacklist.c b/certs/blacklist.c
index 723b19c96256..c9a435b15af4 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -21,8 +21,10 @@
 
 static struct key *blacklist_keyring;
 
+#ifdef CONFIG_SYSTEM_REVOCATION_LIST
 extern __initconst const u8 revocation_certificate_list[];
 extern __initconst const unsigned long revocation_certificate_list_size;
+#endif
 
 /*
  * The description must be a type prefix, a colon and then an even number of
@@ -225,6 +227,7 @@ static int __init blacklist_init(void)
  */
 device_initcall(blacklist_init);
 
+#ifdef CONFIG_SYSTEM_REVOCATION_LIST
 /*
  * Load the compiled-in list of revocation X.509 certificates.
  */
@@ -237,3 +240,4 @@ static __init int load_revocation_certificate_list(void)
 				     blacklist_keyring);
 }
 late_initcall(load_revocation_certificate_list);
+#endif
-- 
2.18.4

Re: [PATCH] certs: Fix wrong kconfig option used for x509_revocation_list

[David Howells]

Eric Snowberg <eric.snowberg@oracle.com> wrote:

> +ifeq ($(CONFIG_SYSTEM_REVOCATION_LIST),y)
> +obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += revocation_certificates.o
> +endif

Should the ifeq be referring to CONFIG_SYSTEM_REVOCATION_KEYS rather than
CONFIG_SYSTEM_REVOCATION_LIST?  In fact, since S_R_K depends indirectly on
S_B_K, you should be able to just do:

	+obj-$(CONFIG_SYSTEM_REVOCATION_KEYS) += revocation_certificates.o

> +#ifdef CONFIG_SYSTEM_REVOCATION_LIST

Here also?

> + hostprogs-always-$(CONFIG_SYSTEM_BLACKLIST_KEYRING)   += extract-cert

And here too?

(As an aside, I wonder if SYSTEM_REVOCATION_CERTS would be a better name, but
I'm okay with leaving it as-is for now).

David

Re: [PATCH] certs: Fix wrong kconfig option used for x509_revocation_list

[Eric Snowberg]

> On Mar 3, 2021, at 2:24 AM, David Howells <dhowells@redhat.com> wrote:
> 
> Eric Snowberg <eric.snowberg@oracle.com> wrote:
> 
>> +ifeq ($(CONFIG_SYSTEM_REVOCATION_LIST),y)
>> +obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += revocation_certificates.o
>> +endif
> 
> Should the ifeq be referring to CONFIG_SYSTEM_REVOCATION_KEYS rather than
> CONFIG_SYSTEM_REVOCATION_LIST?  In fact, since S_R_K depends indirectly on
> S_B_K, you should be able to just do:
> 
> 	+obj-$(CONFIG_SYSTEM_REVOCATION_KEYS) += revocation_certificates.o

Since S_R_K is a string, I could not get that to work.  I could get this
to work:

obj-$(CONFIG_SYSTEM_REVOCATION_LIST) += revocation_certificates.o

If there is another way of doing this with S_R_K instead, let me know.

>> +#ifdef CONFIG_SYSTEM_REVOCATION_LIST
> 
> Here also?

When S_R_L is defined, S_R_K will also always be defined too.  Either as an
empty string or a path to a file.  With my change, it works the same as the 
current code in CONFIG_SYSTEM_TRUSTED_KEYS and CONFIG_SYSTEM_TRUSTED_KEYRING,
which also uses the extract_certs script.  It can properly handle a NULL 
string. If I changed it to S_R_K here, it seems confusing to me, since one 
might assume it is only defined when someone adds a string to S_R_K.  But, 
I can change it if you’d like.


>> + hostprogs-always-$(CONFIG_SYSTEM_BLACKLIST_KEYRING)   += extract-cert
> 
> And here too?

Re: [PATCH] certs: Fix wrong kconfig option used for x509_revocation_list

[David Howells]

Eric Snowberg <eric.snowberg@oracle.com> wrote:

> Since S_R_K is a string, I could not get that to work.

Okay.

> >> + hostprogs-always-$(CONFIG_SYSTEM_BLACKLIST_KEYRING)   += extract-cert

I think this should then refer to CONFIG_SYSTEM_REVOCATION_LIST.

David

Re: [PATCH] certs: Fix wrong kconfig option used for x509_revocation_list

[Jarkko Sakkinen]

On Tue, Mar 02, 2021 at 10:44:18PM -0500, Eric Snowberg wrote:
> This fixes a build issue when x509_revocation_list is not defined.

"Fix a"

Let's stick to the imperative form in commit messages.

> 
> $ make ARCH=x86_64 O=build64 all
> 
>  EXTRACT_CERTS   ../
> At main.c:154:
> - SSL error:0909006C:PEM routines:get_name:no start line: crypto/pem/pem_lib.c:745
> extract-cert: ../: Is a directory
> make[2]: [../certs/Makefile:119: certs/x509_revocation_list] Error 1 (ignored)
> 
> When the new CONFIG_SYSTEM_REVOCATION_LIST was added [1], it was not carried
> into the code for preloading the revocation certificates [2].  Change from
> using the original CONFIG_SYSTEM_BLACKLIST_KEYRING  to the new
> CONFIG_SYSTEM_REVOCATION_LIST.
> 
> [1] https://lore.kernel.org/keyrings/EDA280F9-F72D-4181-93C7-CDBE95976FF7@oracle.com/T/#m562c1b27bf402190e7bb573ad20eff5b6310d08f
> [2] https://lore.kernel.org/keyrings/EDA280F9-F72D-4181-93C7-CDBE95976FF7@oracle.com/T/#m07e258bf019ccbac23820fad5192ceffa74fc6ab
> 
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
>  certs/Makefile    | 7 +++++--
>  certs/blacklist.c | 4 ++++
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/certs/Makefile b/certs/Makefile
> index e3f4926fd21e..3bc43c88a6d2 100644
> --- a/certs/Makefile
> +++ b/certs/Makefile
> @@ -4,7 +4,10 @@
>  #
>  
>  obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o system_certificates.o common.o
> -obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist.o revocation_certificates.o common.o
> +obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist.o common.o
> +ifeq ($(CONFIG_SYSTEM_REVOCATION_LIST),y)
> +obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += revocation_certificates.o
> +endif
>  ifneq ($(CONFIG_SYSTEM_BLACKLIST_HASH_LIST),"")
>  obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist_hashes.o
>  else
> @@ -105,7 +108,7 @@ $(obj)/signing_key.x509: scripts/extract-cert $(X509_DEP) FORCE
>  	$(call if_changed,extract_certs,$(MODULE_SIG_KEY_SRCPREFIX)$(CONFIG_MODULE_SIG_KEY))
>  endif # CONFIG_MODULE_SIG
>  
> -ifeq ($(CONFIG_SYSTEM_BLACKLIST_KEYRING),y)
> +ifeq ($(CONFIG_SYSTEM_REVOCATION_LIST),y)
>  
>  $(eval $(call config_filename,SYSTEM_REVOCATION_KEYS))
>  
> diff --git a/certs/blacklist.c b/certs/blacklist.c
> index 723b19c96256..c9a435b15af4 100644
> --- a/certs/blacklist.c
> +++ b/certs/blacklist.c
> @@ -21,8 +21,10 @@
>  
>  static struct key *blacklist_keyring;
>  
> +#ifdef CONFIG_SYSTEM_REVOCATION_LIST
>  extern __initconst const u8 revocation_certificate_list[];
>  extern __initconst const unsigned long revocation_certificate_list_size;
> +#endif
>  
>  /*
>   * The description must be a type prefix, a colon and then an even number of
> @@ -225,6 +227,7 @@ static int __init blacklist_init(void)
>   */
>  device_initcall(blacklist_init);
>  
> +#ifdef CONFIG_SYSTEM_REVOCATION_LIST
>  /*
>   * Load the compiled-in list of revocation X.509 certificates.
>   */
> @@ -237,3 +240,4 @@ static __init int load_revocation_certificate_list(void)
>  				     blacklist_keyring);
>  }
>  late_initcall(load_revocation_certificate_list);
> +#endif
> -- 
> 2.18.4
> 
> 

Code change looks good.

/Jarkko