| Message ID | 20240531010331.134441-2-ross.philipson@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | x86: Trenchboot secure dynamic launch Linux kernel support | expand |
On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote: > From: Arvind Sankar <nivedita@alum.mit.edu> > > There are use cases for storing the offset of a symbol in kernel_info. > For example, the trenchboot series [0] needs to store the offset of the > Measured Launch Environment header in kernel_info. So either there are other use cases that you should enumerate, or just be straight and state that this is done for Trenchboot. I believe latter is the case, and there is no reason to project further. If it does not interfere kernel otherwise, it should be fine just by that. Also I believe that it is written as Trenchboot, without "series" ;-) Think when writing commit message that it will some day be part of the commit log, not a series flying in the air. Sorry for the nitpicks but better to be punctual and that way also transparent as possible, right? > > Since commit (note: commit ID from tip/master) > > commit 527afc212231 ("x86/boot: Check that there are no run-time relocations") > > run-time relocations are not allowed in the compressed kernel, so simply > using the symbol in kernel_info, as > > .long symbol > > will cause a linker error because this is not position-independent. > > With kernel_info being a separate object file and in a different section > from startup_32, there is no way to calculate the offset of a symbol > from the start of the image in a position-independent way. > > To enable such use cases, put kernel_info into its own section which is "To allow Trenchboot to access the fields of kernel_info..." Much more understandable. > placed at a predetermined offset (KERNEL_INFO_OFFSET) via the linker > script. This will allow calculating the symbol offset in a > position-independent way, by adding the offset from the start of > kernel_info to KERNEL_INFO_OFFSET. > > Ensure that kernel_info is aligned, and use the SYM_DATA.* macros > instead of bare labels. This stores the size of the kernel_info > structure in the ELF symbol table. Aligned to which boundary and short explanation why to that boundary, i.e. state the obvious if you bring it up anyway here. Just seems to be progressing pretty well so taking my eye glass and looking into nitty gritty details... BR, Jarkko
On 6/4/24 11:18 AM, Jarkko Sakkinen wrote: > On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote: >> From: Arvind Sankar <nivedita@alum.mit.edu> >> >> There are use cases for storing the offset of a symbol in kernel_info. >> For example, the trenchboot series [0] needs to store the offset of the >> Measured Launch Environment header in kernel_info. > > So either there are other use cases that you should enumerate, or just > be straight and state that this is done for Trenchboot. The kernel_info concept came about because of the work we were doing on TrenchBoot but it was not done for TrenchBoot. It was a collaborative effort between the TrenchBoot team and H. Peter Anvin at Intel. He actually envisioned it being useful elsewhere. If you find the original commits for it (that went in stand-alone) from Daniel Kiper, there is a fair amount of detail what kernel_info is supposed to be and should be used for. > > I believe latter is the case, and there is no reason to project further. > If it does not interfere kernel otherwise, it should be fine just by > that. > > Also I believe that it is written as Trenchboot, without "series" ;-) > Think when writing commit message that it will some day be part of the > commit log, not a series flying in the air. > > Sorry for the nitpicks but better to be punctual and that way also > transparent as possible, right? No problem. We submit the patch sets to get feedback :) Thanks for the feedback. > >> >> Since commit (note: commit ID from tip/master) >> >> commit 527afc212231 ("x86/boot: Check that there are no run-time relocations") >> >> run-time relocations are not allowed in the compressed kernel, so simply >> using the symbol in kernel_info, as >> >> .long symbol >> >> will cause a linker error because this is not position-independent. >> >> With kernel_info being a separate object file and in a different section >> from startup_32, there is no way to calculate the offset of a symbol >> from the start of the image in a position-independent way. >> >> To enable such use cases, put kernel_info into its own section which is > > "To allow Trenchboot to access the fields of kernel_info..." > > Much more understandable. > >> placed at a predetermined offset (KERNEL_INFO_OFFSET) via the linker >> script. This will allow calculating the symbol offset in a >> position-independent way, by adding the offset from the start of >> kernel_info to KERNEL_INFO_OFFSET. >> >> Ensure that kernel_info is aligned, and use the SYM_DATA.* macros >> instead of bare labels. This stores the size of the kernel_info >> structure in the ELF symbol table. > > Aligned to which boundary and short explanation why to that boundary, > i.e. state the obvious if you bring it up anyway here. > > Just seems to be progressing pretty well so taking my eye glass and > looking into nitty gritty details... So a lot of this is up in the air if you read the responses between us and Ard Biesheuvel. It would be nice to get rid of the part where kernel_info is forced to a fixed offset in the setup kernel. Thanks Ross > > BR, Jarkko
diff --git a/arch/x86/boot/compressed/kernel_info.S b/arch/x86/boot/compressed/kernel_info.S index f818ee8fba38..c18f07181dd5 100644 --- a/arch/x86/boot/compressed/kernel_info.S +++ b/arch/x86/boot/compressed/kernel_info.S @@ -1,12 +1,23 @@ /* SPDX-License-Identifier: GPL-2.0 */ +#include <linux/linkage.h> #include <asm/bootparam.h> +#include "kernel_info.h" - .section ".rodata.kernel_info", "a" +/* + * If a field needs to hold the offset of a symbol from the start + * of the image, use the macro below, eg + * .long rva(symbol) + * This will avoid creating run-time relocations, which are not + * allowed in the compressed kernel. + */ + +#define rva(X) (((X) - kernel_info) + KERNEL_INFO_OFFSET) - .global kernel_info + .section ".rodata.kernel_info", "a" -kernel_info: + .balign 16 +SYM_DATA_START(kernel_info) /* Header, Linux top (structure). */ .ascii "LToP" /* Size. */ @@ -19,4 +30,4 @@ kernel_info: kernel_info_var_len_data: /* Empty for time being... */ -kernel_info_end: +SYM_DATA_END_LABEL(kernel_info, SYM_L_LOCAL, kernel_info_end) diff --git a/arch/x86/boot/compressed/kernel_info.h b/arch/x86/boot/compressed/kernel_info.h new file mode 100644 index 000000000000..c127f84aec63 --- /dev/null +++ b/arch/x86/boot/compressed/kernel_info.h @@ -0,0 +1,12 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef BOOT_COMPRESSED_KERNEL_INFO_H +#define BOOT_COMPRESSED_KERNEL_INFO_H + +#ifdef CONFIG_X86_64 +#define KERNEL_INFO_OFFSET 0x500 +#else /* 32-bit */ +#define KERNEL_INFO_OFFSET 0x100 +#endif + +#endif /* BOOT_COMPRESSED_KERNEL_INFO_H */ diff --git a/arch/x86/boot/compressed/vmlinux.lds.S b/arch/x86/boot/compressed/vmlinux.lds.S index 083ec6d7722a..718c52f3f1e6 100644 --- a/arch/x86/boot/compressed/vmlinux.lds.S +++ b/arch/x86/boot/compressed/vmlinux.lds.S @@ -7,6 +7,7 @@ OUTPUT_FORMAT(CONFIG_OUTPUT_FORMAT) #include <asm/cache.h> #include <asm/page_types.h> +#include "kernel_info.h" #ifdef CONFIG_X86_64 OUTPUT_ARCH(i386:x86-64) @@ -27,6 +28,11 @@ SECTIONS HEAD_TEXT _ehead = . ; } + .rodata.kernel_info KERNEL_INFO_OFFSET : { + *(.rodata.kernel_info) + } + ASSERT(ABSOLUTE(kernel_info) == KERNEL_INFO_OFFSET, "kernel_info at bad address!") + .rodata..compressed : { *(.rodata..compressed) }