From 58c2e99012bff25367af5ffa269e077fa4a14182 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 18 Aug 2025 09:22:34 +0200 Subject: [PATCH 1/6] fundamental/macro: adjust indentation of ifdefs --- src/fundamental/macro-fundamental.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/fundamental/macro-fundamental.h b/src/fundamental/macro-fundamental.h index 7a962df82a..a322a4587c 100644 --- a/src/fundamental/macro-fundamental.h +++ b/src/fundamental/macro-fundamental.h @@ -421,7 +421,7 @@ assert_cc(sizeof(dummy_t) == 0); /* Restriction/bug (see below) was fixed in GCC 15 and clang 19. */ #if __GNUC__ >= 15 || (defined(__clang__) && __clang_major__ >= 19) -#define DECLARE_FLEX_ARRAY(type, name) type name[] +# define DECLARE_FLEX_ARRAY(type, name) type name[] #else /* Declare a flexible array usable in a union. * This is essentially a work-around for a pointless constraint in C99 @@ -443,9 +443,9 @@ assert_cc(sizeof(dummy_t) == 0); ".popsection\n") #ifdef SBAT_DISTRO - #define DECLARE_SBAT(text) DECLARE_NOALLOC_SECTION(".sbat", text) +# define DECLARE_SBAT(text) DECLARE_NOALLOC_SECTION(".sbat", text) #else - #define DECLARE_SBAT(text) +# define DECLARE_SBAT(text) #endif #define typeof_field(struct_type, member) typeof(((struct_type *) 0)->member) From dbd7b16819ce6e0d7b9a17cb08d42c0c94a2b441 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 18 Aug 2025 21:42:10 +0200 Subject: [PATCH 2/6] ukify: extend error message when section is too small --- src/ukify/ukify.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/ukify/ukify.py b/src/ukify/ukify.py index a09ea09f9e..5cd902bd9d 100755 --- a/src/ukify/ukify.py +++ b/src/ukify/ukify.py @@ -1045,7 +1045,10 @@ def pe_add_sections(opts: UkifyConfig, uki: UKI, output: str) -> None: for i, s in enumerate(pe.sections[:n_original_sections]): if pe_strip_section_name(s.Name) == section.name and section.name != '.dtbauto': if new_section.Misc_VirtualSize > s.SizeOfRawData: - raise PEError(f'Not enough space in existing section {section.name} to append new data') + raise PEError( + f'Not enough space in existing section {section.name} to append new data' + f' (need {new_section.Misc_VirtualSize}, have {s.SizeOfRawData})' + ) padding = bytes(new_section.SizeOfRawData - new_section.Misc_VirtualSize) pe.__data__ = ( From 2ddbbb22dbaf14adc96f95950e5356b71ceb56f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 19 Aug 2025 09:25:32 +0200 Subject: [PATCH 3/6] ukify: adjust comments --- src/ukify/ukify.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ukify/ukify.py b/src/ukify/ukify.py index 5cd902bd9d..6d55abab35 100755 --- a/src/ukify/ukify.py +++ b/src/ukify/ukify.py @@ -943,10 +943,10 @@ def pe_add_sections(opts: UkifyConfig, uki: UKI, output: str) -> None: pe.FILE_HEADER.NumberOfSymbols = 0 pe.FILE_HEADER.IMAGE_FILE_LOCAL_SYMS_STRIPPED = True - # Old stubs might have been stripped, leading to unaligned raw data values, so let's fix them up here. # pylint thinks that Structure doesn't have various members that it has… # pylint: disable=no-member + # Old stubs might have been stripped, leading to unaligned raw data values, so let's fix them up here. for i, section in enumerate(pe.sections): oldp = section.PointerToRawData oldsz = section.SizeOfRawData @@ -1418,11 +1418,11 @@ def make_uki(opts: UkifyConfig) -> None: for section in opts.sections: uki.add_section(section) - # Don't add a sbat section to profile PE binaries. + # Don't add an .sbat section to profile PE binaries. if (opts.join_profiles or not opts.profile) and not opts.pcrsig: if linux is not None: - # Merge the .sbat sections from stub, kernel and parameter, so that revocation can be done on - # either. + # Merge the .sbat sections from stub, kernel, and parameter, so + # that revocation can be done on either. input_pes = [opts.stub, linux] if not opts.sbat: opts.sbat = [STUB_SBAT] From ec1d031f3de02f84beca89e2b402d085fba62be4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 19 Aug 2025 11:02:44 +0200 Subject: [PATCH 4/6] ukify: fix insertion of padding in merged sections The padding was done to expand the new section contents to the expected size of the new section. And this then would be used for the content in the existing section. The new section cannot be larger than the old section, but it can be smaller. If the new section was smaller, then we'd not write enough padding and the output file would be corrupted. This was observed in CI when the .sbat section in the stub was padded to 1k. The UKI with an .sbat section that was merged and was fairly short would hit this scenario and be corrupted. --- src/ukify/ukify.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/ukify/ukify.py b/src/ukify/ukify.py index 6d55abab35..e3c427166d 100755 --- a/src/ukify/ukify.py +++ b/src/ukify/ukify.py @@ -1050,14 +1050,13 @@ def pe_add_sections(opts: UkifyConfig, uki: UKI, output: str) -> None: f' (need {new_section.Misc_VirtualSize}, have {s.SizeOfRawData})' ) - padding = bytes(new_section.SizeOfRawData - new_section.Misc_VirtualSize) + padding = bytes(s.SizeOfRawData - new_section.Misc_VirtualSize) pe.__data__ = ( pe.__data__[: s.PointerToRawData] + data + padding + pe.__data__[pe.sections[i + 1].PointerToRawData :] ) - s.SizeOfRawData = new_section.SizeOfRawData s.Misc_VirtualSize = new_section.Misc_VirtualSize break else: From b4a1854397e4072f884dd23c5ec2da22bd2cf114 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 18 Aug 2025 09:43:26 +0200 Subject: [PATCH 5/6] sd-boot: pad .sbat section to 1k bytes Fedora's kernels now ship with a .sbat section: kernel,1,Red Hat,kernel-core,6.17.0-0.rc1.250814g0cc53520e68b.20.fc44.x86_64,mailto:secalert@redhat.com kernel.fedora,1,Red Hat,kernel-core,6.17.0-0.rc1.250814g0cc53520e68b.20.fc44.x86_64,mailto:secalert@redhat.com This pushes the combined .sbat section just over its pre-allocated size of 512 bytes: File "/usr/bin/ukify", line 1048, in pe_add_sections raise PEError(f'Not enough space in existing section {section.name} to append new data') PEError: Not enough space in existing section .sbat to append new data PE sections need to align to 512 bytes, so to make it all fit we pad the .sbat section with zeros to 1k. Various tools already should strip trailing zeros when using sbat sections, since ukify always inserts a trailing NUL. The defines are moved to sbat.h, they are used only in sd-stub and sd-boot. --- src/boot/stub.c | 2 +- src/fundamental/macro-fundamental.h | 23 ++++++++++++++--------- src/fundamental/sbat.h | 8 ++++++++ 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/boot/stub.c b/src/boot/stub.c index 77678bc796..575b4a982c 100644 --- a/src/boot/stub.c +++ b/src/boot/stub.c @@ -51,7 +51,7 @@ enum { /* magic string to find in the binary image */ DECLARE_NOALLOC_SECTION(".sdmagic", "#### LoaderInfo: systemd-stub " GIT_VERSION " ####"); -DECLARE_SBAT(SBAT_STUB_SECTION_TEXT); +DECLARE_SBAT_PADDED(SBAT_STUB_SECTION_TEXT); static char16_t* pe_section_to_str16( EFI_LOADED_IMAGE_PROTOCOL *loaded_image, diff --git a/src/fundamental/macro-fundamental.h b/src/fundamental/macro-fundamental.h index a322a4587c..2f2b3df277 100644 --- a/src/fundamental/macro-fundamental.h +++ b/src/fundamental/macro-fundamental.h @@ -436,17 +436,22 @@ assert_cc(sizeof(dummy_t) == 0); } #endif -/* Declares an ELF read-only string section that does not occupy memory at runtime. */ -#define DECLARE_NOALLOC_SECTION(name, text) \ - asm(".pushsection " name ",\"S\"\n\t" \ - ".ascii " STRINGIFY(text) "\n\t" \ +/* Declare an ELF read-only string section that does not occupy memory at runtime. */ +#define DECLARE_NOALLOC_SECTION(name, text) \ + asm(".pushsection " name ",\"S\"\n\t" \ + ".ascii " STRINGIFY(text) "\n\t" \ ".popsection\n") -#ifdef SBAT_DISTRO -# define DECLARE_SBAT(text) DECLARE_NOALLOC_SECTION(".sbat", text) -#else -# define DECLARE_SBAT(text) -#endif +/* Similar to DECLARE_NOALLOC_SECTION, but pad the section with extra 512 bytes. After taking alignment into + * account, the section has up to 1024 bytes minus the size of the original content of padding, and this + * extra space can be used to extend the contents. This is intended for the .sbat section. */ +#define DECLARE_NOALLOC_SECTION_PADDED(name, text) \ + assert_cc(STRLEN(text) <= 512); \ + asm(".pushsection " name ",\"S\"\n\t" \ + ".ascii " STRINGIFY(text) "\n\t" \ + ".balign 512\n\t" \ + ".fill 512, 1, 0\n\t" \ + ".popsection\n") #define typeof_field(struct_type, member) typeof(((struct_type *) 0)->member) #define sizeof_field(struct_type, member) sizeof(((struct_type *) 0)->member) diff --git a/src/fundamental/sbat.h b/src/fundamental/sbat.h index 9288e05812..1a6e678b35 100644 --- a/src/fundamental/sbat.h +++ b/src/fundamental/sbat.h @@ -12,3 +12,11 @@ SBAT_PROJECT "-stub" ",1,The systemd Developers," SBAT_PROJECT "," PROJECT_VERSION "," PROJECT_URL "\n" \ SBAT_PROJECT "-stub" "." SBAT_DISTRO "," STRINGIFY(SBAT_DISTRO_GENERATION) "," SBAT_DISTRO_SUMMARY "," SBAT_DISTRO_PKGNAME "," SBAT_DISTRO_VERSION "," SBAT_DISTRO_URL "\n" #endif + +#ifdef SBAT_DISTRO +# define DECLARE_SBAT(text) DECLARE_NOALLOC_SECTION(".sbat", text) +# define DECLARE_SBAT_PADDED(text) DECLARE_NOALLOC_SECTION_PADDED(".sbat", text) +#else +# define DECLARE_SBAT(text) +# define DECLARE_SBAT_PADDED(text) +#endif From ee45041da19b9db3352460f917a417a36db71782 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 19 Aug 2025 11:11:28 +0200 Subject: [PATCH 6/6] ukify: stop appending NUL to merged .sbat section Our tools will strip trailing NULs, so this is not causing harm, except for the corner case when the .sbat section is exactly of the maximum size and now it wouldn't fit because we're one byte short. But it also is not needed. Sections have an encoded size of the data and the reader must use that and must be prepared to handle a text section that does not end in NUL. Appending of the NUL was added in c3f7501c4d014482b17988d5aed1d88127a50b6e, without any discussion of this change. Since we didn't insert the NUL before, the tools must have been prepared to work without it. I think it's better to keep the code clean and not do this unnecessary step. --- src/ukify/ukify.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/ukify/ukify.py b/src/ukify/ukify.py index e3c427166d..c034befd85 100755 --- a/src/ukify/ukify.py +++ b/src/ukify/ukify.py @@ -1150,10 +1150,12 @@ def merge_sbat(input_pe: list[Path], input_text: list[str]) -> str: continue sbat += split[1:] - return ( - 'sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md\n' - + '\n'.join(sbat) - + '\n\x00' + return '\n'.join( + ( + 'sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md', + *sbat, + '', # an empty line so that we end up with a newline at the end + ) )