x86/boot: clear rsdp address in boot_params for broken loaders
diff mbox series

Message ID 20181203103811.17056-1-jgross@suse.com
State In Next
Commit 182ddd16194cd082f25fa1b063dae3c7c5cce384
Headers show
Series
  • x86/boot: clear rsdp address in boot_params for broken loaders
Related show

Commit Message

Juergen Gross Dec. 3, 2018, 10:38 a.m. UTC
In case a broken boot loader doesn't clear its struct boot_params clear
rsdp_addr in sanitize_boot_params().

This fixes commit e6e094e053af75 ("x86/acpi, x86/boot: Take RSDP
address from boot params if available") e.g. for the case of a boot via
systemd-boot.

Fixes: e6e094e053af75 ("x86/acpi, x86/boot: Take RSDP address from boot params if available")
Reported-by: Gunnar Krueger <taijian@posteo.de>
Tested-by: Gunnar Krueger <taijian@posteo.de>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/include/asm/bootparam_utils.h | 1 +
 1 file changed, 1 insertion(+)

Comments

H. Peter Anvin Dec. 3, 2018, 11:07 p.m. UTC | #1
On December 3, 2018 2:38:11 AM PST, Juergen Gross <jgross@suse.com> wrote:
>In case a broken boot loader doesn't clear its struct boot_params clear
>rsdp_addr in sanitize_boot_params().
>
>This fixes commit e6e094e053af75 ("x86/acpi, x86/boot: Take RSDP
>address from boot params if available") e.g. for the case of a boot via
>systemd-boot.
>
>Fixes: e6e094e053af75 ("x86/acpi, x86/boot: Take RSDP address from boot
>params if available")
>Reported-by: Gunnar Krueger <taijian@posteo.de>
>Tested-by: Gunnar Krueger <taijian@posteo.de>
>Signed-off-by: Juergen Gross <jgross@suse.com>
>---
> arch/x86/include/asm/bootparam_utils.h | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/arch/x86/include/asm/bootparam_utils.h
>b/arch/x86/include/asm/bootparam_utils.h
>index a07ffd23e4dd..f6f6ef436599 100644
>--- a/arch/x86/include/asm/bootparam_utils.h
>+++ b/arch/x86/include/asm/bootparam_utils.h
>@@ -36,6 +36,7 @@ static void sanitize_boot_params(struct boot_params
>*boot_params)
> 	 */
> 	if (boot_params->sentinel) {
> 		/* fields in boot_params are left uninitialized, clear them */
>+		boot_params->acpi_rsdp_addr = 0;
> 		memset(&boot_params->ext_ramdisk_image, 0,
> 		       (char *)&boot_params->efi_info -
> 			(char *)&boot_params->ext_ramdisk_image);

Isn't this already covered by the memset()? If not, we should extend the memset() to maximal coverage.
Juergen Gross Dec. 4, 2018, 5:32 a.m. UTC | #2
On 04/12/2018 00:07, hpa@zytor.com wrote:
> On December 3, 2018 2:38:11 AM PST, Juergen Gross <jgross@suse.com> wrote:
>> In case a broken boot loader doesn't clear its struct boot_params clear
>> rsdp_addr in sanitize_boot_params().
>>
>> This fixes commit e6e094e053af75 ("x86/acpi, x86/boot: Take RSDP
>> address from boot params if available") e.g. for the case of a boot via
>> systemd-boot.
>>
>> Fixes: e6e094e053af75 ("x86/acpi, x86/boot: Take RSDP address from boot
>> params if available")
>> Reported-by: Gunnar Krueger <taijian@posteo.de>
>> Tested-by: Gunnar Krueger <taijian@posteo.de>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> arch/x86/include/asm/bootparam_utils.h | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/x86/include/asm/bootparam_utils.h
>> b/arch/x86/include/asm/bootparam_utils.h
>> index a07ffd23e4dd..f6f6ef436599 100644
>> --- a/arch/x86/include/asm/bootparam_utils.h
>> +++ b/arch/x86/include/asm/bootparam_utils.h
>> @@ -36,6 +36,7 @@ static void sanitize_boot_params(struct boot_params
>> *boot_params)
>> 	 */
>> 	if (boot_params->sentinel) {
>> 		/* fields in boot_params are left uninitialized, clear them */
>> +		boot_params->acpi_rsdp_addr = 0;
>> 		memset(&boot_params->ext_ramdisk_image, 0,
>> 		       (char *)&boot_params->efi_info -
>> 			(char *)&boot_params->ext_ramdisk_image);
> 
> Isn't this already covered by the memset()? If not, we should extend the memset() to maximal coverage.

I'd like to send a followup patch doing that. And I'd like to not only
test sentinel for being non-zero, but all padding fields as well. This
should be 4.21 material, though.


Juergen
H. Peter Anvin Dec. 4, 2018, 5:49 a.m. UTC | #3
On 12/3/18 9:32 PM, Juergen Gross wrote:
> 
> I'd like to send a followup patch doing that. And I'd like to not only
> test sentinel for being non-zero, but all padding fields as well. This
> should be 4.21 material, though.
> 

No, you can't do that.  That breaks backwards compatibility.

	-hpa
Juergen Gross Dec. 4, 2018, 6:03 a.m. UTC | #4
On 04/12/2018 06:49, H. Peter Anvin wrote:
> On 12/3/18 9:32 PM, Juergen Gross wrote:
>>
>> I'd like to send a followup patch doing that. And I'd like to not only
>> test sentinel for being non-zero, but all padding fields as well. This
>> should be 4.21 material, though.
>>
> 
> No, you can't do that.  That breaks backwards compatibility.

So you are speaking about paddings which are at places where there used
to be some information? Shouldn't those be named "_res*"?
Recycling such paddings with some useful information seems to be rather
dangerous then.

I'd like to have at least some idea which boot loader is not passing a
clean struct boot_params. So I think we should at least have some debug
or info messages telling us which paddings are not zero initially to be
able to either fix the boot loader or switch from _pad* to _res* naming.


Juergen

Patch
diff mbox series

diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index a07ffd23e4dd..f6f6ef436599 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -36,6 +36,7 @@  static void sanitize_boot_params(struct boot_params *boot_params)
 	 */
 	if (boot_params->sentinel) {
 		/* fields in boot_params are left uninitialized, clear them */
+		boot_params->acpi_rsdp_addr = 0;
 		memset(&boot_params->ext_ramdisk_image, 0,
 		       (char *)&boot_params->efi_info -
 			(char *)&boot_params->ext_ramdisk_image);