Messages in this thread | | | Subject | Re: [PATCH 1/1] ACPI: fix acpi table use after free | From | George Kennedy <> | Date | Wed, 17 Mar 2021 18:28:27 -0400 |
| |
On 3/17/2021 4:14 PM, Rafael J. Wysocki wrote: > On Monday, March 15, 2021 5:19:29 PM CET Rafael J. Wysocki wrote: >> On Sun, Mar 14, 2021 at 8:00 PM Mike Rapoport <rppt@linux.ibm.com> wrote: >>> On Thu, Mar 11, 2021 at 04:36:31PM +0100, Rafael J. Wysocki wrote: >>>> On Wed, Mar 10, 2021 at 8:47 PM David Hildenbrand <david@redhat.com> wrote: >>>>>> There is some care that should be taken to make sure we get the order >>>>>> right, but I don't see a fundamental issue here. >>>> Me neither. >>>> >>>>>> If I understand correctly, Rafael's concern is about changing the parts of >>>>>> ACPICA that should be OS agnostic, so I think we just need another place to >>>>>> call memblock_reserve() rather than acpi_tb_install_table_with_override(). >>>> Something like this. >>>> >>>> There is also the problem that memblock_reserve() needs to be called >>>> for all of the tables early enough, which will require some reordering >>>> of the early init code. >>>> >>>>>> Since the reservation should be done early in x86::setup_arch() (and >>>>>> probably in arm64::setup_arch()) we might just have a function that parses >>>>>> table headers and reserves them, similarly to how we parse the tables >>>>>> during KASLR setup. >>>> Right. >>> I've looked at it a bit more and we do something like the patch below that >>> nearly duplicates acpi_tb_parse_root_table() which is not very nice. >> It looks to me that the code need not be duplicated (see below). >> >>> Besides, reserving ACPI tables early and then calling acpi_table_init() >>> (and acpi_tb_parse_root_table() again would mean doing the dance with >>> early_memremap() twice for no good reason. >> That'd be simply inefficient which is kind of acceptable to me to start with. >> >> And I changing the ACPICA code can be avoided at least initially, it >> by itself would be a good enough reason. >> >>> I believe the most effective way to deal with this would be to have a >>> function that does parsing, reservation and installs the tables supplied by >>> the firmware which can be called really early and then another function >>> that overrides tables if needed a some later point. >> I agree that this should be the direction to go into. > So maybe something like the patch below?
Do you want me to try it out in the failing setup?
George > > I'm not sure if acpi_boot_table_prepare() gets called early enough, though. > > Also this still may not play well with initrd-based table overrides. Erik, do > you have any insights here? > > And ia64 needs to be updated too. > > --- > arch/x86/kernel/acpi/boot.c | 12 +++++++++--- > arch/x86/kernel/setup.c | 3 +++ > drivers/acpi/tables.c | 24 +++++++++++++++++++++--- > include/linux/acpi.h | 9 +++++++-- > 4 files changed, 40 insertions(+), 8 deletions(-) > > Index: linux-pm/arch/x86/kernel/acpi/boot.c > =================================================================== > --- linux-pm.orig/arch/x86/kernel/acpi/boot.c > +++ linux-pm/arch/x86/kernel/acpi/boot.c > @@ -1541,7 +1541,7 @@ static const struct dmi_system_id acpi_d > * ... > */ > > -void __init acpi_boot_table_init(void) > +void __init acpi_boot_table_prepare(void) > { > dmi_check_system(acpi_dmi_table); > > @@ -1554,10 +1554,16 @@ void __init acpi_boot_table_init(void) > /* > * Initialize the ACPI boot-time table parser. > */ > - if (acpi_table_init()) { > + if (acpi_table_prepare()) > disable_acpi(); > +} > + > +void __init acpi_boot_table_init(void) > +{ > + if (acpi_disabled) > return; > - } > + > + acpi_table_init(); > > acpi_table_parse(ACPI_SIG_BOOT, acpi_parse_sbf); > > Index: linux-pm/arch/x86/kernel/setup.c > =================================================================== > --- linux-pm.orig/arch/x86/kernel/setup.c > +++ linux-pm/arch/x86/kernel/setup.c > @@ -1070,6 +1070,9 @@ void __init setup_arch(char **cmdline_p) > /* preallocate 4k for mptable mpc */ > e820__memblock_alloc_reserved_mpc_new(); > > + /* Look for ACPI tables and reserve memory occupied by them. */ > + acpi_boot_table_prepare(); > + > #ifdef CONFIG_X86_CHECK_BIOS_CORRUPTION > setup_bios_corruption_check(); > #endif > Index: linux-pm/include/linux/acpi.h > =================================================================== > --- linux-pm.orig/include/linux/acpi.h > +++ linux-pm/include/linux/acpi.h > @@ -222,11 +222,13 @@ void __iomem *__acpi_map_table(unsigned > void __acpi_unmap_table(void __iomem *map, unsigned long size); > int early_acpi_boot_init(void); > int acpi_boot_init (void); > +void acpi_boot_table_prepare (void); > void acpi_boot_table_init (void); > int acpi_mps_check (void); > int acpi_numa_init (void); > > -int acpi_table_init (void); > +int acpi_table_prepare (void); > +void acpi_table_init (void); > int acpi_table_parse(char *id, acpi_tbl_table_handler handler); > int __init acpi_table_parse_entries(char *id, unsigned long table_size, > int entry_id, > @@ -814,9 +816,12 @@ static inline int acpi_boot_init(void) > return 0; > } > > +static inline void acpi_boot_table_prepare(void) > +{ > +} > + > static inline void acpi_boot_table_init(void) > { > - return; > } > > static inline int acpi_mps_check(void) > Index: linux-pm/drivers/acpi/tables.c > =================================================================== > --- linux-pm.orig/drivers/acpi/tables.c > +++ linux-pm/drivers/acpi/tables.c > @@ -788,9 +788,10 @@ acpi_status acpi_os_table_override(struc > * result: sdt_entry[] is initialized > */ > > -int __init acpi_table_init(void) > +int __init acpi_table_prepare(void) > { > acpi_status status; > + int i; > > if (acpi_verify_table_checksum) { > pr_info("Early table checksum verification enabled\n"); > @@ -803,12 +804,29 @@ int __init acpi_table_init(void) > status = acpi_initialize_tables(initial_tables, ACPI_MAX_TABLES, 0); > if (ACPI_FAILURE(status)) > return -EINVAL; > - acpi_table_initrd_scan(); > > - check_multiple_madt(); > + for (i = 0; i < ACPI_MAX_TABLES; i++) { > + struct acpi_table_desc *table_desc = &initial_tables[i]; > + > + if (!table_desc->address || !table_desc->length) > + break; > + > + pr_info("Reserving %4s table memory at [0x%llx - 0x%llx]\n", > + table_desc->signature.ascii, table_desc->address, > + table_desc->address + table_desc->length - 1); > + > + memblock_reserve(table_desc->address, table_desc->length); > + } > + > return 0; > } > > +void __init acpi_table_init(void) > +{ > + acpi_table_initrd_scan(); > + check_multiple_madt(); > +} > + > static int __init acpi_parse_apic_instance(char *str) > { > if (!str) > > >
| |