In the non-EFI boot path the ACPI RSDP table is currently found via
either EBDA or by searching through low memory for the RSDP magic.
This requires the RSDP to be located in the first 1MB of physical
memory. Xen PVH guests, however, get the RSDP address via the start of
day information block.
In order to support an arbitrary RSDP address this patch series adds
the physical address of the RSDP to the boot params structure filled
by the boot loader.
Juergen Gross (3):
x86/xen: fix boot loader version reported for pvh guests
x86/boot: add acpi rsdp address to setup_header
x86/acpi: take rsdp address for boot params if available
Documentation/x86/boot.txt | 32 +++++++++++++++++++++++++++++++-
arch/x86/boot/header.S | 6 +++++-
arch/x86/include/asm/acpi.h | 7 +++++++
arch/x86/include/asm/x86_init.h | 2 ++
arch/x86/include/uapi/asm/bootparam.h | 4 ++++
arch/x86/kernel/acpi/boot.c | 6 ++++++
arch/x86/kernel/head32.c | 1 +
arch/x86/kernel/head64.c | 2 ++
arch/x86/kernel/setup.c | 17 +++++++++++++++++
arch/x86/kernel/x86_init.c | 3 +--
arch/x86/xen/enlighten_pvh.c | 2 +-
11 files changed, 77 insertions(+), 5 deletions(-)
--
2.16.4
The boot loader version reported via sysfs is wrong in case of the
kernel being booted via the Xen PVH boot entry. it should be 2.12
(0x020c), but it is reported to be 2.18 (0x0212).
As the current way to set the version is error prone use the more
readable variant (2 << 8) | 12.
Cc: <[email protected]> # 4.12
Signed-off-by: Juergen Gross <[email protected]>
---
arch/x86/xen/enlighten_pvh.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/xen/enlighten_pvh.c b/arch/x86/xen/enlighten_pvh.c
index c85d1a88f476..f7f77023288a 100644
--- a/arch/x86/xen/enlighten_pvh.c
+++ b/arch/x86/xen/enlighten_pvh.c
@@ -75,7 +75,7 @@ static void __init init_pvh_bootparams(void)
* Version 2.12 supports Xen entry point but we will use default x86/PC
* environment (i.e. hardware_subarch 0).
*/
- pvh_bootparams.hdr.version = 0x212;
+ pvh_bootparams.hdr.version = (2 << 8) | 12;
pvh_bootparams.hdr.type_of_loader = (9 << 4) | 0; /* Xen loader */
x86_init.acpi.get_root_pointer = pvh_get_root_pointer;
--
2.16.4
Xen PVH guests receive the address of the RSDP table from Xen. In order
to support booting a Xen PVH guest via Grub2 using the standard x86
boot entry we need a way for Grub2 to pass the RSDP address to the
kernel.
For this purpose expand the struct setup_header to hold the physical
address of the RSDP address. Being zero means it isn't specified and
has to be located the legacy way (searching through low memory or
EBDA).
While documenting the new setup_header layout and protocol version
2.14 add the missing documentation of protocol version 2.13.
There are grub2 versions in several distros with a downstream patch
violating the boot protocol by writing past the end of setup_header.
This requires another update of the boot protocol to enable the kernel
to distinguish between a specified RSDP address and one filled with
garbage by such a broken grub2.
From protocol 2.14 on grub2 will write the version it is supporting
(but never a higher value than found to be supported by the kernel)
ored with 0x8000 to the version field of setup_header. This enables
the kernel to know up to which field grub2 has written information
to. All fields after that are supposed to be clobbered.
Signed-off-by: Juergen Gross <[email protected]>
---
V3: fix commit message and some documentation bits (Ingo Molnar)
V4: add version feedback from bootloader
---
Documentation/x86/boot.txt | 32 +++++++++++++++++++++++++++++++-
arch/x86/boot/header.S | 6 +++++-
arch/x86/include/asm/x86_init.h | 2 ++
arch/x86/include/uapi/asm/bootparam.h | 4 ++++
arch/x86/kernel/head32.c | 1 +
arch/x86/kernel/head64.c | 2 ++
arch/x86/kernel/setup.c | 17 +++++++++++++++++
7 files changed, 62 insertions(+), 2 deletions(-)
diff --git a/Documentation/x86/boot.txt b/Documentation/x86/boot.txt
index 5e9b826b5f62..c8f8d7e7a60c 100644
--- a/Documentation/x86/boot.txt
+++ b/Documentation/x86/boot.txt
@@ -61,6 +61,18 @@ Protocol 2.12: (Kernel 3.8) Added the xloadflags field and extension fields
to struct boot_params for loading bzImage and ramdisk
above 4G in 64bit.
+Protocol 2.13: (Kernel 3.14) Support 32- and 64-bit flags being set in
+ xloadflags to support booting a 64-bit kernel from 32-bit
+ EFI
+
+Protocol 2.14: (Kernel 4.20) Added acpi_rsdp_addr holding the physical
+ address of the ACPI RSDP table.
+ The bootloader updates version with:
+ 0x8000 | min(kernel-version, bootloader-version)
+ kernel-version being the protocol version supported by
+ the kernel and bootloader-version the protocol version
+ supported by the bootloader.
+
**** MEMORY LAYOUT
The traditional memory map for the kernel loader, used for Image or
@@ -197,6 +209,7 @@ Offset Proto Name Meaning
0258/8 2.10+ pref_address Preferred loading address
0260/4 2.10+ init_size Linear memory required during initialization
0264/4 2.11+ handover_offset Offset of handover entry point
+0268/8 2.14+ acpi_rsdp_addr Physical address of RSDP table
(1) For backwards compatibility, if the setup_sects field contains 0, the
real value is 4.
@@ -309,7 +322,7 @@ Protocol: 2.00+
Contains the magic number "HdrS" (0x53726448).
Field name: version
-Type: read
+Type: modify
Offset/size: 0x206/2
Protocol: 2.00+
@@ -317,6 +330,12 @@ Protocol: 2.00+
e.g. 0x0204 for version 2.04, and 0x0a11 for a hypothetical version
10.17.
+ Up to protocol version 2.13 this information is only read by the
+ bootloader. From protocol version 2.14 onwards the bootloader will
+ write the used protocol version ored with 0x8000 to the field. The
+ used protocol version will be the minimum of the supported protocol
+ versions of the bootloader and the kernel.
+
Field name: realmode_swtch
Type: modify (optional)
Offset/size: 0x208/4
@@ -744,6 +763,17 @@ Offset/size: 0x264/4
See EFI HANDOVER PROTOCOL below for more details.
+Field name: acpi_rsdp_addr
+Type: write
+Offset/size: 0x268/8
+Protocol: 2.14+
+
+ This field can be set by the boot loader to tell the kernel the
+ physical address of the ACPI RSDP table.
+
+ A value of 0 indicates the kernel should fall back to the standard
+ methods to locate the RSDP.
+
**** THE IMAGE CHECKSUM
diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index 850b8762e889..4c881c850125 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -300,7 +300,7 @@ _start:
# Part 2 of the header, from the old setup.S
.ascii "HdrS" # header signature
- .word 0x020d # header version number (>= 0x0105)
+ .word 0x020e # header version number (>= 0x0105)
# or else old loadlin-1.5 will fail)
.globl realmode_swtch
realmode_swtch: .word 0, 0 # default_switch, SETUPSEG
@@ -558,6 +558,10 @@ pref_address: .quad LOAD_PHYSICAL_ADDR # preferred load addr
init_size: .long INIT_SIZE # kernel initialization size
handover_offset: .long 0 # Filled in by build.c
+acpi_rsdp_addr: .quad 0 # 64-bit physical pointer to the
+ # ACPI RSDP table, added with
+ # version 2.14
+
# End of setup header #####################################################
.section ".entrytext", "ax"
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index b85a7c54c6a1..0f842104862c 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -303,4 +303,6 @@ extern void x86_init_noop(void);
extern void x86_init_uint_noop(unsigned int unused);
extern bool x86_pnpbios_disabled(void);
+void x86_verify_bootdata_version(void);
+
#endif
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index a06cbf019744..22f89d040ddd 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -16,6 +16,9 @@
#define RAMDISK_PROMPT_FLAG 0x8000
#define RAMDISK_LOAD_FLAG 0x4000
+/* version flags */
+#define VERSION_WRITTEN 0x8000
+
/* loadflags */
#define LOADED_HIGH (1<<0)
#define KASLR_FLAG (1<<1)
@@ -86,6 +89,7 @@ struct setup_header {
__u64 pref_address;
__u32 init_size;
__u32 handover_offset;
+ __u64 acpi_rsdp_addr;
} __attribute__((packed));
struct sys_desc_table {
diff --git a/arch/x86/kernel/head32.c b/arch/x86/kernel/head32.c
index ec6fefbfd3c0..76fa3b836598 100644
--- a/arch/x86/kernel/head32.c
+++ b/arch/x86/kernel/head32.c
@@ -37,6 +37,7 @@ asmlinkage __visible void __init i386_start_kernel(void)
cr4_init_shadow();
sanitize_boot_params(&boot_params);
+ x86_verify_bootdata_version();
x86_early_init_platform_quirks();
diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index ddee1f0870c4..5dc377dc9d7b 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -457,6 +457,8 @@ void __init x86_64_start_reservations(char *real_mode_data)
if (!boot_params.hdr.version)
copy_bootdata(__va(real_mode_data));
+ x86_verify_bootdata_version();
+
x86_early_init_platform_quirks();
switch (boot_params.hdr.hardware_subarch) {
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index b4866badb235..20321710efb4 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1281,6 +1281,23 @@ void __init setup_arch(char **cmdline_p)
unwind_init();
}
+/*
+ * From boot protocol 2.14 onwards we expect the bootloader to set the
+ * version to 0x8000 | <used version>. In case we find a version >= 2.14
+ * without the 0x8000 we assume the boot loader supports 2.13 only and
+ * reset the version accordingly. The 0x8000 flag is removed in any case.
+ */
+void __init x86_verify_bootdata_version(void)
+{
+ if (boot_params.hdr.version & VERSION_WRITTEN)
+ boot_params.hdr.version &= ~VERSION_WRITTEN;
+ else if (boot_params.hdr.version >= 0x020e)
+ boot_params.hdr.version = 0x020d;
+
+ if (boot_params.hdr.version < 0x020e)
+ boot_params.hdr.acpi_rsdp_addr = 0;
+}
+
#ifdef CONFIG_X86_32
static struct resource video_ram_resource = {
--
2.16.4
In case the rsdp address in struct boot_params is specified don't try
to find the table by searching, but take the address directly as set
by the boot loader.
Signed-off-by: Juergen Gross <[email protected]>
---
V3: use a generic retrieval function with a __weak annotated default
function (Ingo Molnar)
V4: check boot params version before using acpi_rsdp_addr
use x86_init structure instead of __weak
---
arch/x86/include/asm/acpi.h | 7 +++++++
arch/x86/kernel/acpi/boot.c | 6 ++++++
arch/x86/kernel/x86_init.c | 3 +--
3 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
index a303d7b7d763..2f01eb4d6208 100644
--- a/arch/x86/include/asm/acpi.h
+++ b/arch/x86/include/asm/acpi.h
@@ -142,6 +142,8 @@ static inline u64 acpi_arch_get_root_pointer(void)
void acpi_generic_reduced_hw_init(void);
+u64 x86_default_get_root_pointer(void);
+
#else /* !CONFIG_ACPI */
#define acpi_lapic 0
@@ -153,6 +155,11 @@ static inline void disable_acpi(void) { }
static inline void acpi_generic_reduced_hw_init(void) { }
+static inline u64 x86_default_get_root_pointer(void)
+{
+ return 0;
+}
+
#endif /* !CONFIG_ACPI */
#define ARCH_HAS_POWER_INIT 1
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 3b20607d581b..e8fea7ffa306 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -48,6 +48,7 @@
#include <asm/mpspec.h>
#include <asm/smp.h>
#include <asm/i8259.h>
+#include <asm/setup.h>
#include "sleep.h" /* To include x86_acpi_suspend_lowlevel */
static int __initdata acpi_force = 0;
@@ -1771,3 +1772,8 @@ void __init arch_reserve_mem_area(acpi_physical_address addr, size_t size)
e820__range_add(addr, size, E820_TYPE_ACPI);
e820__update_table_print();
}
+
+u64 x86_default_get_root_pointer(void)
+{
+ return boot_params.hdr.acpi_rsdp_addr;
+}
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index 2792b5573818..50a2b492fdd6 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -31,7 +31,6 @@ static int __init iommu_init_noop(void) { return 0; }
static void iommu_shutdown_noop(void) { }
static bool __init bool_x86_init_noop(void) { return false; }
static void x86_op_int_noop(int cpu) { }
-static u64 u64_x86_init_noop(void) { return 0; }
/*
* The platform setup functions are preset with the default functions
@@ -96,7 +95,7 @@ struct x86_init_ops x86_init __initdata = {
},
.acpi = {
- .get_root_pointer = u64_x86_init_noop,
+ .get_root_pointer = x86_default_get_root_pointer,
.reduced_hw_early_init = acpi_generic_reduced_hw_init,
},
};
--
2.16.4
* Juergen Gross <[email protected]> wrote:
> In the non-EFI boot path the ACPI RSDP table is currently found via
> either EBDA or by searching through low memory for the RSDP magic.
> This requires the RSDP to be located in the first 1MB of physical
> memory. Xen PVH guests, however, get the RSDP address via the start of
> day information block.
>
> In order to support an arbitrary RSDP address this patch series adds
> the physical address of the RSDP to the boot params structure filled
> by the boot loader.
>
> Juergen Gross (3):
> x86/xen: fix boot loader version reported for pvh guests
> x86/boot: add acpi rsdp address to setup_header
> x86/acpi: take rsdp address for boot params if available
>
> Documentation/x86/boot.txt | 32 +++++++++++++++++++++++++++++++-
> arch/x86/boot/header.S | 6 +++++-
> arch/x86/include/asm/acpi.h | 7 +++++++
> arch/x86/include/asm/x86_init.h | 2 ++
> arch/x86/include/uapi/asm/bootparam.h | 4 ++++
> arch/x86/kernel/acpi/boot.c | 6 ++++++
> arch/x86/kernel/head32.c | 1 +
> arch/x86/kernel/head64.c | 2 ++
> arch/x86/kernel/setup.c | 17 +++++++++++++++++
> arch/x86/kernel/x86_init.c | 3 +--
> arch/x86/xen/enlighten_pvh.c | 2 +-
> 11 files changed, 77 insertions(+), 5 deletions(-)
I have some vague memories of an older variant of this feature breaking stuff and resulting in
me involuntarily participating in an overly long bisection session.
If that memory is right and I'm not confusing it with some other patchset, could you please
provide some more context, what that old problem was, how it was resolved, whether it is
expected to trigger on any machines, etc., to create some warm fuzzy feelings about this
patch-set and to reduce my bisectofobia? ;-)
Thanks,
Ingo
On 10/10/2018 08:23, Ingo Molnar wrote:
>
> * Juergen Gross <[email protected]> wrote:
>
>> In the non-EFI boot path the ACPI RSDP table is currently found via
>> either EBDA or by searching through low memory for the RSDP magic.
>> This requires the RSDP to be located in the first 1MB of physical
>> memory. Xen PVH guests, however, get the RSDP address via the start of
>> day information block.
>>
>> In order to support an arbitrary RSDP address this patch series adds
>> the physical address of the RSDP to the boot params structure filled
>> by the boot loader.
>>
>> Juergen Gross (3):
>> x86/xen: fix boot loader version reported for pvh guests
>> x86/boot: add acpi rsdp address to setup_header
>> x86/acpi: take rsdp address for boot params if available
>>
>> Documentation/x86/boot.txt | 32 +++++++++++++++++++++++++++++++-
>> arch/x86/boot/header.S | 6 +++++-
>> arch/x86/include/asm/acpi.h | 7 +++++++
>> arch/x86/include/asm/x86_init.h | 2 ++
>> arch/x86/include/uapi/asm/bootparam.h | 4 ++++
>> arch/x86/kernel/acpi/boot.c | 6 ++++++
>> arch/x86/kernel/head32.c | 1 +
>> arch/x86/kernel/head64.c | 2 ++
>> arch/x86/kernel/setup.c | 17 +++++++++++++++++
>> arch/x86/kernel/x86_init.c | 3 +--
>> arch/x86/xen/enlighten_pvh.c | 2 +-
>> 11 files changed, 77 insertions(+), 5 deletions(-)
>
> I have some vague memories of an older variant of this feature breaking stuff and resulting in
> me involuntarily participating in an overly long bisection session.
>
> If that memory is right and I'm not confusing it with some other patchset, could you please
> provide some more context, what that old problem was, how it was resolved, whether it is
> expected to trigger on any machines, etc., to create some warm fuzzy feelings about this
> patch-set and to reduce my bisectofobia? ;-)
You can just dive into the discussion we had back in February:
https://lore.kernel.org/lkml/[email protected]/
The scheme I have used in V5 of the series is the one you agreed to use
back then.
A quick summary of the problem you mentioned:
There are some downstream variants of grub2 with a patch breaking the
boot protocol by writing garbage past the end of setup_header. Adding
a new field at the end of setup_header (here: rsdp_address) resulted in
those grub2 variants clobbering the preset value of 0.
The solution is to let grub2 report back the used boot protocol version
with setting a flag "I am reporting back my version". The kernel now is
capable to know which fields of setup_header are known to grub2 and can
act accordingly.
The related grub2 patch series is under review right now.
Juergen
* Juergen Gross <[email protected]> wrote:
> You can just dive into the discussion we had back in February:
That was half a year and a thousand commits ago! ;-)
> https://lore.kernel.org/lkml/[email protected]/
>
> The scheme I have used in V5 of the series is the one you agreed to use
> back then.
>
> A quick summary of the problem you mentioned:
>
> There are some downstream variants of grub2 with a patch breaking the
> boot protocol by writing garbage past the end of setup_header. Adding
> a new field at the end of setup_header (here: rsdp_address) resulted in
> those grub2 variants clobbering the preset value of 0.
>
> The solution is to let grub2 report back the used boot protocol version
> with setting a flag "I am reporting back my version". The kernel now is
> capable to know which fields of setup_header are known to grub2 and can
> act accordingly.
>
> The related grub2 patch series is under review right now.
Ok, that's reassuring - that's all the context I needed.
Would it help grub2 review+integration if we applied this to -tip and staged
it there until the Grub patches are accepted, or can I consider those changes
as grub2 upstream accepted?
I'd like to help make it happen, let me know what the best route is.
Thanks,
Ingo
On 10/10/2018 09:19, Ingo Molnar wrote:
>
> * Juergen Gross <[email protected]> wrote:
>
>> You can just dive into the discussion we had back in February:
>
> That was half a year and a thousand commits ago! ;-)
Yes. :-)
>
>> https://lore.kernel.org/lkml/[email protected]/
>>
>> The scheme I have used in V5 of the series is the one you agreed to use
>> back then.
>>
>> A quick summary of the problem you mentioned:
>>
>> There are some downstream variants of grub2 with a patch breaking the
>> boot protocol by writing garbage past the end of setup_header. Adding
>> a new field at the end of setup_header (here: rsdp_address) resulted in
>> those grub2 variants clobbering the preset value of 0.
>>
>> The solution is to let grub2 report back the used boot protocol version
>> with setting a flag "I am reporting back my version". The kernel now is
>> capable to know which fields of setup_header are known to grub2 and can
>> act accordingly.
>>
>> The related grub2 patch series is under review right now.
>
> Ok, that's reassuring - that's all the context I needed.
>
> Would it help grub2 review+integration if we applied this to -tip and staged
> it there until the Grub patches are accepted, or can I consider those changes
> as grub2 upstream accepted?
Applying to -tip would really be great! I can send you a note when the
grub series has been accepted. The grub maintainer has already sent me
a note he will be looking at the patches soon.
> I'd like to help make it happen, let me know what the best route is.
I appreciate that a lot!
Thanks,
Juergen
I just noticed this patch -- I missed it because the cover message
seemed far more harmless so I didn't notice this change.
THIS PATCH IS FATALLY WRONG AND NEEDS TO BE IMMEDIATELY REVERTED BEFORE
ANYONE STARTS RELYING ON IT; IT HAS THE POTENTIAL OF BREAKING THE
BOOTLOADER PROTOCOL FOR ALL FUTURE.
It seems to be based on fundamental misconceptions about the various
data structures in the protocol, and does so in a way that completely
breaks the way the protocol is designed to work.
The protocol is specifically designed such that fields are not version
dependencies. The version number is strictly to inform the boot loader
about which capabilities the kernel has, so that the boot loader can
know if a certain data field is meaningful and/or honored.
> +Protocol 2.14: (Kernel 4.20) Added acpi_rsdp_addr holding the physical
> + address of the ACPI RSDP table.
> + The bootloader updates version with:
> + 0x8000 | min(kernel-version, bootloader-version)
> + kernel-version being the protocol version supported by
> + the kernel and bootloader-version the protocol version
> + supported by the bootloader.
[...]
> **** MEMORY LAYOUT
>
> The traditional memory map for the kernel loader, used for Image or
> @@ -197,6 +209,7 @@ Offset Proto Name Meaning
> 0258/8 2.10+ pref_address Preferred loading address
> 0260/4 2.10+ init_size Linear memory required during initialization
> 0264/4 2.11+ handover_offset Offset of handover entry point
> +0268/8 2.14+ acpi_rsdp_addr Physical address of RSDP table
NO.
That is not how struct setup_header works, nor does this belong here.
struct setup_header contains *initialized data*, and has a length byte
at offset 0x201. The bootloader is responsible for copying the full
structure into the appropriate offset (0x1f1) in struct boot_params.
The length byte isn't actually a requirement, since the maximum possible
size of this structure is 144 bytes, and the kernel will (obviously) not
look at the older fields anyway, but it is good practice. The kernel or
any other entity is free to zero out the bytes past this length pointer.
There are only 24 bytes left in this structure, and this would occupy 8
of them for no valid reason. The *only* valid reason to put a
zero-initialized field in struct setup_header is if it used by the
16-bit legacy BIOS boot, which is obviously not the case here.
This field thus belongs in struct boot_params, not struct setup_header.
>
> @@ -317,6 +330,12 @@ Protocol: 2.00+
> e.g. 0x0204 for version 2.04, and 0x0a11 for a hypothetical version
> 10.17.
>
> + Up to protocol version 2.13 this information is only read by the
> + bootloader. From protocol version 2.14 onwards the bootloader will
> + write the used protocol version ored with 0x8000 to the field. The
> + used protocol version will be the minimum of the supported protocol
> + versions of the bootloader and the kernel.
> +
Again, this is completely wrong. The version number is communication to
the bootloader, which may end up going through multiple stages.
Modifying this field breaks this invariant in a not-very-subtle way.
Fields in struct setup_header are to be initialized from the image
provided in the kernel header.
Fields in struct boot_params are to be initialized to zero.
There is a field called "sentinel" which attempts to detect broken
bootloaders which do not do this correctly; however, when enabling new
bootloaders the Right Thing to do is to make sure they adhere to the
protocol as defined, rather than pushing a new hack onto the kernel.
Thus:
1. Please revert this patch immediately, and destroy any boot loaders
which tries to implement this.
2. Add the acpi_rsdp_addr to struct boot_params.
3. DO NOT modify the boot protocol version header field. Instead
make sure that the bootloader follows the protocol and zeroes
all unknown fields in struct boot_params.
4. Possibly make the kernel panic if it notices that the boot version
header has been mucked with, in case some of these boot loaders
have already escaped into the field.
One more aspect on this patchset: when CONFIG_ACPI_TABLE_UPGRADE was
inroduced, it was seen as a security problem and disabled by default
(unlike for device tree, where feeding entries from the boot loader is
standard operating procedure.)
Thus functionally makes that possible to bypass that prohibition. Are we
giving up that battle, or should this also be conditionalized and
default to off (presumably with Xen requiring it)?
-hpa
On 09/11/2018 23:23, H. Peter Anvin wrote:
> I just noticed this patch -- I missed it because the cover message
> seemed far more harmless so I didn't notice this change.
>
> THIS PATCH IS FATALLY WRONG AND NEEDS TO BE IMMEDIATELY REVERTED BEFORE
> ANYONE STARTS RELYING ON IT; IT HAS THE POTENTIAL OF BREAKING THE
> BOOTLOADER PROTOCOL FOR ALL FUTURE.
It is already broken and this patch tries to repair it.
> It seems to be based on fundamental misconceptions about the various
> data structures in the protocol, and does so in a way that completely
> breaks the way the protocol is designed to work.
>
> The protocol is specifically designed such that fields are not version
> dependencies. The version number is strictly to inform the boot loader
> about which capabilities the kernel has, so that the boot loader can
> know if a certain data field is meaningful and/or honored.
Right. That was where I started in early 2018.
Unfortunately there are many major distros shipping boot loaders which
write crap data past the end of setup_header.
>
>> +Protocol 2.14: (Kernel 4.20) Added acpi_rsdp_addr holding the physical
>> + address of the ACPI RSDP table.
>> + The bootloader updates version with:
>> + 0x8000 | min(kernel-version, bootloader-version)
>> + kernel-version being the protocol version supported by
>> + the kernel and bootloader-version the protocol version
>> + supported by the bootloader.
>
> [...]
>
>> **** MEMORY LAYOUT
>>
>> The traditional memory map for the kernel loader, used for Image or
>> @@ -197,6 +209,7 @@ Offset Proto Name Meaning
>> 0258/8 2.10+ pref_address Preferred loading address
>> 0260/4 2.10+ init_size Linear memory required during initialization
>> 0264/4 2.11+ handover_offset Offset of handover entry point
>> +0268/8 2.14+ acpi_rsdp_addr Physical address of RSDP table
>
> NO.
>
> That is not how struct setup_header works, nor does this belong here.
>
> struct setup_header contains *initialized data*, and has a length byte
> at offset 0x201. The bootloader is responsible for copying the full
> structure into the appropriate offset (0x1f1) in struct boot_params.
Yes, but some boot loaders copy more than that clobbering initialized
kernel data (like in my case acpi_rsdp_addr).
> The length byte isn't actually a requirement, since the maximum possible
> size of this structure is 144 bytes, and the kernel will (obviously) not
> look at the older fields anyway, but it is good practice. The kernel or
> any other entity is free to zero out the bytes past this length pointer.
>
> There are only 24 bytes left in this structure, and this would occupy 8
> of them for no valid reason. The *only* valid reason to put a
> zero-initialized field in struct setup_header is if it used by the
> 16-bit legacy BIOS boot, which is obviously not the case here.
>
> This field thus belongs in struct boot_params, not struct setup_header.
Okay, I can change that. Hoping that all boot loaders really write
zeroes to that field in case they don't know it.
>> @@ -317,6 +330,12 @@ Protocol: 2.00+
>> e.g. 0x0204 for version 2.04, and 0x0a11 for a hypothetical version
>> 10.17.
>>
>> + Up to protocol version 2.13 this information is only read by the
>> + bootloader. From protocol version 2.14 onwards the bootloader will
>> + write the used protocol version ored with 0x8000 to the field. The
>> + used protocol version will be the minimum of the supported protocol
>> + versions of the bootloader and the kernel.
>> +
>
> Again, this is completely wrong. The version number is communication to
> the bootloader, which may end up going through multiple stages.
> Modifying this field breaks this invariant in a not-very-subtle way.
>
> Fields in struct setup_header are to be initialized from the image
> provided in the kernel header.
>
> Fields in struct boot_params are to be initialized to zero.
See above. grub2 in Debian, RHEL, ... doesn't do that reliably.
> There is a field called "sentinel" which attempts to detect broken
> bootloaders which do not do this correctly; however, when enabling new
> bootloaders the Right Thing to do is to make sure they adhere to the
> protocol as defined, rather than pushing a new hack onto the kernel.
>
> Thus:
>
> 1. Please revert this patch immediately, and destroy any boot loaders
> which tries to implement this.> 2. Add the acpi_rsdp_addr to struct boot_params.
> 3. DO NOT modify the boot protocol version header field. Instead
> make sure that the bootloader follows the protocol and zeroes
> all unknown fields in struct boot_params.
How can I do this for boot loaders shipped since several years?
> 4. Possibly make the kernel panic if it notices that the boot version
> header has been mucked with, in case some of these boot loaders
> have already escaped into the field.
So don't let a new kernel boot from a disk with above grub2?
I don't think so.
Juergen
>
> Unfortunately there are many major distros shipping boot loaders which
> write crap data past the end of setup_header.
>
Yes. We know that and it is resolved by:
a) the length field in setup_header;
b) the "sentinel" field which catches legacy non-compliant bootloaders.
>>
>> This field thus belongs in struct boot_params, not struct setup_header.
>
> Okay, I can change that. Hoping that all boot loaders really write
> zeroes to that field in case they don't know it.
>
This is what we added the sentinel field for: bootloaders which don't zero
unknown fields (read: Grub) will trigger the sentinel, and we wipe most of
this structure.
>>
>> Fields in struct boot_params are to be initialized to zero.
>
> See above. grub2 in Debian, RHEL, ... doesn't do that reliably.
>
See above.
>> There is a field called "sentinel" which attempts to detect broken
>> bootloaders which do not do this correctly; however, when enabling new
>> bootloaders the Right Thing to do is to make sure they adhere to the
>> protocol as defined, rather than pushing a new hack onto the kernel.
>>
>> Thus:
>>
>> 1. Please revert this patch immediately, and destroy any boot loaders
>> which tries to implement this.> 2. Add the acpi_rsdp_addr to struct boot_params.
>> 3. DO NOT modify the boot protocol version header field. Instead
>> make sure that the bootloader follows the protocol and zeroes
>> all unknown fields in struct boot_params.
>
> How can I do this for boot loaders shipped since several years?
Once again, you are adding new functionality; that is when you should fix
their implementation. The sentinel handles legacy bootloaders.
>> 4. Possibly make the kernel panic if it notices that the boot version
>> header has been mucked with, in case some of these boot loaders
>> have already escaped into the field.
>
> So don't let a new kernel boot from a disk with above grub2?
>
> I don't think so.
If there are any grubs which escaped with this broken protocol hack only.
-hpa
On 10/11/2018 07:32, H. Peter Anvin wrote:
>>
>> Unfortunately there are many major distros shipping boot loaders which
>> write crap data past the end of setup_header.
>>
>
> Yes. We know that and it is resolved by:
>
> a) the length field in setup_header;
> b) the "sentinel" field which catches legacy non-compliant bootloaders.
Doesn't help for boot loaders reading struct setup_header from the
kernel image and then writing e.g. 512 bytes back to the setup_header
location. The sentinel is cleared and the length field just isn't
taken into account. And this is what happened.
>
>>>
>>> This field thus belongs in struct boot_params, not struct setup_header.
>>
>> Okay, I can change that. Hoping that all boot loaders really write
>> zeroes to that field in case they don't know it.
>>
>
> This is what we added the sentinel field for: bootloaders which don't zero
> unknown fields (read: Grub) will trigger the sentinel, and we wipe most of
> this structure.
Unfortunately the sentinel seems to be cleared by said broken grub.
Juergen
On 11/9/18 11:02 PM, Juergen Gross wrote:
>>
>> Yes. We know that and it is resolved by:
>>
>> a) the length field in setup_header;
>> b) the "sentinel" field which catches legacy non-compliant bootloaders.
>
> Doesn't help for boot loaders reading struct setup_header from the
> kernel image and then writing e.g. 512 bytes back to the setup_header
> location. The sentinel is cleared and the length field just isn't
> taken into account. And this is what happened.
>
This is insane?! How do they manage to do this... it's not like this isn't
written out in plain English to follow. I am, once again, utterly and
genuinely baffled about how many ways Grub can do things wrong.
So we should probably add a terminal sentinel field at offset 0x281, which is
one byte past the longest possible setup_header structure; in fact, we may
just want to explicitly pad setup_header with zeroes to its final size, if
nothing else to make it explicit how little space is actually left in there.
It would be enormously helpful if you could find out any more details about
exactly what they are doing to break things.
Many thanks,
-hpa
On 10/11/2018 08:16, H. Peter Anvin wrote:
> On 11/9/18 11:02 PM, Juergen Gross wrote:
>>>
>>> Yes. We know that and it is resolved by:
>>>
>>> a) the length field in setup_header;
>>> b) the "sentinel" field which catches legacy non-compliant bootloaders.
>>
>> Doesn't help for boot loaders reading struct setup_header from the
>> kernel image and then writing e.g. 512 bytes back to the setup_header
>> location. The sentinel is cleared and the length field just isn't
>> taken into account. And this is what happened.
>>
>
> This is insane?! How do they manage to do this... it's not like this isn't
> written out in plain English to follow. I am, once again, utterly and
> genuinely baffled about how many ways Grub can do things wrong.
>
> So we should probably add a terminal sentinel field at offset 0x281, which is
> one byte past the longest possible setup_header structure; in fact, we may
> just want to explicitly pad setup_header with zeroes to its final size, if
> nothing else to make it explicit how little space is actually left in there.
How would that help? The garabge data written could have the correct
terminal sentinel value by chance.
That's why I re-used an existing field in setup_header (the version) to
let grub tell the kernel which part of setup_header was written by grub.
That's the only way I could find to let the kernel distinguish between
garbage and actual data.
> It would be enormously helpful if you could find out any more details about
> exactly what they are doing to break things.
That's easy:
The memory layout is:
0x1f1 bytes of data, including the sentinel, the setup_header, and then
more data.
grub did read the kernel's setup_header in the correct size into its
buffer (which contains random garbage before that), intializes the first
0x1f1 including the sentinel byte, and then writes back the buffer, but
using a too large length for that.
Juergen
On 09/11/2018 23:23, H. Peter Anvin wrote:
> I just noticed this patch -- I missed it because the cover message
> seemed far more harmless so I didn't notice this change.
>
> THIS PATCH IS FATALLY WRONG AND NEEDS TO BE IMMEDIATELY REVERTED BEFORE
> ANYONE STARTS RELYING ON IT; IT HAS THE POTENTIAL OF BREAKING THE
> BOOTLOADER PROTOCOL FOR ALL FUTURE.
>
> It seems to be based on fundamental misconceptions about the various
> data structures in the protocol, and does so in a way that completely
> breaks the way the protocol is designed to work.
>
> The protocol is specifically designed such that fields are not version
> dependencies. The version number is strictly to inform the boot loader
> about which capabilities the kernel has, so that the boot loader can
> know if a certain data field is meaningful and/or honored.
>
>> +Protocol 2.14: (Kernel 4.20) Added acpi_rsdp_addr holding the physical
>> + address of the ACPI RSDP table.
>> + The bootloader updates version with:
>> + 0x8000 | min(kernel-version, bootloader-version)
>> + kernel-version being the protocol version supported by
>> + the kernel and bootloader-version the protocol version
>> + supported by the bootloader.
>
> [...]
>
>> **** MEMORY LAYOUT
>>
>> The traditional memory map for the kernel loader, used for Image or
>> @@ -197,6 +209,7 @@ Offset Proto Name Meaning
>> 0258/8 2.10+ pref_address Preferred loading address
>> 0260/4 2.10+ init_size Linear memory required during initialization
>> 0264/4 2.11+ handover_offset Offset of handover entry point
>> +0268/8 2.14+ acpi_rsdp_addr Physical address of RSDP table
>
> NO.
>
> That is not how struct setup_header works, nor does this belong here.
>
> struct setup_header contains *initialized data*, and has a length byte
> at offset 0x201. The bootloader is responsible for copying the full
> structure into the appropriate offset (0x1f1) in struct boot_params.
>
> The length byte isn't actually a requirement, since the maximum possible
> size of this structure is 144 bytes, and the kernel will (obviously) not
> look at the older fields anyway, but it is good practice. The kernel or
> any other entity is free to zero out the bytes past this length pointer.
>
> There are only 24 bytes left in this structure, and this would occupy 8
> of them for no valid reason. The *only* valid reason to put a
> zero-initialized field in struct setup_header is if it used by the
> 16-bit legacy BIOS boot, which is obviously not the case here.
>
> This field thus belongs in struct boot_params, not struct setup_header.
Would you be okay with putting acpi_rsdp_addr at offset 0x0cc (_pad4)?
Juergen
On 11/10/18 1:03 AM, Juergen Gross wrote:
>
> How would that help? The garabge data written could have the correct
> terminal sentinel value by chance.
>
> That's why I re-used an existing field in setup_header (the version) to
> let grub tell the kernel which part of setup_header was written by grub.
>
> That's the only way I could find to let the kernel distinguish between
> garbage and actual data.
There is plenty of space *before* the setup_header part of struct boot_params
too -- look a the various __pad fields, especially (in your case), __pad3[16]
and __pad4[116] would suit the bill just fine.
>> It would be enormously helpful if you could find out any more details about
>> exactly what they are doing to break things.
>
> That's easy:
>
> The memory layout is:
>
> 0x1f1 bytes of data, including the sentinel, the setup_header, and then
> more data.
>
> grub did read the kernel's setup_header in the correct size into its
> buffer (which contains random garbage before that), intializes the first
> 0x1f1 including the sentinel byte, and then writes back the buffer, but
> using a too large length for that.
Are you kidding me... it really overwrites it with completely random data, and
not simply overspilling contents of the file?
In that case it might not be possible (or desirable) to use those N bytes
following the setup_heaader, or we need to a bigger sentinel than one byte
(probability being what it is, 256^n gets to be a pretty big number for any n,
very quickly drowning in the noise compared to other potential sources of boot
failures, and most likely less fatal than most.)
How big is this garbage dump? I'm going to brave a guess it is 512 bytes. In
that case this is hardly a big deal: the E820 map begins at 0x290, and the
setup_header maximum goes to 0x280, so it is only 15 bytes lost. If it is
worse than that, we would risk losing __pad8[48] and __pad9[276], and
especially the latter would be painful. In those case perhaps we should use
0x281..0x290 as a 15-byte sentinel; that is going to be virtually foolproof.
I'm also thinking that it might be desirable to add a flags field (__pad2
would be ideal) to struct boot_params; it would let us recycle some of the
obsolete fields (hd0_info, hd1_info, sys_desc_table, olpc_ofw_header, ...) and
perhaps be able to add some more robustness against these sort of things. This
would be the right way to do what your version feedback mechanism would do.
The reason why the feedback mechanism is fundamentally broken is that it only
gives the boot loader a way to assert that it supports a certain version of
the protocol, but it doesn't say *which* bootloader does such an assert -- and
therefore it is still wide open to implementation error.
We do, in fact, already have a feedback mechanism: the bootloader ID and
bootloader version. One way we could deal with this problem is to bump the
bootloader version reported by Grub, and add a quirk to the kernel that if the
bootloader ID is Grub (7) and the version is less than a certain number, zero
those fields. In fact, the more I think about it, this is what we should do.
That being said, Grub really needs to be kept honest. They keep making both
severe design (like refusing to use the BIOS and UEFI entry points provided by
the kernel by default) and implementation errors, apparently without
meaningful oversight. I really ask that the people more closely involved with
Grub try to keep a closer eye on their code as it applies to Linux.
-hpa
On November 10, 2018 7:22:29 AM PST, Juergen Gross <[email protected]> wrote:
>On 09/11/2018 23:23, H. Peter Anvin wrote:
>> I just noticed this patch -- I missed it because the cover message
>> seemed far more harmless so I didn't notice this change.
>>
>> THIS PATCH IS FATALLY WRONG AND NEEDS TO BE IMMEDIATELY REVERTED
>BEFORE
>> ANYONE STARTS RELYING ON IT; IT HAS THE POTENTIAL OF BREAKING THE
>> BOOTLOADER PROTOCOL FOR ALL FUTURE.
>>
>> It seems to be based on fundamental misconceptions about the various
>> data structures in the protocol, and does so in a way that completely
>> breaks the way the protocol is designed to work.
>>
>> The protocol is specifically designed such that fields are not
>version
>> dependencies. The version number is strictly to inform the boot
>loader
>> about which capabilities the kernel has, so that the boot loader can
>> know if a certain data field is meaningful and/or honored.
>>
>>> +Protocol 2.14: (Kernel 4.20) Added acpi_rsdp_addr holding the
>physical
>>> + address of the ACPI RSDP table.
>>> + The bootloader updates version with:
>>> + 0x8000 | min(kernel-version, bootloader-version)
>>> + kernel-version being the protocol version supported by
>>> + the kernel and bootloader-version the protocol version
>>> + supported by the bootloader.
>>
>> [...]
>>
>>> **** MEMORY LAYOUT
>>>
>>> The traditional memory map for the kernel loader, used for Image or
>>> @@ -197,6 +209,7 @@ Offset Proto Name Meaning
>>> 0258/8 2.10+ pref_address Preferred loading address
>>> 0260/4 2.10+ init_size Linear memory required during initialization
>>> 0264/4 2.11+ handover_offset Offset of handover entry point
>>> +0268/8 2.14+ acpi_rsdp_addr Physical address of RSDP table
>>
>> NO.
>>
>> That is not how struct setup_header works, nor does this belong here.
>>
>> struct setup_header contains *initialized data*, and has a length
>byte
>> at offset 0x201. The bootloader is responsible for copying the full
>> structure into the appropriate offset (0x1f1) in struct boot_params.
>>
>> The length byte isn't actually a requirement, since the maximum
>possible
>> size of this structure is 144 bytes, and the kernel will (obviously)
>not
>> look at the older fields anyway, but it is good practice. The kernel
>or
>> any other entity is free to zero out the bytes past this length
>pointer.
>>
>> There are only 24 bytes left in this structure, and this would occupy
>8
>> of them for no valid reason. The *only* valid reason to put a
>> zero-initialized field in struct setup_header is if it used by the
>> 16-bit legacy BIOS boot, which is obviously not the case here.
>>
>> This field thus belongs in struct boot_params, not struct
>setup_header.
>
>Would you be okay with putting acpi_rsdp_addr at offset 0x0cc (_pad4)?
>
>
>Juergen
I'd prefer if you used __pad3 offset 0x70 to keep the large block, and that way your field is also aligned.
However, if you have some specific reason to prefer __pad4 it's no big deal, although I'm curious what it would be.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Sun, Nov 11, 2018 at 10:49:39AM -0800, H. Peter Anvin wrote:
> On 11/10/18 1:03 AM, Juergen Gross wrote:
> >
> > How would that help? The garabge data written could have the correct
> > terminal sentinel value by chance.
> >
> > That's why I re-used an existing field in setup_header (the version) to
> > let grub tell the kernel which part of setup_header was written by grub.
> >
> > That's the only way I could find to let the kernel distinguish between
> > garbage and actual data.
>
> There is plenty of space *before* the setup_header part of struct boot_params
> too -- look a the various __pad fields, especially (in your case), __pad3[16]
> and __pad4[116] would suit the bill just fine.
>
> >> It would be enormously helpful if you could find out any more details about
> >> exactly what they are doing to break things.
> >
> > That's easy:
> >
> > The memory layout is:
> >
> > 0x1f1 bytes of data, including the sentinel, the setup_header, and then
> > more data.
> >
> > grub did read the kernel's setup_header in the correct size into its
> > buffer (which contains random garbage before that), intializes the first
> > 0x1f1 including the sentinel byte, and then writes back the buffer, but
> > using a too large length for that.
>
> Are you kidding me... it really overwrites it with completely random data, and
> not simply overspilling contents of the file?
>
> In that case it might not be possible (or desirable) to use those N bytes
> following the setup_heaader, or we need to a bigger sentinel than one byte
> (probability being what it is, 256^n gets to be a pretty big number for any n,
> very quickly drowning in the noise compared to other potential sources of boot
> failures, and most likely less fatal than most.)
>
> How big is this garbage dump? I'm going to brave a guess it is 512 bytes. In
> that case this is hardly a big deal: the E820 map begins at 0x290, and the
> setup_header maximum goes to 0x280, so it is only 15 bytes lost. If it is
> worse than that, we would risk losing __pad8[48] and __pad9[276], and
> especially the latter would be painful. In those case perhaps we should use
> 0x281..0x290 as a 15-byte sentinel; that is going to be virtually foolproof.
>
> I'm also thinking that it might be desirable to add a flags field (__pad2
> would be ideal) to struct boot_params; it would let us recycle some of the
> obsolete fields (hd0_info, hd1_info, sys_desc_table, olpc_ofw_header, ...) and
> perhaps be able to add some more robustness against these sort of things. This
> would be the right way to do what your version feedback mechanism would do.
>
> The reason why the feedback mechanism is fundamentally broken is that it only
> gives the boot loader a way to assert that it supports a certain version of
> the protocol, but it doesn't say *which* bootloader does such an assert -- and
> therefore it is still wide open to implementation error.
>
> We do, in fact, already have a feedback mechanism: the bootloader ID and
> bootloader version. One way we could deal with this problem is to bump the
> bootloader version reported by Grub, and add a quirk to the kernel that if the
> bootloader ID is Grub (7) and the version is less than a certain number, zero
> those fields. In fact, the more I think about it, this is what we should do.
>
> That being said, Grub really needs to be kept honest. They keep making both
> severe design (like refusing to use the BIOS and UEFI entry points provided by
> the kernel by default) and implementation errors, apparently without
> meaningful oversight. I really ask that the people more closely involved with
> Grub try to keep a closer eye on their code as it applies to Linux.
Cc-ing GRUB and Daniel Kiper (maintainer of GRUB).
Could folks please please CC Daniel Kiper on any of these patches in the future?
Thanks.
>
> -hpa