All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/1] ACPI: fix acpi table use after free
@ 2021-03-03 20:09 George Kennedy
  2021-03-04 12:14   ` Rafael J. Wysocki
  0 siblings, 1 reply; 46+ messages in thread
From: George Kennedy @ 2021-03-03 20:09 UTC (permalink / raw)
  To: robert.moore, erik.kaneda, rafael.j.wysocki, lenb, linux-acpi,
	devel, linux-kernel, rppt, konrad.wilk, dan.carpenter,
	dhaval.giani
  Cc: george.kennedy

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.
Use memblock_reserve() to reserve all the ACPI table pages.

Signed-off-by: George Kennedy <george.kennedy@oracle.com>
---
 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();
 	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));
+
 	acpi_tb_print_table_header(new_table_desc->address,
 				   new_table_desc->pointer);
 
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread
* [Devel] Re: [PATCH 1/1] ACPI: fix acpi table use after free
  2021-03-15 16:19                         ` Rafael J. Wysocki
@ 2021-03-15 18:05 ` Rafael J. Wysocki
  -1 siblings, 0 replies; 46+ messages in thread
From: Rafael J. Wysocki @ 2021-03-15 18:05 UTC (permalink / raw)
  To: devel

[-- Attachment #1: Type: text/plain, Size: 3125 bytes --]

On Mon, Mar 15, 2021 at 5:19 PM Rafael J. Wysocki <rafael(a)kernel.org> wrote:
>
> On Sun, Mar 14, 2021 at 8:00 PM Mike Rapoport <rppt(a)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(a)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.
>
> However, it looks to me that something like the following could be
> done to start with:
>
> (a) Make __acpi_map_table() call memblock_reserve() in addition to
> early_memremap().
>
> My assumption here is that the memblock_reserve() will simply be
> ignored if it is called too late.
>
> (b) Introduce acpi_reserve_tables() as something like
>
> void __init acpi_table_reserve(void)
> {
>         acpi_initialize_tables(initial_tables, ACPI_MAX_TABLES, 0);
> }
>
> Because initial_tables is passed to acpi_initialize_tables() above and
> allow_resize is 0, the array used by it will simply get overwritten
> when acpi_table_init() gets called.
>
> (c) Make setup_arch() call acpi_table_reserve() like in the original
> patch from George.
>
> Would that work?

Well, that doesn't work, so more digging ...

^ permalink raw reply	[flat|nested] 46+ messages in thread
* [Devel] Re: [PATCH 1/1] ACPI: fix acpi table use after free
  2021-03-17 22:28                           ` George Kennedy
@ 2021-03-18 15:42 ` Rafael J. Wysocki
  -1 siblings, 0 replies; 46+ messages in thread
From: Rafael J. Wysocki @ 2021-03-18 15:42 UTC (permalink / raw)
  To: devel

[-- Attachment #1: Type: text/plain, Size: 2538 bytes --]

On Wed, Mar 17, 2021 at 11:28 PM George Kennedy
<george.kennedy(a)oracle.com> wrote:
>
>
>
> 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(a)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(a)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?

Yes, please!

^ permalink raw reply	[flat|nested] 46+ messages in thread
* [Devel] Re: [PATCH 1/1] ACPI: fix acpi table use after free
  2021-03-20  8:25                                 ` Mike Rapoport
