[v9,01/19] x86/boot: Place kernel_info at a fixed offset

Commit Message

Ross Philipson May 31, 2024, 1:03 a.m. UTC

From: Arvind Sankar <[e-mail]>

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.

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
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.

Signed-off-by: Arvind Sankar <[e-mail]>
Cc: Ross Philipson <[e-mail]>
Signed-off-by: Ross Philipson <[e-mail]>
---
 arch/x86/boot/compressed/kernel_info.S | 19 +++++++++++++++----
 arch/x86/boot/compressed/kernel_info.h | 12 ++++++++++++
 arch/x86/boot/compressed/vmlinux.lds.S |  6 ++++++
 3 files changed, 33 insertions(+), 4 deletions(-)
 create mode 100644 arch/x86/boot/compressed/kernel_info.h

Comments

Jarkko Sakkinen June 4, 2024, 6:18 p.m. UTC | #1

On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote:
> From: Arvind Sankar <[e-mail]>
>
> 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

Ross Philipson June 4, 2024, 8:28 p.m. UTC | #2

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 <[e-mail]>
>>
>> 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

Patch

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)
 	}