Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot

[Vivek Goyal]

On Wed, Aug 15, 2018 at 01:42:47PM -0400, Vivek Goyal wrote:
> On Wed, Aug 15, 2018 at 07:27:33PM +0200, Yannik Sembritzki wrote:
> > Would this be okay?
> 
> [ CC dave young, Baoquan, Justin Forbes]
> 
> Hi Yannik,
> 
> I am reading that bug and wondering that what broke it. It used to work,
> so some change broke it. 
> 
> Justin said that we have been signing fedora kernels with fedora keys so
> looks like no change there.
> 
> Previously, I think all the keys used to go in system keyring and it
> used to work. Is it somehow because of split in builtin keyring and
> secondary system keyring. Could it be that fedora key used to show
> up in system keyring previously and it worked but now it shows up
> in secondary system keyring and by default we don't use keys from
> that keyring for signature verification?

I do strongly suspect it is due to the fact that .system_keyring got
split into .builtin_trusted_keys and .secondary_trusted_keys.

commit d3bfe84129f65e0af2450743ebdab33d161d01c9
Author: David Howells <dhowells@xxxxxxxxxx>
Date:   Wed Apr 6 16:14:27 2016 +0100

    certs: Add a secondary system keyring that can be added to dynamically

int verify_pkcs7_signature(const void *data, size_t len,
        if (ret < 0)
                goto error;
 
-       if (!trusted_keys)
-               trusted_keys = system_trusted_keyring;
+       if (!trusted_keys) {
+               trusted_keys = builtin_trusted_keys;
+       } else if (trusted_keys == (void *)1UL) {


Previously we defaulted to using system_trusted_keyring, and that
contained both builtin keys and all the keys we imported from UEFI db,
MokList variable etc.

Now this patch changed it to trusting builtin_trusted_keys by default,
and all the other keys go to secondary_trusted_keys kerying. And that
probably explains why it broke.

So checking for keys in both the keyrings makes sense to me. 

I am wondering why did we have to split this keyring to begin with. 
So there are use cases where we want to trust builtin keys but
not the ones which came from other places (UEFI secure boot db, or
user loaded one)?

I am finding code in upstream kernel to add keys to secondary
keyring. I do see extra patches we are carrying in fedora which
add keys to secondary keyring from various sources.

Thinking loud to make sure if it safe to trust keys in .secondary_trusted_keys
keyring and there are no gotchas. I don't know secureboot and UEFI
parts that well. I have copied Peter Jones, James Bottomley and Matthew Garret
to cc as well, just to see if they can think of any issues in trusting
keys from .secondary_trusted_keys keyring for kernel signature
verification.

Thanks
Vivek

> 
> > 
> > diff --git a/arch/x86/kernel/kexec-bzimage64.c
> > b/arch/x86/kernel/kexec-bzimage64.c
> > index 7326078e..2ba47e24 100644
> > --- a/arch/x86/kernel/kexec-bzimage64.c
> > +++ b/arch/x86/kernel/kexec-bzimage64.c
> > @@ -41,6 +41,9 @@
> >  #define MIN_KERNEL_LOAD_ADDR   0x100000
> >  #define MIN_INITRD_LOAD_ADDR   0x1000000
> >  
> > +// Allow both builtin trusted keys and secondary trusted keys
> > +#define TRUST_FULL_KEYRING     (void *)1UL
> > +
> >  /*
> >   * This is a place holder for all boot loader specific data structure which
> >   * gets allocated in one call but gets freed much later during cleanup
> > @@ -532,7 +535,7 @@ static int bzImage64_cleanup(void *loader_data)
> >  static int bzImage64_verify_sig(const char *kernel, unsigned long
> > kernel_len)
> >  {
> >         return verify_pefile_signature(kernel, kernel_len,
> > -                                      NULL,
> > +                                      TRUST_FULL_KEYRING,
> >                                        VERIFYING_KEXEC_PE_SIGNATURE);
> >  }
> >  #endif
> > --
> > 
> > On 15.08.2018 18:54, Linus Torvalds wrote:
> > > This needs more people involved, and at least a sign-off.
> > >
> > > It looks ok, but I think we need a #define for the magical (void *)1UL
> > > thing. I see the use in verify_pkcs7_signature(), but still.
> > >
> > >               Linus
> > >
> > >
> > >
> > > On Wed, Aug 15, 2018 at 3:11 AM Yannik Sembritzki <yannik@xxxxxxxxxxxxx> wrote:
> > >> ---
> > >>  arch/x86/kernel/kexec-bzimage64.c | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> > >> index 7326078e..eaaa125d 100644
> > >> --- a/arch/x86/kernel/kexec-bzimage64.c
> > >> +++ b/arch/x86/kernel/kexec-bzimage64.c
> > >> @@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
> > >>  static int bzImage64_verify_sig(const char *kernel, unsigned long kernel_len)
> > >>  {
> > >>         return verify_pefile_signature(kernel, kernel_len,
> > >> -                                      NULL,
> > >> +                                      (void *)1UL,
> > >>                                        VERIFYING_KEXEC_PE_SIGNATURE);
> > >>  }
> > >>  #endif
> > >> --
> > >> 2.17.1
> > >>
> > >> The exact scenario under which this issue occurs is described here:
> > >> https://bugzilla.redhat.com/show_bug.cgi?id=1554113
> > >>
> >