@ 2021-03-22 16:57 ` Rafael J. Wysocki
  0 siblings, 0 replies; 46+ messages in thread
From: Rafael J. Wysocki @ 2021-03-22 16:57 UTC (permalink / raw)
  To: devel

[-- Attachment #1: Type: text/plain, Size: 4481 bytes --]

On Sat, Mar 20, 2021 at 9:25 AM Mike Rapoport <rppt(a)linux.ibm.com> wrote:
>
> On Thu, Mar 18, 2021 at 04:22:37PM +0100, Rafael J. Wysocki wrote:
> > On Thu, Mar 18, 2021 at 11:50 AM Rafael J. Wysocki <rafael(a)kernel.org> wrote:
> > >
> > > On Thu, Mar 18, 2021 at 8:25 AM Mike Rapoport <rppt(a)linux.ibm.com> wrote:
> > > >
> > > > On Wed, Mar 17, 2021 at 09:14:37PM +0100, 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(a)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(a)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?
> > > > >
> > > > > I'm not sure if acpi_boot_table_prepare() gets called early enough, though.
> > > >
> > > > To be 100% safe it should be called before e820__memblock_setup().
> > >
> > > OK
> >
> > Well, that said, reserve_bios_regions() doesn't seem to have concerns
> > like this and I'm not sure why ACPI tables should be reserved before
> > this runs.  That applies to efi_reserve_boot_services() too.
> >
> > I can put the new call before e820__memblock_alloc_reserved_mpc_new(),
> > but I'm not sure why to put it before efi_reserve_boot_services(),
> > say?
>
> The general idea is to reserve all the memory used by the firmware before
> memblock allocations are possible, i.e. before e820__memblock_setup().
> Currently this is not the case, but it does not make it more correct.

I see.

> Theoretically, it is possible that reserve_bios_regions() will cause a
> memory allocation and the allocated memory will be exactly at the area
> where ACPI tables reside.
>
> In practice I believe this is very unlikely, but who knows.
>
> Another advantage of having ACPI tables handy by the time we do the memory
> detection is that we will be able to SRAT earlier and simplify NUMA
> initialization.

OK, fair enough.

^ permalink raw reply	[flat|nested] 46+ messages in thread
end of thread, other threads:[~2021-03-24 15:46 UTC | newest]

Thread overview: 46+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-03-03 20:09 [PATCH 1/1] ACPI: fix acpi table use after free George Kennedy
2021-03-04 12:14 ` [Devel] " Rafael J. Wysocki
2021-03-04 12:14   ` Rafael J. Wysocki
2021-03-04 23:14   ` George Kennedy
2021-03-05 13:30     ` [Devel] " Rafael J. Wysocki
2021-03-05 13:30       ` Rafael J. Wysocki
2021-03-05 13:40       ` David Hildenbrand
2021-03-05 15:24         ` George Kennedy
2021-03-10 18:39         ` [Devel] " Rafael J. Wysocki
2021-03-10 18:39           ` Rafael J. Wysocki
2021-03-10 18:54           ` [Devel] " Rafael J. Wysocki
2021-03-10 18:54             ` Rafael J. Wysocki
2021-03-10 19:10             ` David Hildenbrand
2021-03-10 19:38               ` Mike Rapoport
2021-03-10 19:47                 ` David Hildenbrand
2021-03-11 15:36                   ` [Devel] " Rafael J. Wysocki
2021-03-11 15:36                     ` Rafael J. Wysocki
2021-03-14 18:59                     ` Mike Rapoport
2021-03-15 16:19                       ` [Devel] " Rafael J. Wysocki
2021-03-15 16:19                         ` Rafael J. Wysocki
2021-03-17 20:14                         ` [Devel] " Rafael J. Wysocki
2021-03-17 20:14                           ` Rafael J. Wysocki
2021-03-17 22:28                           ` George Kennedy
2021-03-18  7:25                           ` Mike Rapoport
2021-03-18 10:50                             ` [Devel] " Rafael J. Wysocki
2021-03-18 10:50                               ` Rafael J. Wysocki
2021-03-18 15:22                               ` [Devel] " Rafael J. Wysocki
2021-03-18 15:22                                 ` Rafael J. Wysocki
2021-03-20  8:25                                 ` Mike Rapoport
2021-03-23 19:26                                   ` [PATCH] ACPI: tables: x86: Reserve memory occupied by ACPI tables Rafael J. Wysocki
2021-03-24  8:24                                     ` Mike Rapoport
2021-03-24 13:27                                       ` Rafael J. Wysocki
2021-03-24 13:49                                         ` George Kennedy
2021-03-24 15:42                                         ` George Kennedy
2021-03-24 15:44                                           ` Rafael J. Wysocki
2021-03-07  7:46       ` [PATCH 1/1] ACPI: fix acpi table use after free Mike Rapoport
2021-03-09 17:54         ` Mike Rapoport
2021-03-09 18:29           ` [Devel] " Rafael J. Wysocki
2021-03-09 18:29             ` Rafael J. Wysocki
2021-03-09 20:16             ` Mike Rapoport
  -- strict thread matches above, loose matches on Subject: below --
2021-03-15 18:05 [Devel] " Rafael J. Wysocki
2021-03-15 18:05 ` Rafael J. Wysocki
2021-03-18 15:42 [Devel] " Rafael J. Wysocki
2021-03-18 15:42 ` Rafael J. Wysocki
2021-03-22 16:57 [Devel] " Rafael J. Wysocki
2021-03-22 16:57 ` Rafael J. Wysocki

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.