[PATCH] certs: always use secondary keyring first if possible

[PATCH] certs: always use secondary keyring first if possible

[Dave Young]

Commit d3bfe84129f6 introduced secondary_trusted_keys keyring, current
users of verify_pkcs7_signature are below:
net/wireless/reg.c : uses its own trusted_keys
kernel/module_signing.c : pass NULL trusted_keys 
crypto/asymmetric_keys/verify_pefile.c : pass NULL trusted_keys

For both module and pefile verification, there is no reason to use builtin
keys only. Actually in Fedora kernel module signing code passes 1UL, but
kexec code does not pass 1UL for pefile verification thus we have below bug
https://bugzilla.redhat.com/show_bug.cgi?id=1470995

Drop the hard code 1UL checking so that pefile verification can use
secondary keyring as well.

Signed-off-by: Dave Young <dyoung@redhat.com>
---
 certs/system_keyring.c |    2 --
 1 file changed, 2 deletions(-)

--- linux-x86.orig/certs/system_keyring.c
+++ linux-x86/certs/system_keyring.c
@@ -229,8 +229,6 @@ int verify_pkcs7_signature(const void *d
 		goto error;
 
 	if (!trusted_keys) {
-		trusted_keys = builtin_trusted_keys;
-	} else if (trusted_keys == (void *)1UL) {
 #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
 		trusted_keys = secondary_trusted_keys;
 #else

Re: [PATCH] certs: always use secondary keyring first if possible

[Dave Young]

On 11/18/17 at 12:47pm, Dave Young wrote:
> Commit d3bfe84129f6 introduced secondary_trusted_keys keyring, current
> users of verify_pkcs7_signature are below:
> net/wireless/reg.c : uses its own trusted_keys
> kernel/module_signing.c : pass NULL trusted_keys 
> crypto/asymmetric_keys/verify_pefile.c : pass NULL trusted_keys
> 
> For both module and pefile verification, there is no reason to use builtin
> keys only. Actually in Fedora kernel module signing code passes 1UL, but
> kexec code does not pass 1UL for pefile verification thus we have below bug
> https://bugzilla.redhat.com/show_bug.cgi?id=1470995
> 
> Drop the hard code 1UL checking so that pefile verification can use
> secondary keyring as well.
> 
> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
>  certs/system_keyring.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> --- linux-x86.orig/certs/system_keyring.c
> +++ linux-x86/certs/system_keyring.c
> @@ -229,8 +229,6 @@ int verify_pkcs7_signature(const void *d
>  		goto error;
>  
>  	if (!trusted_keys) {
> -		trusted_keys = builtin_trusted_keys;
> -	} else if (trusted_keys == (void *)1UL) {
>  #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
>  		trusted_keys = secondary_trusted_keys;
>  #else

Ping, can anyone review this?

Thanks
Dave

Re: [PATCH] certs: always use secondary keyring first if possible

[Dave Young]

On 11/18/17 at 12:47pm, Dave Young wrote:
> Commit d3bfe84129f6 introduced secondary_trusted_keys keyring, current
> users of verify_pkcs7_signature are below:
> net/wireless/reg.c : uses its own trusted_keys
> kernel/module_signing.c : pass NULL trusted_keys 
> crypto/asymmetric_keys/verify_pefile.c : pass NULL trusted_keys
> 
> For both module and pefile verification, there is no reason to use builtin
> keys only. Actually in Fedora kernel module signing code passes 1UL, but
> kexec code does not pass 1UL for pefile verification thus we have below bug
> https://bugzilla.redhat.com/show_bug.cgi?id=1470995
> 
> Drop the hard code 1UL checking so that pefile verification can use
> secondary keyring as well.
> 
> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
>  certs/system_keyring.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> --- linux-x86.orig/certs/system_keyring.c
> +++ linux-x86/certs/system_keyring.c
> @@ -229,8 +229,6 @@ int verify_pkcs7_signature(const void *d
>  		goto error;
>  
>  	if (!trusted_keys) {
> -		trusted_keys = builtin_trusted_keys;
> -	} else if (trusted_keys == (void *)1UL) {
>  #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
>  		trusted_keys = secondary_trusted_keys;
>  #else

Another ping.

If the (-1UL) is really needed, below file need update to use it
But I think it is ugly..
crypto/asymmetric_keys/verify_pefile.c

Thanks
Dave

Re: [PATCH] certs: always use secondary keyring first if possible

[Dave Young]

On 12/14/17 at 06:25pm, Dave Young wrote:
> On 11/18/17 at 12:47pm, Dave Young wrote:
> > Commit d3bfe84129f6 introduced secondary_trusted_keys keyring, current
> > users of verify_pkcs7_signature are below:
> > net/wireless/reg.c : uses its own trusted_keys
> > kernel/module_signing.c : pass NULL trusted_keys 
> > crypto/asymmetric_keys/verify_pefile.c : pass NULL trusted_keys
> > 
> > For both module and pefile verification, there is no reason to use builtin
> > keys only. Actually in Fedora kernel module signing code passes 1UL, but
> > kexec code does not pass 1UL for pefile verification thus we have below bug
> > https://bugzilla.redhat.com/show_bug.cgi?id=1470995
> > 
> > Drop the hard code 1UL checking so that pefile verification can use
> > secondary keyring as well.
> > 
> > Signed-off-by: Dave Young <dyoung@redhat.com>
> > ---
> >  certs/system_keyring.c |    2 --
> >  1 file changed, 2 deletions(-)
> > 
> > --- linux-x86.orig/certs/system_keyring.c
> > +++ linux-x86/certs/system_keyring.c
> > @@ -229,8 +229,6 @@ int verify_pkcs7_signature(const void *d
> >  		goto error;
> >  
> >  	if (!trusted_keys) {
> > -		trusted_keys = builtin_trusted_keys;
> > -	} else if (trusted_keys == (void *)1UL) {
> >  #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> >  		trusted_keys = secondary_trusted_keys;
> >  #else
> 
> Another ping.
> 
> If the (-1UL) is really needed, below file need update to use it
> But I think it is ugly..
> crypto/asymmetric_keys/verify_pefile.c

Ping again.  Can anyone response to this issue?

Let me describe more details about the problem:

In Fedora kernel there is a patch below which is not upstreamed:
https://src.fedoraproject.org/rpms/kernel/blob/master/f/Fix-for-module-sig-verification.patch

It has below changes:

---
diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index 937c844..d3d6f95 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -81,6 +81,6 @@ int mod_verify_sig(const void *mod, unsigned long *_modlen)
 	}

 	return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
-				      NULL, VERIFYING_MODULE_SIGNATURE,
+				      (void *)1UL, VERIFYING_MODULE_SIGNATURE,
 				      NULL, NULL);
 }
---

Above change is needed because the verify_pkcs7_signature is doing
below:
---
        if (!trusted_keys) {
                trusted_keys = builtin_trusted_keys;
        } else if (trusted_keys == (void *)1UL) {
#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
                trusted_keys = secondary_trusted_keys;
#else
                trusted_keys = builtin_trusted_keys;
#endif
        }
---

The trusted_keys is an argument passed to verify_pkcs7_signature
function. We can see that users of this function must pass "-1UL"
as trusted_keys to use secondary keyring.  This "-1UL" is not
documented and it looks a hardcode api.   Besides of the module
signing code, actually as I mentioned in the patch log kexec/kdump
also need passing "-1UL" to use the secondary keyring.

But why do we need that hack?  If I understand it correctly
if use secondary then builtin can still be used, see commit log
of d3bfe84129f65e0af2450743ebdab33d161d01c9:
    If the secondary keyring is enabled, a link is created from that to
    .builtin_trusted_keys so that the the latter will automatically be searched
    too if the secondary keyring is searched.

So why not directly use secondary in case trusted_keys == NULL?

I'm not familar with the certs/keyring code, if I'm wrong please
correct me.

--
Thanks
Dave

Re: [PATCH] certs: always use secondary keyring first if possible

[Dave Young]

On 06/08/18 at 03:28pm, Dave Young wrote:
> On 12/14/17 at 06:25pm, Dave Young wrote:
> > On 11/18/17 at 12:47pm, Dave Young wrote:
> > > Commit d3bfe84129f6 introduced secondary_trusted_keys keyring, current
> > > users of verify_pkcs7_signature are below:
> > > net/wireless/reg.c : uses its own trusted_keys
> > > kernel/module_signing.c : pass NULL trusted_keys 
> > > crypto/asymmetric_keys/verify_pefile.c : pass NULL trusted_keys
> > > 
> > > For both module and pefile verification, there is no reason to use builtin
> > > keys only. Actually in Fedora kernel module signing code passes 1UL, but
> > > kexec code does not pass 1UL for pefile verification thus we have below bug
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1470995
> > > 
> > > Drop the hard code 1UL checking so that pefile verification can use
> > > secondary keyring as well.
> > > 
> > > Signed-off-by: Dave Young <dyoung@redhat.com>
> > > ---
> > >  certs/system_keyring.c |    2 --
> > >  1 file changed, 2 deletions(-)
> > > 
> > > --- linux-x86.orig/certs/system_keyring.c
> > > +++ linux-x86/certs/system_keyring.c
> > > @@ -229,8 +229,6 @@ int verify_pkcs7_signature(const void *d
> > >  		goto error;
> > >  
> > >  	if (!trusted_keys) {
> > > -		trusted_keys = builtin_trusted_keys;
> > > -	} else if (trusted_keys == (void *)1UL) {
> > >  #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> > >  		trusted_keys = secondary_trusted_keys;
> > >  #else
> > 
> > Another ping.
> > 
> > If the (-1UL) is really needed, below file need update to use it
> > But I think it is ugly..
> > crypto/asymmetric_keys/verify_pefile.c
> 
> Ping again.  Can anyone response to this issue?
> 
> Let me describe more details about the problem:
> 
> In Fedora kernel there is a patch below which is not upstreamed:
> https://src.fedoraproject.org/rpms/kernel/blob/master/f/Fix-for-module-sig-verification.patch
> 
> It has below changes:
> 
> ---
> diff --git a/kernel/module_signing.c b/kernel/module_signing.c
> index 937c844..d3d6f95 100644
> --- a/kernel/module_signing.c
> +++ b/kernel/module_signing.c
> @@ -81,6 +81,6 @@ int mod_verify_sig(const void *mod, unsigned long *_modlen)
>  	}
> 
>  	return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
> -				      NULL, VERIFYING_MODULE_SIGNATURE,
> +				      (void *)1UL, VERIFYING_MODULE_SIGNATURE,
>  				      NULL, NULL);
>  }
> ---
> 
> Above change is needed because the verify_pkcs7_signature is doing
> below:
> ---
>         if (!trusted_keys) {
>                 trusted_keys = builtin_trusted_keys;
>         } else if (trusted_keys == (void *)1UL) {
> #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
>                 trusted_keys = secondary_trusted_keys;
> #else
>                 trusted_keys = builtin_trusted_keys;
> #endif
>         }
> ---
> 
> The trusted_keys is an argument passed to verify_pkcs7_signature
> function. We can see that users of this function must pass "-1UL"
> as trusted_keys to use secondary keyring.  This "-1UL" is not

I meant to say "1UL" instead of "-1UL"..

> documented and it looks a hardcode api.   Besides of the module
> signing code, actually as I mentioned in the patch log kexec/kdump
> also need passing "-1UL" to use the secondary keyring.
> 
> But why do we need that hack?  If I understand it correctly
> if use secondary then builtin can still be used, see commit log
> of d3bfe84129f65e0af2450743ebdab33d161d01c9:
>     If the secondary keyring is enabled, a link is created from that to
>     .builtin_trusted_keys so that the the latter will automatically be searched
>     too if the secondary keyring is searched.
> 
> So why not directly use secondary in case trusted_keys == NULL?
> 
> I'm not familar with the certs/keyring code, if I'm wrong please
> correct me.
> 
> --
> Thanks
> Dave
>