All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86/acpi: check rsdp address received via bootparams to be valid
@ 2018-01-16 13:57 Juergen Gross
  2018-01-16 15:46 ` Ingo Molnar
  2018-01-17 11:49 ` Rafael J. Wysocki
  0 siblings, 2 replies; 12+ messages in thread
From: Juergen Gross @ 2018-01-16 13:57 UTC (permalink / raw)
  To: linux-kernel, linux-acpi, x86
  Cc: lenb, rafael.j.wysocki, hpa, tglx, mingo, 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 <jgross@suse.com>
---
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(-)

diff --git a/drivers/acpi/acpica/tbxfroot.c b/drivers/acpi/acpica/tbxfroot.c
index f9f9a7da2cad..9edc71780a38 100644
--- a/drivers/acpi/acpica/tbxfroot.c
+++ b/drivers/acpi/acpica/tbxfroot.c
@@ -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
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 2b77db914752..facff5c11f32 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -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)) {
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index e1dd1a8d42b6..1edc47bd13c1 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -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,
-- 
2.13.6


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/acpi: check rsdp address received via bootparams to be valid
  2018-01-16 13:57 [PATCH] x86/acpi: check rsdp address received via bootparams to be valid Juergen Gross
@ 2018-01-16 15:46 ` Ingo Molnar
  2018-01-16 15:56   ` Juergen Gross
  2018-01-17 11:49 ` Rafael J. Wysocki
  1 sibling, 1 reply; 12+ messages in thread
From: Ingo Molnar @ 2018-01-16 15:46 UTC (permalink / raw)
  To: Juergen Gross
  Cc: linux-kernel, linux-acpi, x86, lenb, rafael.j.wysocki, hpa, tglx, mingo


* Juergen Gross <jgross@suse.com> wrote:

> 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/acpi: check rsdp address received via bootparams to be valid
  2018-01-16 15:46 ` Ingo Molnar
@ 2018-01-16 15:56   ` Juergen Gross
  2018-01-16 16:35     ` Ingo Molnar
  0 siblings, 1 reply; 12+ messages in thread
From: Juergen Gross @ 2018-01-16 15:56 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: linux-kernel, linux-acpi, x86, lenb, rafael.j.wysocki, hpa, tglx, mingo

On 16/01/18 16:46, Ingo Molnar wrote:
> 
> * Juergen Gross <jgross@suse.com> wrote:
> 
>> 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.

> 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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/acpi: check rsdp address received via bootparams to be valid
  2018-01-16 15:56   ` Juergen Gross
@ 2018-01-16 16:35     ` Ingo Molnar
  2018-01-17 12:28       ` Juergen Gross
  0 siblings, 1 reply; 12+ messages in thread
From: Ingo Molnar @ 2018-01-16 16:35 UTC (permalink / raw)
  To: Juergen Gross
  Cc: linux-kernel, linux-acpi, x86, lenb, rafael.j.wysocki, hpa, tglx, mingo


* Juergen Gross <jgross@suse.com> wrote:

> On 16/01/18 16:46, Ingo Molnar wrote:
> > 
> > * Juergen Gross <jgross@suse.com> wrote:
> > 
> >> 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.
> 
> > 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/acpi: check rsdp address received via bootparams to be valid
  2018-01-16 13:57 [PATCH] x86/acpi: check rsdp address received via bootparams to be valid Juergen Gross
  2018-01-16 15:46 ` Ingo Molnar
@ 2018-01-17 11:49 ` Rafael J. Wysocki
  2018-01-17 12:20   ` Juergen Gross
  1 sibling, 1 reply; 12+ messages in thread
From: Rafael J. Wysocki @ 2018-01-17 11:49 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
	the arch/x86 maintainers, Len Brown, Rafael Wysocki,
	H. Peter Anvin, Thomas Gleixner, Ingo Molnar

On Tue, Jan 16, 2018 at 2:57 PM, Juergen Gross <jgross@suse.com> wrote:
> 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 <jgross@suse.com>
> ---
> 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(-)
>
> diff --git a/drivers/acpi/acpica/tbxfroot.c b/drivers/acpi/acpica/tbxfroot.c
> index f9f9a7da2cad..9edc71780a38 100644
> --- a/drivers/acpi/acpica/tbxfroot.c
> +++ b/drivers/acpi/acpica/tbxfroot.c
> @@ -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?

> + *
> + *
> + * 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
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 2b77db914752..facff5c11f32 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -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.

>                 return pa;
>
>         if (efi_enabled(EFI_CONFIG_TABLES)) {
> diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
> index e1dd1a8d42b6..1edc47bd13c1 100644
> --- a/include/acpi/acpixf.h
> +++ b/include/acpi/acpixf.h
> @@ -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,
> --

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/acpi: check rsdp address received via bootparams to be valid
  2018-01-17 11:49 ` Rafael J. Wysocki
@ 2018-01-17 12:20   ` Juergen Gross
  0 siblings, 0 replies; 12+ messages in thread
From: Juergen Gross @ 2018-01-17 12:20 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
	the arch/x86 maintainers, Len Brown, Rafael Wysocki,
	H. Peter Anvin, Thomas Gleixner, Ingo Molnar

On 17/01/18 12:49, Rafael J. Wysocki wrote:
> On Tue, Jan 16, 2018 at 2:57 PM, Juergen Gross <jgross@suse.com> wrote:
>> 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 <jgross@suse.com>
>> ---
>> 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(-)
>>
>> diff --git a/drivers/acpi/acpica/tbxfroot.c b/drivers/acpi/acpica/tbxfroot.c
>> index f9f9a7da2cad..9edc71780a38 100644
>> --- a/drivers/acpi/acpica/tbxfroot.c
>> +++ b/drivers/acpi/acpica/tbxfroot.c
>> @@ -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?

It seemed to fit in here.

>> + *
>> + *
>> + * 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
>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
>> index 2b77db914752..facff5c11f32 100644
>> --- a/drivers/acpi/osl.c
>> +++ b/drivers/acpi/osl.c
>> @@ -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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/acpi: check rsdp address received via bootparams to be valid
  2018-01-16 16:35     ` Ingo Molnar
@ 2018-01-17 12:28       ` Juergen Gross
  2018-01-17 15:34         ` Ingo Molnar
  0 siblings, 1 reply; 12+ messages in thread
From: Juergen Gross @ 2018-01-17 12:28 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: linux-kernel, linux-acpi, x86, lenb, rafael.j.wysocki, hpa, tglx, mingo

On 16/01/18 17:35, Ingo Molnar wrote:
> 
> * Juergen Gross <jgross@suse.com> wrote:
> 
>> On 16/01/18 16:46, Ingo Molnar wrote:
>>>
>>> * Juergen Gross <jgross@suse.com> wrote:
>>>
>>>> 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.
>>
>>> 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.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/acpi: check rsdp address received via bootparams to be valid
  2018-01-17 12:28       ` Juergen Gross
@ 2018-01-17 15:34         ` Ingo Molnar
  2018-01-17 18:12           ` Juergen Gross
  0 siblings, 1 reply; 12+ messages in thread
From: Ingo Molnar @ 2018-01-17 15:34 UTC (permalink / raw)
  To: Juergen Gross
  Cc: linux-kernel, linux-acpi, x86, lenb, rafael.j.wysocki, hpa, tglx, mingo


* Juergen Gross <jgross@suse.com> wrote:

> On 16/01/18 17:35, Ingo Molnar wrote:
> > 
> > * Juergen Gross <jgross@suse.com> wrote:
> > 
> >> On 16/01/18 16:46, Ingo Molnar wrote:
> >>>
> >>> * Juergen Gross <jgross@suse.com> wrote:
> >>>
> >>>> 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.
> >>
> >>> 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.
> 
> 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?

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/acpi: check rsdp address received via bootparams to be valid
  2018-01-17 15:34         ` Ingo Molnar
@ 2018-01-17 18:12           ` Juergen Gross
  2018-02-13 16:32               ` Ingo Molnar
  0 siblings, 1 reply; 12+ messages in thread
From: Juergen Gross @ 2018-01-17 18:12 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: linux-kernel, linux-acpi, x86, lenb, rafael.j.wysocki, hpa, tglx, mingo

On 17/01/18 16:34, Ingo Molnar wrote:
> 
> * Juergen Gross <jgross@suse.com> wrote:
> 
>> On 16/01/18 17:35, Ingo Molnar wrote:
>>>
>>> * Juergen Gross <jgross@suse.com> wrote:
>>>
>>>> On 16/01/18 16:46, Ingo Molnar wrote:
>>>>>
>>>>> * Juergen Gross <jgross@suse.com> wrote:
>>>>>
>>>>>> 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.
>>>>
>>>>> 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.
>>
>> 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?
> 
> 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.

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

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

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

>From version 0x020e on (current version without my patches is 0x020d)
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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/acpi: check rsdp address received via bootparams to be valid
  2018-01-17 18:12           ` Juergen Gross
@ 2018-02-13 16:32               ` Ingo Molnar
  0 siblings, 0 replies; 12+ messages in thread
From: Ingo Molnar @ 2018-02-13 16:32 UTC (permalink / raw)
  To: Juergen Gross
  Cc: linux-kernel, linux-acpi, x86, lenb, rafael.j.wysocki, hpa, tglx, mingo


* Juergen Gross <jgross@suse.com> wrote:

> So what about the following:
> 
> From version 0x020e on (current version without my patches is 0x020d)
> 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.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/acpi: check rsdp address received via bootparams to be valid
@ 2018-02-13 16:32               ` Ingo Molnar
  0 siblings, 0 replies; 12+ messages in thread
From: Ingo Molnar @ 2018-02-13 16:32 UTC (permalink / raw)
  To: Juergen Gross, H. Peter Anvin
  Cc: linux-kernel, linux-acpi, x86, lenb, rafael.j.wysocki, hpa, tglx, mingo


* Juergen Gross <jgross@suse.com> wrote:

> So what about the following:
> 
> From version 0x020e on (current version without my patches is 0x020d)
> 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.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/acpi: check rsdp address received via bootparams to be valid
  2018-02-13 16:32               ` Ingo Molnar
  (?)
@ 2018-02-13 16:48               ` Juergen Gross
  -1 siblings, 0 replies; 12+ messages in thread
From: Juergen Gross @ 2018-02-13 16:48 UTC (permalink / raw)
  To: Ingo Molnar, H. Peter Anvin
  Cc: linux-kernel, linux-acpi, x86, lenb, rafael.j.wysocki, tglx, mingo

On 13/02/18 17:32, Ingo Molnar wrote:
> 
> * Juergen Gross <jgross@suse.com> wrote:
> 
>> So what about the following:
>>
>> From version 0x020e on (current version without my patches is 0x020d)
>> 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.
> 
> 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.

> I've removed the patches for the time being.

Seems a sensible thing to do.


Juergen

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-02-13 16:48 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-16 13:57 [PATCH] x86/acpi: check rsdp address received via bootparams to be valid Juergen Gross
2018-01-16 15:46 ` Ingo Molnar
2018-01-16 15:56   ` Juergen Gross
2018-01-16 16:35     ` Ingo Molnar
2018-01-17 12:28       ` Juergen Gross
2018-01-17 15:34         ` Ingo Molnar
2018-01-17 18:12           ` Juergen Gross
2018-02-13 16:32             ` Ingo Molnar
2018-02-13 16:32               ` Ingo Molnar
2018-02-13 16:48               ` Juergen Gross
2018-01-17 11:49 ` Rafael J. Wysocki
2018-01-17 12:20   ` Juergen Gross

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.