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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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