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

[Demi Marie Obenour <demiobenour@xxxxxxxxx>]

On 5/1/25 7:34 AM, Andrew Cooper 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.
> 
> 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.

What about having both?

One of the goals of SBAT is to keep the size of revocations under control.
That requires as many downstreams as possible to share an SBAT section entry
so that a single revocation can be used for all of them.  If everyone uses
a different SBAT entry, does SBAT provide any functionality beyond meeting
Microsoft requirements?
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)

Attachment: OpenPGP_0xB288B55FFF9C22C1.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature