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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > > ---
> > > 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
>