x86/acpi: check rsdp address received via bootparams to be valid
Subsystems (2)
PATCH 1/1 x86/acpi: check rsdp address received via bootparams to be valid
There seem to exist several grub2 versions trashing boot_params.hdr.acpi_rsdp_addr.
So don't just believe this address to be valid, but verify it pointing to a valid RSDP table.
---
To be applied on top of my RSDP series currently in tip.git x86/boot Mike Galbraith has tested this patch to repair his broken boot
--- drivers/acpi/acpica/tbxfroot.c | 36 ++++++++++++++++++++++++++++++++++++ drivers/acpi/osl.c | 2 +- include/acpi/acpixf.h | 5 +++++ 3 files changed, 42 insertions(+), 1 deletion(-)
drivers/acpi/acpica/tbxfroot.c +36 -0
@@ -249,6 +249,42 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_find_root_pointer)/******************************************************************************* * +* FUNCTION: acpi_verify_root_pointer +* +* +* PARAMETERS: rsdp_address - Pointer to suspected RSDP +* +* RETURN: AE_OK if rsdp_address really points to a RSDP, AE_NOT_FOUND +* else +* +* DESCRIPTION: Verify a physical address being a valid RSDP +* +******************************************************************************/ + +acpi_status ACPI_INIT_FUNCTION +acpi_verify_root_pointer (acpi_physical_address rsdp_address) +{ +acpi_status status; +struct acpi_table_rsdp *rsdp; + +ACPI_FUNCTION_TRACE (acpi_verify_root_pointer); + + rsdp =acpi_os_map_memory (rsdp_address,sizeof (*rsdp)); + +if (!rsdp) +return_ACPI_STATUS (AE_NO_MEMORY ); + + status =acpi_tb_validate_rsdp (rsdp); + +acpi_os_unmap_memory (rsdp,sizeof (*rsdp)); + +return_ACPI_STATUS (status); +} + +ACPI_EXPORT_SYMBOL_INIT (acpi_verify_root_pointer) + +/******************************************************************************* +* * FUNCTION: acpi_tb_scan_memory_for_rsdp * * PARAMETERS: start_address - Starting pointer for search
drivers/acpi/osl.c +1 -1
@@ -201,7 +201,7 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)return acpi_rsdp;#endif pa =acpi_arch_get_root_pointer (); -if (pa) +if (pa &&acpi_verify_root_pointer (pa) ==AE_OK )return pa;if (efi_enabled (EFI_CONFIG_TABLES )) {
include/acpi/acpixf.h +5 -0
@@ -505,6 +505,11 @@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status ACPI_INIT_FUNCTION ACPI_EXTERNAL_RETURN_STATUS(acpi_status ACPI_INIT_FUNCTION acpi_find_root_pointer(acpi_physical_address *rsdp_address)) + +ACPI_EXTERNAL_RETURN_OK(acpi_status ACPI_INIT_FUNCTION + acpi_verify_root_pointer(acpi_physical_address + rsdp_address)) + ACPI_EXTERNAL_RETURN_STATUS(acpi_status acpi_get_table_header(acpi_string signature, u32 instance,
Signature · 1 line
2.13.6
Quoted 5 lines · Juergen Gross
There seem to exist several grub2 versions trashing boot_params.hdr.acpi_rsdp_addr.
So don't just believe this address to be valid, but verify it pointing to a valid RSDP table.
Exactly what kind of values do those Grub2 versions put into that field? Pointer to something, or random noise?
Also, what exactly does 'validation' mean, how robustly does it filter out of spec uses of the boot protocol?
Thanks,
Ingo
Quoted 11 lines · Ingo Molnar
Quoted 5 lines · Juergen Gross
There seem to exist several grub2 versions trashing boot_params.hdr.acpi_rsdp_addr.
So don't just believe this address to be valid, but verify it pointing to a valid RSDP table.
Exactly what kind of values do those Grub2 versions put into that field? Pointer to something, or random noise?
Looks like random noise. On Mike's system it was 0x000000000151.
Quoted 2 lines
Also, what exactly does 'validation' mean, how robustly does it filter out of spec uses of the boot protocol?
It validates the RSDP has the correct 8 byte eye catcher in it and the checksum of the structure is correct. Searching the RSDP by scanning memory is using the same checks, so I guess this ought to be okay. Odds are about 1 : 2^80 for false positives.
Juergen
Quoted 15 lines · Juergen Gross
Quoted 11 lines · Ingo Molnar
Quoted 5 lines · Juergen Gross
There seem to exist several grub2 versions trashing boot_params.hdr.acpi_rsdp_addr.
So don't just believe this address to be valid, but verify it pointing to a valid RSDP table.
Exactly what kind of values do those Grub2 versions put into that field? Pointer to something, or random noise?
Looks like random noise. On Mike's system it was
0x000000000151.
Quoted 7 lines
Quoted 2 lines
Also, what exactly does 'validation' mean, how robustly does it filter out of spec uses of the boot protocol?
It validates the RSDP has the correct 8 byte eye catcher in it and the checksum of the structure is correct. Searching the RSDP by scanning memory is using the same checks, so I guess this ought to be okay. Odds are about 1 : 2^80 for false positives.
Ok, this should work - but only because the RSDP is defined in such a robust fashion.
The boot protocol extension is still fragile: what I worry about is that if we start relying on the extended boot protocol with widespread installed base of out of spec Grub2 loaders, other extensions (which cannot be sanity checked) would be less robust.
Is there a way to detect the broken Grub2 versions somehow and just limit the boot protocol for them?
The other solution would be to just discontinue this boot protocol extension and define a new one.
Thanks,
Ingo
Quoted 34 lines · Ingo Molnar
Quoted 15 lines · Juergen Gross
Quoted 11 lines · Ingo Molnar
Quoted 5 lines · Juergen Gross
There seem to exist several grub2 versions trashing boot_params.hdr.acpi_rsdp_addr.
So don't just believe this address to be valid, but verify it pointing to a valid RSDP table.
Exactly what kind of values do those Grub2 versions put into that field? Pointer to something, or random noise?
Looks like random noise. On Mike's system it was
0x000000000151.Quoted 7 lines
Quoted 2 lines
Also, what exactly does 'validation' mean, how robustly does it filter out of spec uses of the boot protocol?
It validates the RSDP has the correct 8 byte eye catcher in it and the checksum of the structure is correct. Searching the RSDP by scanning memory is using the same checks, so I guess this ought to be okay. Odds are about 1 : 2^80 for false positives.
Ok, this should work - but only because the RSDP is defined in such a robust fashion.
The boot protocol extension is still fragile: what I worry about is that if we start relying on the extended boot protocol with widespread installed base of out of spec Grub2 loaders, other extensions (which cannot be sanity checked) would be less robust.
Quoted 5 lines
Is there a way to detect the broken Grub2 versions somehow and just limit the boot protocol for them?
The other solution would be to just discontinue this boot protocol extension and define a new one.
I think the best way to do it would be to let grub2 report the version of the protocol it is honoring. This reported version should not be higher than that of the kernel. The kernel would then know which fields of setup_header are known by grub2 and thus can be trusted to be correct.
I can modify my patch series to add the grub2 version information before the new rsdp_address field so this band-aid patch won't be required.
What do you think?
Juergen
Quoted 47 lines · Juergen Gross
Quoted 34 lines · Ingo Molnar
Quoted 15 lines · Juergen Gross
Quoted 11 lines · Ingo Molnar
Quoted 5 lines · Juergen Gross
There seem to exist several grub2 versions trashing boot_params.hdr.acpi_rsdp_addr.
So don't just believe this address to be valid, but verify it pointing to a valid RSDP table.
Exactly what kind of values do those Grub2 versions put into that field? Pointer to something, or random noise?
Looks like random noise. On Mike's system it was
0x000000000151.Quoted 7 lines
Quoted 2 lines
Also, what exactly does 'validation' mean, how robustly does it filter out of spec uses of the boot protocol?
It validates the RSDP has the correct 8 byte eye catcher in it and the checksum of the structure is correct. Searching the RSDP by scanning memory is using the same checks, so I guess this ought to be okay. Odds are about 1 : 2^80 for false positives.
Ok, this should work - but only because the RSDP is defined in such a robust fashion.
The boot protocol extension is still fragile: what I worry about is that if we start relying on the extended boot protocol with widespread installed base of out of spec Grub2 loaders, other extensions (which cannot be sanity checked) would be less robust.
Quoted 5 lines
Is there a way to detect the broken Grub2 versions somehow and just limit the boot protocol for them?
The other solution would be to just discontinue this boot protocol extension and define a new one.
I think the best way to do it would be to let grub2 report the version of the protocol it is honoring. This reported version should not be higher than that of the kernel. The kernel would then know which fields of
setup_headerare known by grub2 and thus can be trusted to be correct.
Quoted 4 lines
I can modify my patch series to add the grub2 version information before the new
rsdp_addressfield so this band-aid patch won't be required.What do you think?
Such an approach would be a lot more future proof I think - except that I don't think the ABI should be 'version' based but 'size' based, like sys_sched_getattr().
The idea is that we introduce a 'size' field in the existing boot params, which would be zero for old bootloaders. (Is such a free field available?)
The boot loader puts the boot parameter structure size it currently supports, and the kernel reads it and acts accordingly.
This is flexible in a both forwards and backwards ABI-compatible manner, i.e. old kernel could run with new boot loader and old bootloader could run with new kernel, while having the maximum common set of boot protocol features supported.
If such an interface would be possible to create, that is.
Thanks,
Ingo
Quoted 50 lines · Ingo Molnar
Quoted 47 lines · Juergen Gross
Quoted 34 lines · Ingo Molnar
Quoted 15 lines · Juergen Gross
Quoted 11 lines · Ingo Molnar
Quoted 5 lines · Juergen Gross
There seem to exist several grub2 versions trashing boot_params.hdr.acpi_rsdp_addr.
So don't just believe this address to be valid, but verify it pointing to a valid RSDP table.
Exactly what kind of values do those Grub2 versions put into that field? Pointer to something, or random noise?
Looks like random noise. On Mike's system it was
0x000000000151.Quoted 7 lines
Quoted 2 lines
Also, what exactly does 'validation' mean, how robustly does it filter out of spec uses of the boot protocol?
It validates the RSDP has the correct 8 byte eye catcher in it and the checksum of the structure is correct. Searching the RSDP by scanning memory is using the same checks, so I guess this ought to be okay. Odds are about 1 : 2^80 for false positives.
Ok, this should work - but only because the RSDP is defined in such a robust fashion.
The boot protocol extension is still fragile: what I worry about is that if we start relying on the extended boot protocol with widespread installed base of out of spec Grub2 loaders, other extensions (which cannot be sanity checked) would be less robust.
Quoted 5 lines
Is there a way to detect the broken Grub2 versions somehow and just limit the boot protocol for them?
The other solution would be to just discontinue this boot protocol extension and define a new one.
I think the best way to do it would be to let grub2 report the version of the protocol it is honoring. This reported version should not be higher than that of the kernel. The kernel would then know which fields of
setup_headerare known by grub2 and thus can be trusted to be correct.
Quoted 8 lines
Quoted 4 lines
I can modify my patch series to add the grub2 version information before the new
rsdp_addressfield so this band-aid patch won't be required.What do you think?
Such an approach would be a lot more future proof I think - except that I don't think the ABI should be 'version' based but 'size' based, like
sys_sched_getattr().
Hmm, the interface from the kernel to the bootloader already has the version in it. So it would be more natural to use the same kind of information in the other direction, too.
Quoted 2 lines
The idea is that we introduce a 'size' field in the existing boot params, which would be zero for old bootloaders. (Is such a free field available?)
No. Look in arch/x86/include/uapi/asm/bootparam.h for struct setup_header.
Quoted 2 lines
The boot loader puts the boot parameter structure size it currently supports, and the kernel reads it and acts accordingly.
And here the problems begin: lets say current kernel knows about setup_header size of 200 bytes. The boot loader is writing 300 into the size field. Does this mean the boot loader is very new, or did it just write garbage into the size field?
Quoted 5 lines
This is flexible in a both forwards and backwards ABI-compatible manner, i.e. old kernel could run with new boot loader and old bootloader could run with new kernel, while having the maximum common set of boot protocol features supported.
If such an interface would be possible to create, that is.
What I suggested (adding the boot loader version after the current end) won't work. We need some kind of feedback in the current header, as otherwise we can never be sure whether the feedback is garbage or real.
So what about the following:
Quoted 1 line
From version
0x020eon (current version without my patches is0x020d)
the boot loader will write its supported version or'ed with 0x8000 into the same version field. So we can be sure any added fields are filled with real data or the information has been evaluated by the boot loader, if the returned version included that field. If the boot loader finds the kernel supports a version < 0x020e it won't return its supported version.
Juergen
Quoted 9 lines · Juergen Gross
So what about the following:
From version
0x020eon (current version without my patches is0x020d) the boot loader will write its supported version or'ed with0x8000into the same version field. So we can be sure any added fields are filled with real data or the information has been evaluated by the boot loader, if the returned version included that field. If the boot loader finds the kernel supports a version <0x020eit won't return its supported version.
This seems like a usable workaround to me, as long as everyone involved agrees with it.
I've removed the patches for the time being.
Thanks,
Ingo
Quoted 13 lines · Ingo Molnar
Quoted 9 lines · Juergen Gross
So what about the following:
From version
0x020eon (current version without my patches is0x020d) the boot loader will write its supported version or'ed with0x8000into the same version field. So we can be sure any added fields are filled with real data or the information has been evaluated by the boot loader, if the returned version included that field. If the boot loader finds the kernel supports a version <0x020eit won't return its supported version.
Quoted 2 lines
This seems like a usable workaround to me, as long as everyone involved agrees with it.
Okay, thanks for the feedback.
I'll go that route.
Quoted 1 line
I've removed the patches for the time being.
Seems a sensible thing to do.
Juergen
Rafael J. Wysocki
maintainerQuoted 21 lines · Juergen Gross
There seem to exist several grub2 versions trashing boot_params.hdr.acpi_rsdp_addr.
So don't just believe this address to be valid, but verify it pointing to a valid RSDP table.
Signed-off-by Juergen Gross <[email protected]>---
To be applied on top of my RSDP series currently in tip.git x86/boot Mike Galbraith has tested this patch to repair his broken boot--- drivers/acpi/acpica/tbxfroot.c | 36 ++++++++++++++++++++++++++++++++++++ drivers/acpi/osl.c | 2 +- include/acpi/acpixf.h | 5 +++++ 3 files changed, 42 insertions(+), 1 deletion(-)drivers/acpi/acpica/tbxfroot.c +0
-0
drivers/acpi/acpica/tbxfroot.c ACPI_EXPORT_SYMBOL_INIT(acpi_find_root_pointer) · line 252
Quoted 4 lines
drivers/acpi/acpica/tbxfroot.c +1
-0@@ -249,6 +249,42 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_find_root_pointer)/******************************************************************************* * +* FUNCTION: acpi_verify_root_pointer
Why do you want this to go into ACPICA?
Quoted 43 lines
drivers/acpi/acpica/tbxfroot.c +35
-0+* +* +* PARAMETERS: rsdp_address - Pointer to suspected RSDP +* +* RETURN: AE_OK if rsdp_address really points to a RSDP, AE_NOT_FOUND +* else +* +* DESCRIPTION: Verify a physical address being a valid RSDP +* +******************************************************************************/ + +acpi_status ACPI_INIT_FUNCTION +acpi_verify_root_pointer (acpi_physical_address rsdp_address) +{ +acpi_status status; +struct acpi_table_rsdp *rsdp; + +ACPI_FUNCTION_TRACE (acpi_verify_root_pointer); + + rsdp =acpi_os_map_memory (rsdp_address,sizeof (*rsdp)); + +if (!rsdp) +return_ACPI_STATUS (AE_NO_MEMORY ); + + status =acpi_tb_validate_rsdp (rsdp); + +acpi_os_unmap_memory (rsdp,sizeof (*rsdp)); + +return_ACPI_STATUS (status); +} + +ACPI_EXPORT_SYMBOL_INIT (acpi_verify_root_pointer) + +/******************************************************************************* +* * FUNCTION: acpi_tb_scan_memory_for_rsdp * * PARAMETERS: start_address - Starting pointer for search drivers/acpi/osl.c +0
-0
drivers/acpi/osl.c acpi_physical_address __init acpi_os_get_root_pointer(void) · line 204
Quoted 5 lines
drivers/acpi/osl.c +1
-1@@ -201,7 +201,7 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)return acpi_rsdp;#endif pa =acpi_arch_get_root_pointer (); -if (pa) +if (pa &&acpi_verify_root_pointer (pa) ==AE_OK )
All things needed could just be done locally in this file.
Quoted 20 lines
drivers/acpi/osl.c +0
-0return pa;if (efi_enabled (EFI_CONFIG_TABLES )) {include/acpi/acpixf.h +5
-1@@ -505,6 +505,11 @@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status ACPI_INIT_FUNCTION ACPI_EXTERNAL_RETURN_STATUS(acpi_status ACPI_INIT_FUNCTION acpi_find_root_pointer(acpi_physical_address *rsdp_address)) + +ACPI_EXTERNAL_RETURN_OK(acpi_status ACPI_INIT_FUNCTION + acpi_verify_root_pointer(acpi_physical_address + rsdp_address)) + ACPI_EXTERNAL_RETURN_STATUS(acpi_status acpi_get_table_header(acpi_string signature, u32 instance, --
Quoted 23 lines · Rafael J. Wysocki
Quoted 22 lines · Juergen Gross
There seem to exist several grub2 versions trashing boot_params.hdr.acpi_rsdp_addr.
So don't just believe this address to be valid, but verify it pointing to a valid RSDP table.
Signed-off-by Juergen Gross <[email protected]>---
To be applied on top of my RSDP series currently in tip.git x86/boot Mike Galbraith has tested this patch to repair his broken boot--- drivers/acpi/acpica/tbxfroot.c | 36 ++++++++++++++++++++++++++++++++++++ drivers/acpi/osl.c | 2 +- include/acpi/acpixf.h | 5 +++++ 3 files changed, 42 insertions(+), 1 deletion(-)drivers/acpi/acpica/tbxfroot.c +0
-0@@ -249,6 +249,42 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_find_root_pointer)
Quoted 5 lines
drivers/acpi/acpica/tbxfroot.c ACPI_EXPORT_SYMBOL_INIT(acpi_find_root_pointer) · line 252
Quoted 3 lines
drivers/acpi/acpica/tbxfroot.c +1
-0/******************************************************************************* * +* FUNCTION: acpi_verify_root_pointer Why do you want this to go into ACPICA?
It seemed to fit in here.
Quoted 42 lines
Quoted 42 lines
drivers/acpi/acpica/tbxfroot.c +35
-0+* +* +* PARAMETERS: rsdp_address - Pointer to suspected RSDP +* +* RETURN: AE_OK if rsdp_address really points to a RSDP, AE_NOT_FOUND +* else +* +* DESCRIPTION: Verify a physical address being a valid RSDP +* +******************************************************************************/ + +acpi_status ACPI_INIT_FUNCTION +acpi_verify_root_pointer (acpi_physical_address rsdp_address) +{ +acpi_status status; +struct acpi_table_rsdp *rsdp; + +ACPI_FUNCTION_TRACE (acpi_verify_root_pointer); + + rsdp =acpi_os_map_memory (rsdp_address,sizeof (*rsdp)); + +if (!rsdp) +return_ACPI_STATUS (AE_NO_MEMORY ); + + status =acpi_tb_validate_rsdp (rsdp); + +acpi_os_unmap_memory (rsdp,sizeof (*rsdp)); + +return_ACPI_STATUS (status); +} + +ACPI_EXPORT_SYMBOL_INIT (acpi_verify_root_pointer) + +/******************************************************************************* +* * FUNCTION: acpi_tb_scan_memory_for_rsdp * * PARAMETERS: start_address - Starting pointer for search drivers/acpi/osl.c +0
-0
Quoted 8 lines
drivers/acpi/osl.c acpi_physical_address __init acpi_os_get_root_pointer(void) · line 204
Quoted 6 lines
drivers/acpi/osl.c +1
-1@@ -201,7 +201,7 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)return acpi_rsdp;#endif pa =acpi_arch_get_root_pointer (); -if (pa) +if (pa &&acpi_verify_root_pointer (pa) ==AE_OK )All things needed could just be done locally in this file.
Okay, if you prefer it this way I can move acpi_verify_root_pointer() to osl.c.
Juergen