Re: [PATCH 2/2] efi/cper: Avoid possible OOB when checking generic data block
On Tue, Jan 22, 2019 at 04:09:12PM +, Ross Lagerwall wrote:
> When checking a generic status block, we iterate over all the generic
> data blocks. The loop condition only checks that the start of the
> generic data block is valid (within estatus->data_length) but not the
> whole block. Because the size of data blocks (excluding error data) may
> vary depending on the revision and the revision is contained within the
> data block, ensure that enough of the current data block is valid before
> dereferencing any members otherwise an OOB access may occur if
Please write out the OOB abbreviation in your commit messages.
> estatus->data_length is invalid. This relies on the fact that
> struct acpi_hest_generic_data_v300 is a superset of the earlier version.
> Also rework the other checks to avoid potential underflow.
>
> Signed-off-by: Ross Lagerwall
> ---
> drivers/firmware/efi/cper.c | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index a7902fccdcfa..7cc18874b9d0 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -546,7 +546,7 @@ EXPORT_SYMBOL_GPL(cper_estatus_check_header);
> int cper_estatus_check(const struct acpi_hest_generic_status *estatus)
> {
> struct acpi_hest_generic_data *gdata;
> - unsigned int data_len, gedata_len;
> + unsigned int data_len, record_len;
> int rc;
>
> rc = cper_estatus_check_header(estatus);
> @@ -555,10 +555,12 @@ int cper_estatus_check(const struct
> acpi_hest_generic_status *estatus)
> data_len = estatus->data_length;
>
> apei_estatus_for_each_section(estatus, gdata) {
> - gedata_len = acpi_hest_get_error_length(gdata);
> - if (gedata_len > data_len - acpi_hest_get_size(gdata))
> + if (sizeof(struct acpi_hest_generic_data) > data_len)
> return -EINVAL;
< newline here.
Also, add a new line before the data_len assignment above, in the function.
> - data_len -= acpi_hest_get_record_size(gdata);
> + record_len = acpi_hest_get_record_size(gdata);
record_size so that it matches the function name it is used to compute
this.
Btw, trying to grok this code is making my head spin.
> + if (record_len > data_len)
> + return -EINVAL;
< newline here.
Btw, those checks in the loop you can abstract away into a separate
function so that you end up with something more readable like:
apei_estatus_for_each_section(estatus, gdata) {
record_size = check_hest_record_size(gdata, data_len);
if (!record_size)
return -EINVAL;
data_len -= record_size;
}
for example.
> + data_len -= record_len;
> }
> if (data_len)
> return -EINVAL;
> --
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH] acpi: bgrt: parse BGRT to obtain BMP address before it gets clobbered
On Wed, 23 Jan 2019 at 06:31, Dave Young wrote:
>
> Hi,
>
> On 01/22/19 at 04:06pm, Ard Biesheuvel wrote:
> > The bitmap left in the framebuffer by the firmware is described by an
> > ACPI table called "BGRT", which describes the size, pixel format and
> > the address of a BMP image in memory. While the BGRT ACPI table is
> > guaranteed to reside in a "ACPI reclaim" memory region, which is
> > never touched by Linux. The BMP image, however, typically resides
> > in EFI Boot Services Memory, which may have been overwritten by the
> > time the BGRT discovery routine runs.
>
> I vaguely remember boot service memory is reserved on X86, so this
> is an issue of arm only?
>
No both ARM and x86 are affected. Harry and/or Peter should be able to
comment on the details.
> >
> > So instead, drop the handling from the ACPI init code, and call the
> > BGRT parsing code immediately after going over the EFI configuration
> > table array, at which time no memory has been touched yet except for
> > the .data/.bss regions covered by the static kernel image.
> >
> > Unfortunately, this involves a non-trivial amount of ACPI entry
> > point and root table parsing, but we cannot rely on the normal
> > ACPI infrastructure yet this early in the boot.
> >
> > Also note that we cannot take the 'acpi_disabled' global variable
> > into account, since it may not have assumed the correct value yet
> > (on arm64, the default value is '1' which is overridden to '0' if
> > no DT description has been made available by the firmware)
> >
> > Cc: Peter Jones
> > Signed-off-by: Ard Biesheuvel
> > ---
> > arch/arm64/kernel/acpi.c| 2 -
> > arch/x86/kernel/acpi/boot.c | 2 -
> > drivers/acpi/bgrt.c | 6 --
> > drivers/firmware/efi/efi-bgrt.c | 80 ++--
> > drivers/firmware/efi/efi.c | 13
> > include/linux/efi-bgrt.h| 4 +-
> > 6 files changed, 89 insertions(+), 18 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> > index 44e3c351e1ea..7429a811f76d 100644
> > --- a/arch/arm64/kernel/acpi.c
> > +++ b/arch/arm64/kernel/acpi.c
> > @@ -230,8 +230,6 @@ void __init acpi_boot_table_init(void)
> > early_init_dt_scan_chosen_stdout();
> > } else {
> > acpi_parse_spcr(earlycon_acpi_spcr_enable, true);
> > - if (IS_ENABLED(CONFIG_ACPI_BGRT))
> > - acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
> > }
> > }
> >
> > diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> > index 2624de16cd7a..2d3535b62752 100644
> > --- a/arch/x86/kernel/acpi/boot.c
> > +++ b/arch/x86/kernel/acpi/boot.c
> > @@ -1633,8 +1633,6 @@ int __init acpi_boot_init(void)
> > acpi_process_madt();
> >
> > acpi_table_parse(ACPI_SIG_HPET, acpi_parse_hpet);
> > - if (IS_ENABLED(CONFIG_ACPI_BGRT))
> > - acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
> >
> > if (!acpi_noirq)
> > x86_init.pci.init = pci_acpi_init;
> > diff --git a/drivers/acpi/bgrt.c b/drivers/acpi/bgrt.c
> > index 75af78361ce5..048413e06898 100644
> > --- a/drivers/acpi/bgrt.c
> > +++ b/drivers/acpi/bgrt.c
> > @@ -81,12 +81,6 @@ static const struct attribute_group bgrt_attribute_group
> > = {
> > .bin_attrs = bgrt_bin_attributes,
> > };
> >
> > -int __init acpi_parse_bgrt(struct acpi_table_header *table)
> > -{
> > - efi_bgrt_init(table);
> > - return 0;
> > -}
> > -
> > static int __init bgrt_init(void)
> > {
> > int ret;
> > diff --git a/drivers/firmware/efi/efi-bgrt.c
> > b/drivers/firmware/efi/efi-bgrt.c
> > index b22ccfb0c991..8bd9b96942d2 100644
> > --- a/drivers/firmware/efi/efi-bgrt.c
> > +++ b/drivers/firmware/efi/efi-bgrt.c
> > @@ -27,24 +27,92 @@ struct bmp_header {
> > u32 size;
> > } __packed;
> >
> > -void __init efi_bgrt_init(struct acpi_table_header *table)
> > +void __init efi_bgrt_init(unsigned long rsdp_phys)
> > {
> > void *image;
> > struct bmp_header bmp_header;
> > struct acpi_table_bgrt *bgrt = &bgrt_tab;
> > + struct acpi_table_bgrt *table = NULL;
> > + struct acpi_table_rsdp *rsdp;
> > + struct acpi_table_header *hdr;
> > + u64 xsdt_phys;
> > + u32 rsdt_phys;
> > + unsigned int len;
> >
> > - if (acpi_disabled)
> > + if (!efi_enabled(EFI_MEMMAP))
> > return;
> >
> > - if (!efi_enabled(EFI_MEMMAP))
> > + /* map the root pointer table to find the xsdt/rsdt values */
> > + rsdp = early_memremap(rsdp_phys, sizeof(*rsdp));
> > + if (rsdp && ACPI_VALIDATE_RSDP_SIG(rsdp->signature)) {
> > + xsdt_phys = rsdp->xsdt_physical_address;
> > + rsdt_phys = rsdp->rsdt_physical_address;
> > + } else {
> > + WARN_ON(1);
> > + return;
> > + }
> > + early_memunmap(rsdp, sizeof(*rsdp));
> > +
> > + /* obtain the length of whichever table we will be using */
> > + hdr = early_memremap(xsdt_p
Re: [PATCH v9 11/26] efi: Let architectures decide the flags that should be saved/restored
On 21/01/2019 15:42, Ard Biesheuvel wrote:
> On Mon, 21 Jan 2019 at 16:34, Julien Thierry wrote:
>>
>> Currently, irqflags are saved before calling runtime services and
>> checked for mismatch on return.
>>
>> Provide a pair of overridable macros to save and restore (if needed) the
>> state that need to be preserved on return from a runtime service.
>> This allows to check for flags that are not necesarly related to
>> irqflags.
>>
>> Signed-off-by: Julien Thierry
>> Acked-by: Catalin Marinas
>> Cc: Ard Biesheuvel
>> Cc: linux-efi@vger.kernel.org
>> ---
>> drivers/firmware/efi/runtime-wrappers.c | 17 +++--
>> include/linux/efi.h | 5 +++--
>> 2 files changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/runtime-wrappers.c
>> b/drivers/firmware/efi/runtime-wrappers.c
>> index 8903b9c..2f4b68b 100644
>> --- a/drivers/firmware/efi/runtime-wrappers.c
>> +++ b/drivers/firmware/efi/runtime-wrappers.c
>> @@ -89,11 +89,24 @@
>> efi_rts_work.status;\
>> })
>>
>> +#ifndef arch_efi_save_flags
>> +#define arch_efi_save_flags(state_flags) local_save_flags(state_flags)
>> +#define arch_efi_restore_flags(state_flags)
>> local_irq_restore(state_flags)
>> +#endif
>> +
>> +inline unsigned long efi_call_virt_save_flags(void)
>
> If you drop the 'inline' here,
>
Sure, I'll drop it in the next version.
> Acked-by: Ard Biesheuvel
>
Thanks,
Julien
>> +{
>> + unsigned long flags;
>> +
>> + arch_efi_save_flags(flags);
>> + return flags;
>> +}
>> +
>> void efi_call_virt_check_flags(unsigned long flags, const char *call)
>> {
>> unsigned long cur_flags, mismatch;
>>
>> - local_save_flags(cur_flags);
>> + cur_flags = efi_call_virt_save_flags();
>>
>> mismatch = flags ^ cur_flags;
>> if (!WARN_ON_ONCE(mismatch & ARCH_EFI_IRQ_FLAGS_MASK))
>> @@ -102,7 +115,7 @@ void efi_call_virt_check_flags(unsigned long flags,
>> const char *call)
>> add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_NOW_UNRELIABLE);
>> pr_err_ratelimited(FW_BUG "IRQ flags corrupted (0x%08lx=>0x%08lx) by
>> EFI %s\n",
>>flags, cur_flags, call);
>> - local_irq_restore(flags);
>> + arch_efi_restore_flags(flags);
>> }
>>
>> /*
>> diff --git a/include/linux/efi.h b/include/linux/efi.h
>> index 45ff763..bd80b7e 100644
>> --- a/include/linux/efi.h
>> +++ b/include/linux/efi.h
>> @@ -1607,6 +1607,7 @@ efi_status_t efi_setup_gop(efi_system_table_t
>> *sys_table_arg,
>>
>> bool efi_runtime_disabled(void);
>> extern void efi_call_virt_check_flags(unsigned long flags, const char
>> *call);
>> +extern unsigned long efi_call_virt_save_flags(void);
>>
>> enum efi_secureboot_mode {
>> efi_secureboot_mode_unset,
>> @@ -1652,7 +1653,7 @@ enum efi_secureboot_mode {
>> \
>> arch_efi_call_virt_setup(); \
>> \
>> - local_save_flags(__flags); \
>> + __flags = efi_call_virt_save_flags(); \
>> __s = arch_efi_call_virt(p, f, args); \
>> efi_call_virt_check_flags(__flags, __stringify(f)); \
>> \
>> @@ -1667,7 +1668,7 @@ enum efi_secureboot_mode {
>> \
>> arch_efi_call_virt_setup(); \
>> \
>> - local_save_flags(__flags); \
>> + __flags = efi_call_virt_save_flags(); \
>> arch_efi_call_virt(p, f, args); \
>> efi_call_virt_check_flags(__flags, __stringify(f)); \
>> \
>> --
>> 1.9.1
>>
--
Julien Thierry
