Message-ID: <ZK6hLZcuAH9jXKuL@redhat.com>
Date:   Wed, 12 Jul 2023 13:48:45 +0100
From:   Daniel P. Berrangé <berrange@...hat.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Emanuele Giuseppe Esposito <eesposit@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        Thomas Gleixner <tglx@...utronix.de>, bluca@...ian.org,
        lennart@...ttering.net, Ingo Molnar <mingo@...hat.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Alexander Potapenko <glider@...gle.com>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v2] x86/boot: add .sbat section to the bzImage

On Wed, Jul 12, 2023 at 02:00:02PM +0200, Borislav Petkov wrote:
> On Wed, Jul 12, 2023 at 08:19:32AM +0200, Emanuele Giuseppe Esposito wrote:
> > And any comment on the SBAT string itself? I would like to get an
> > agreement on
> > "linux,1,The Linux Developers,linux,$(KERNELVERSION),https://linux.org"
> > before we use it as semplate also for downstream.
> 
> Yeah, looks useless to me. With your patch I get:
> 
> $ cat linux.sbat
> sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
> linux,1,The Linux Developers,linux,6.5.0-rc1,https://linux.org
> 
> But my branch is:
> 
> $ git describe
> v6.5-rc1-6-g3f01e9fed845

That doesn't make it useless, as the 3rd/4th/5th fields in the SBAT
file are just human targetted metadata. The validation process just
works off the 1st/2nd field.

> So your thing needs to enable CONFIG_LOCALVERSION_AUTO or so which
> allows for uniquely identifying the build. At least I think it did at
> some point. So that you can do stuff like:
> 
> $ file vmlinux
> vmlinux: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), statically linked, BuildID[sha1]=19f22ac85675ea30042fb703373d97c460bb5a61, with debug_info, not stripped
> 
> and you have a build ID sha there.
> 
> But not even that works because if I change the source, I still get the
> same sha. So it needs to be an mechanism which identifies the kernel
> image uniquely.

>From a functional POV, it doesn't have to be unique identified,
as it is just a human targetted metadata field. A friendly git
version as from 'git describe' is more appropriate than a build
ID sha.

> And then why does it have to be a separate section? All those
> requirements need to be written down.
>
> And regardless what you do, this looks like a contract between the
> kernel and userspace tools so it absolutely needs to be documented
> somewhere prominently - not in a commit message with links to flaky URLs
> which would probably change in the future - and explained what the tools
> parse and where one can find that parsing code.

More precisely this is a contract between 'shim' and any other
EFI binary that is intended to be validated by 'shim' during EFI
boot, with SecureBoot enabled. Normally 'shim' would be loading
a bootloader like 'grub', but with unified kernel images (vmlinuz+
cmdline+initrd bundled in one EFI binary), there's a desire to
load the kernels directly from shim without an intermediate
bootloader. IIUC, the sbat info against the kernel would actually
be relevant even if grub is loading the kernel, as grub would still
call back into shim todo validation of the binary for secureboot
compliance.

The shim project has defined this format, and the linked git repo
provided URL is the canonical location for where this is documented.

The first doc gives the background and design approach

  https://github.com/rhboot/shim/blob/main/SBAT.md

The second docs gives an example of how revocation happens

  https://github.com/rhboot/shim/blob/main/SBAT.example.md

shim is also where you'll find the parsing code, though it is
not especially interesting, since this is just a CSV file

  https://github.com/rhboot/shim/blob/main/csv.c

called from

  https://github.com/rhboot/shim/blob/main/sbat.c

> Because if we go and change that, we need to be able to verify whether
> we're not breaking any userspace tools.
>
> Actually, I wouldn't mind even having a small script which does the
> parsing and which we can use to check that we're not breaking things.

The parsing isn't the interesting bit - any trivial CSV parsing
can validate, the semantics of each field are more important.

The first line just identifies the file format and should
never change:

  sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md

The second line identifies the kernel generation

  linux,1,The Linux Developers,linux,6.5.0-rc1,https://linux.org

The first field 'linux' should never change once decided upon, as it is
the name of the upstream project's EFI component - in this case the
linux kernel.

The second field '1' is the most important one, as it is the mechanism
through which revokation takes places, and the only one a human upstream
maintainer should manually change.

If there is discovered a flaw in Linux that allows the Secure Boot chain
to be broken (eg some flaw allowed linux to be exploited as a mechanism
to load an unsigned binary), then this 'generation' number would need
to be incremented when a fix is provided in upstream Linux trees.

The SBAT config for shim would be updated to say 'linux,2' was the new
baseline, at which point it would refuse to load any binaries that still
had 'linux,1' in their sbat PE section.

When a downstream vendor builds the kernel they would actually add a
third record, where they append a vendor identifier to the 'linux'
component name, so the .sbat PE section might say.

 $ cat linux.sbat
 sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
 linux,1,The Linux Developers,linux,6.5.0-rc1,https://linux.org
 linux.fedora,1,The Fedora Project,linux,6.5.0-rc1,https://fedoraproject.org

this allows Fedora to deal with revokation if they make a downstream
only mistake that compromises SecureBoot.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Powered by blists - more mailing lists