Re: [XEN PATCH] sbat: Add SBAT section to the xen binary

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Thu, May 1, 2025 at 12:34 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 01/05/2025 11:49 am, Gerald Elder-Vass wrote:
> > The SBAT section provides a way for the binary to declare a generation
> > id for its upstream source and any vendor changes applied. A compatible
> > loader can then revoke vulnerable binaries by generation, using the
> > binary's declared generation id(s) to determine if it is safe to load.
> >
> > More information about SBAT is available here:
> > https://github.com/rhboot/shim/blob/main/SBAT.md
> >
> > Vendors should append a custom line onto sbat.csv(.in) with their vendor
> > specific sbat data.
> >
> > Populate the SBAT section in the Xen binary by using the information
> > in xen/arch/x86/sbat.csv.in
> >
> > Signed-off-by: Gerald Elder-Vass <gerald.elder-vass@xxxxxxxxx>
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
> > Tested-by: Gerald Elder-Vass <gerald.elder-vass@xxxxxxxxx>
>
> Thankyou for starting to post these patches.
>
> The commit message needs that SBAT is a revocation scheme for UEFI
> SecureBoot, and mandatory now if you want to get signed by Microsoft.
> This wants to be the first sentence, IMO.
>
> That in turn also explains why it's in the EFI binary only, and
> discarded from the ELF binary.
>
> > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> > index d902fb7accd9..6db7475c2c23 100644
> > --- a/xen/arch/x86/Makefile
> > +++ b/xen/arch/x86/Makefile
> > @@ -74,6 +74,7 @@ obj-$(CONFIG_TBOOT) += tboot.o
> >  obj-y += hpet.o
> >  obj-y += vm_event.o
> >  obj-y += xstate.o
> > +obj-y += sbat_data.o
>
> These should be sorted by file name (although hpet.o is clearly out of
> order here).
>
> Where possible, please use dash rather than underscore in filenames,
> although in this case I'd shorten it to just sbat.o and bypass that problem.
>
> >
> >  ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y)
> >  obj-y += domctl.o
> > @@ -277,6 +278,12 @@ $(obj)/efi.lds: AFLAGS-y += -DEFI
> >  $(obj)/xen.lds $(obj)/efi.lds: $(src)/xen.lds.S FORCE
> >       $(call if_changed_dep,cpp_lds_S)
> >
> > +$(obj)/sbat.csv: $(src)/sbat.csv.in
> > +     sed "s/@@VERSION@@/${XEN_FULLVERSION}/" $< > $@
> > +
> > +$(obj)/sbat_data.o: $(obj)/sbat.csv
> > +     $(OBJCOPY) -I binary -O elf64-x86-64 --rename-section 
> > .data=.sbat,readonly,data,contents $< $@
> > +
> >  clean-files := \
> >      include/asm/asm-macros.* \
> >      $(objtree)/.xen-syms.[0-9]* \
> > diff --git a/xen/arch/x86/sbat.csv.in b/xen/arch/x86/sbat.csv.in
> > new file mode 100644
> > index 000000000000..7cdc33dbd998
> > --- /dev/null
> > +++ b/xen/arch/x86/sbat.csv.in
> > @@ -0,0 +1,2 @@
> > +sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
> > +xen,1,Linux Foundation,xen,@@VERSION@@,https://xenproject.org/
>
> I know this is what the SBAT spec says to do, but it's unworkable.
>
> Upstream Xen cannot state or maintain a global generation ID on behalf
> of it's downstreams.  This is true in general, not just for Xen.
>
> For us (XenServer), this needs to be a line starting xen.xenserver,
> because we (and only we) know how our Xen is built and configured.
> Every other downstream will need to do the same.
>
> So, either we want just the SBAT line an nothing else, or we want some
> kind of "to be filled in by the OSV" info, to make it clear that people
> need to alter it.
>

At this point why not make the inclusion of this section conditional?
If the binary is not going to be signed this section won't be used.
For instance I would define a dummy variable at the beginning of
xen/Makefile like XEN_SBAT_NAME.
If it's not provided (people can use external xen-version file) do not
include that section.

> When UEFI SecureBoot becomes security supported, the security team
> probably wants to note in XSAs whether the issue constitutes a breach of
> UEFI-SB, and remind downstreams to bump their generation IDs.
>
> ~Andrew

Frediano