On Fri, Mar 5, 2021 at 12:14 AM George Kennedy
<george.kennedy@xxxxxxxxxx> wrote:
>
> Hello Rafael,
>
> On 3/4/2021 7:14 AM, Rafael J. Wysocki wrote:
> > On Thu, Mar 4, 2021 at 2:22 AM George Kennedy <george.kennedy@xxxxxxxxxx> wrote:
> >> Since commit 7fef431be9c9 ("mm/page_alloc: place pages to tail
> >> in __free_pages_core()") the following use after free occurs
> >> intermittently when acpi tables are accessed.
> >>
> >> BUG: KASAN: use-after-free in ibft_init+0x134/0xc49
> >> Read of size 4 at addr ffff8880be453004 by task swapper/0/1
> >> CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.12.0-rc1-7a7fd0d #1
> >> Call Trace:
> >> dump_stack+0xf6/0x158
> >> print_address_description.constprop.9+0x41/0x60
> >> kasan_report.cold.14+0x7b/0xd4
> >> __asan_report_load_n_noabort+0xf/0x20
> >> ibft_init+0x134/0xc49
> >> do_one_initcall+0xc4/0x3e0
> >> kernel_init_freeable+0x5af/0x66b
> >> kernel_init+0x16/0x1d0
> >> ret_from_fork+0x22/0x30
> >>
> >> ACPI tables mapped via kmap() do not have their mapped pages
> >> reserved and the pages can be "stolen" by the buddy allocator.
> >>
> > What do you mean by this?
>>
> The ibft table, for example, is mapped in via acpi_map() and kmap(). The
> page for the ibft table is not reserved, so it can end up on the freelist.
You appear to be saying that it is not sufficient to kmap() a page in
order to use it safely. It is also necessary to reserve it upfront,
for example with the help of memblock_reserve(). Is that correct? If
so, is there an alternative way to reserve a page frame?
> >
> >> Use memblock_reserve() to reserve all the ACPI table pages.
> > How is this going to help?
> If the ibft table page is not reserved, it will end up on the freelist
> and potentially be allocated before ibft_init() is called.
>
> I believe this is the call that causes the ibft table page (in this case
> pfn=0xbe453) to end up on the freelist:
>
> memmap_init_range: size=bd49b, nid=0, zone=1, start_pfn=1000,
> zone_end_pfn=100000
David, is commit 7fef431be9c9 related to this and if so, then how?
> [ 0.477319] memmap_init_range+0x33b/0x4e2
> [ 0.479053] memmap_init_zone+0x1e0/0x243
> [ 0.485276] free_area_init_node+0xa4e/0xac5
> [ 0.498242] free_area_init+0xf4a/0x107a
> [ 0.509958] zone_sizes_init+0xd9/0x111
> [ 0.511731] paging_init+0x4a/0x4c
> [ 0.512417] setup_arch+0x14f8/0x1758
> [ 0.519193] start_kernel+0x6c/0x46f
> [ 0.519921] x86_64_start_reservations+0x37/0x39
> [ 0.520847] x86_64_start_kernel+0x7b/0x7e
> [ 0.521666] secondary_startup_64_no_verify+0xb0/0xbb
>
> >
> >> Signed-off-by: George Kennedy <george.kennedy@xxxxxxxxxx>
> >> ---
> >> arch/x86/kernel/setup.c | 3 +--
> >> drivers/acpi/acpica/tbinstal.c | 4 ++++
> >> 2 files changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> >> index d883176..97deea3 100644
> >> --- a/arch/x86/kernel/setup.c
> >> +++ b/arch/x86/kernel/setup.c
> >> @@ -1046,6 +1046,7 @@ void __init setup_arch(char **cmdline_p)
> >> cleanup_highmap();
> >>
> >> memblock_set_current_limit(ISA_END_ADDRESS);
> >> + acpi_boot_table_init();
> > This cannot be moved before the acpi_table_upgrade() invocation AFAICS.
> >
> > Why exactly do you want to move it?
>
> Want to make sure there are slots for memblock_reserve() to be able to
> reserve the page.
Well, that may not require reordering the initialization this way.
> >> e820__memblock_setup();
> >>
> >> /*
> >> @@ -1139,8 +1140,6 @@ void __init setup_arch(char **cmdline_p)
> >> /*
> >> * Parse the ACPI tables for possible boot-time SMP configuration.
> >> */
> >> - acpi_boot_table_init();
> >> -
> >> early_acpi_boot_init();
> >>
> >> initmem_init();
> >> diff --git a/drivers/acpi/acpica/tbinstal.c b/drivers/acpi/acpica/tbinstal.c
> >> index 8d1e5b5..4e32b22 100644
> >> --- a/drivers/acpi/acpica/tbinstal.c
> >> +++ b/drivers/acpi/acpica/tbinstal.c
> >> @@ -8,6 +8,7 @@
> >> *****************************************************************************/
> >>
> >> #include <acpi/acpi.h>
> >> +#include <linux/memblock.h>
> >> #include "accommon.h"
> >> #include "actables.h"
> >>
> >> @@ -58,6 +59,9 @@
> >> new_table_desc->flags,
> >> new_table_desc->pointer);
> >>
> >> + memblock_reserve(new_table_desc->address,
> >> + PAGE_ALIGN(new_table_desc->pointer->length));
> >> +
> > Why do you want to do this here in the first place?
>
> If there is a better place to do it, I can move the memblock_reserve()
> there. The memblock_reserve() cannot be done from the ibft code - it's
> too late - the ibft table page has already ended up on the freelist by
> the time ibft_init() is called.
I see.
> >
> > Things like that cannot be done in the ACPICA code in general.
>
> Can you recommend a better place to do the memblock_reserve() from?
Maybe. I need to understand the problem better, though.
Thanks!
