sbat: Add SBAT section to the xen binary

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

Posted by Gerald Elder-Vass 6 months ago

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@cloud.com>
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
Tested-by: Gerald Elder-Vass <gerald.elder-vass@cloud.com>
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
 
 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/
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 9a1dfe1b340a..e6405941e1b7 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -343,6 +343,8 @@ SECTIONS
     *(.reloc)
     __base_relocs_end = .;
   }
+
+  .sbat (NOLOAD) : { *(.sbat) }
 #elif defined(XEN_BUILD_EFI)
   /*
    * Due to the way EFI support is currently implemented, these two symbols
diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
index a17810bb286f..756f97d48183 100644
--- a/xen/include/xen/xen.lds.h
+++ b/xen/include/xen/xen.lds.h
@@ -92,7 +92,8 @@
        *(.comment.*) \
        *(.note.*)
 #else
-#define DISCARD_EFI_SECTIONS
+#define DISCARD_EFI_SECTIONS \
+       *(.sbat)
 #endif
 
 /* Sections to be discarded. */

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

Posted by Andrew Cooper 6 months ago

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@cloud.com>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> Tested-by: Gerald Elder-Vass <gerald.elder-vass@cloud.com>

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

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

Posted by Demi Marie Obenour 6 months ago

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@cloud.com>
>> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
>> Tested-by: Gerald Elder-Vass <gerald.elder-vass@cloud.com>
> 
> 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)

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

Posted by Frediano Ziglio 6 months ago

On Thu, May 1, 2025 at 12:34 PM Andrew Cooper <andrew.cooper3@citrix.com> 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@cloud.com>
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> > Tested-by: Gerald Elder-Vass <gerald.elder-vass@cloud.com>
>
> 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