* [Devel] Re: [PATCH 1/1] ACPI: fix acpi table use after free
@ 2021-03-17 20:14 ` Rafael J. Wysocki
0 siblings, 0 replies; 46+ messages in thread
From: Rafael J. Wysocki @ 2021-03-17 20:14 UTC (permalink / raw)
To: devel
[-- Attachment #1: Type: text/plain, Size: 6515 bytes --]
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.
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)
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 1/1] ACPI: fix acpi table use after free
2021-03-17 20:14 ` [Devel] " Rafael J. Wysocki
(?)
@ 2021-03-17 22:28 ` George Kennedy
2021-03-18 15:42 ` [Devel] " Rafael J. Wysocki
-1 siblings, 1 reply; 46+ messages in thread
From: George Kennedy @ 2021-03-17 22:28 UTC (permalink / raw)
To: Rafael J. Wysocki, Mike Rapoport, Erik Kaneda
Cc: Rafael J. Wysocki, David Hildenbrand, Robert Moore,
Rafael Wysocki, Len Brown, ACPI Devel Maling List,
open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
Linux Kernel Mailing List, Konrad Rzeszutek Wilk, Dan Carpenter,
Dhaval Giani, Andrew Morton, Vlastimil Babka, Oscar Salvador,
Wei Yang, Pankaj Gupta, Michal Hocko
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)
>
>
>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 1/1] ACPI: fix acpi table use after free
@ 2021-03-18 15:42 ` Rafael J. Wysocki
0 siblings, 0 replies; 46+ messages in thread
From: Rafael J. Wysocki @ 2021-03-18 15:42 UTC (permalink / raw)
To: George Kennedy
Cc: Rafael J. Wysocki, Mike Rapoport, Erik Kaneda, Rafael J. Wysocki,
David Hildenbrand, Robert Moore, Rafael Wysocki, Len Brown,
ACPI Devel Maling List,
open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
Linux Kernel Mailing List, Konrad Rzeszutek Wilk, Dan Carpenter,
Dhaval Giani, Andrew Morton, Vlastimil Babka, Oscar Salvador,
Wei Yang, Pankaj Gupta, Michal Hocko
On Wed, Mar 17, 2021 at 11:28 PM George Kennedy
<george.kennedy@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@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?
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-18 15:42 ` Rafael J. Wysocki
0 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
* Re: [PATCH 1/1] ACPI: fix acpi table use after free
2021-03-17 20:14 ` [Devel] " Rafael J. Wysocki
(?)
(?)
@ 2021-03-18 7:25 ` Mike Rapoport
2021-03-18 10:50 ` [Devel] " Rafael J. Wysocki
-1 siblings, 1 reply; 46+ messages in thread
From: Mike Rapoport @ 2021-03-18 7:25 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Erik Kaneda, Rafael J. Wysocki, David Hildenbrand,
George Kennedy, Robert Moore, Rafael Wysocki, Len Brown,
ACPI Devel Maling List,
open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
Linux Kernel Mailing List, Konrad Rzeszutek Wilk, Dan Carpenter,
Dhaval Giani, Andrew Morton, Vlastimil Babka, Oscar Salvador,
Wei Yang, Pankaj Gupta, Michal Hocko
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@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?
>
> 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(). It is
possible to call memblock_reserve() at any time, even before the actual
memory is detected as long as all reservations fit into the static array
that currently has 128 entries on x86.
As e820__memblock_setup() essentially enables memblock allocations,
theoretically the memory occupied by ACPI tables can be allocated even in
x86::setup_arch() unless it is reserved before e820__memblock_setup().
> 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.
I think arm64 as well.
> ---
> 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);
Not related to this patch, but it feels to me like there are too many
acpi_boot_something() :)
> 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)
>
>
>
--
Sincerely yours,
Mike.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 1/1] ACPI: fix acpi table use after free
@ 2021-03-18 10:50 ` Rafael J. Wysocki
0 siblings, 0 replies; 46+ messages in thread
From: Rafael J. Wysocki @ 2021-03-18 10:50 UTC (permalink / raw)
To: Mike Rapoport
Cc: Rafael J. Wysocki, Erik Kaneda, Rafael J. Wysocki,
David Hildenbrand, George Kennedy, Robert Moore, Rafael Wysocki,
Len Brown, ACPI Devel Maling List,
open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
Linux Kernel Mailing List, Konrad Rzeszutek Wilk, Dan Carpenter,
Dhaval Giani, Andrew Morton, Vlastimil Babka, Oscar Salvador,
Wei Yang, Pankaj Gupta, Michal Hocko
On Thu, Mar 18, 2021 at 8:25 AM Mike Rapoport <rppt@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@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?
> >
> > 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
> It is possible to call memblock_reserve() at any time, even before the actual
> memory is detected as long as all reservations fit into the static array
> that currently has 128 entries on x86.
>
> As e820__memblock_setup() essentially enables memblock allocations,
> theoretically the memory occupied by ACPI tables can be allocated even in
> x86::setup_arch() unless it is reserved before e820__memblock_setup().
>
> > 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.
>
> I think arm64 as well.
Right.
> > ---
> > 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);
>
> Not related to this patch, but it feels to me like there are too many
> acpi_boot_something() :)
Well, there was one initially, but it has been split for a few times
due to ordering issues similar to the one at hand.
It could be cleaned up I suppose, though.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Devel] Re: [PATCH 1/1] ACPI: fix acpi table use after free
@ 2021-03-18 10:50 ` Rafael J. Wysocki
0 siblings, 0 replies; 46+ messages in thread
From: Rafael J. Wysocki @ 2021-03-18 10:50 UTC (permalink / raw)
To: devel
[-- Attachment #1: Type: text/plain, Size: 6085 bytes --]
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
> It is possible to call memblock_reserve() at any time, even before the actual
> memory is detected as long as all reservations fit into the static array
> that currently has 128 entries on x86.
>
> As e820__memblock_setup() essentially enables memblock allocations,
> theoretically the memory occupied by ACPI tables can be allocated even in
> x86::setup_arch() unless it is reserved before e820__memblock_setup().
>
> > 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.
>
> I think arm64 as well.
Right.
> > ---
> > 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);
>
> Not related to this patch, but it feels to me like there are too many
> acpi_boot_something() :)
Well, there was one initially, but it has been split for a few times
due to ordering issues similar to the one at hand.
It could be cleaned up I suppose, though.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 1/1] ACPI: fix acpi table use after free
@ 2021-03-18 15:22 ` Rafael J. Wysocki
0 siblings, 0 replies; 46+ messages in thread
From: Rafael J. Wysocki @ 2021-03-18 15:22 UTC (permalink / raw)
To: Mike Rapoport
Cc: Rafael J. Wysocki, Erik Kaneda, David Hildenbrand,
George Kennedy, Robert Moore, Rafael Wysocki, Len Brown,
ACPI Devel Maling List,
open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
Linux Kernel Mailing List, Konrad Rzeszutek Wilk, Dan Carpenter,
Dhaval Giani, Andrew Morton, Vlastimil Babka, Oscar Salvador,
Wei Yang, Pankaj Gupta, Michal Hocko
On Thu, Mar 18, 2021 at 11:50 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Mar 18, 2021 at 8:25 AM Mike Rapoport <rppt@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@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?
> > >
> > > 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?
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Devel] Re: [PATCH 1/1] ACPI: fix acpi table use after free
@ 2021-03-18 15:22 ` Rafael J. Wysocki
0 siblings, 0 replies; 46+ messages in thread
From: Rafael J. Wysocki @ 2021-03-18 15:22 UTC (permalink / raw)
To: devel
[-- Attachment #1: Type: text/plain, Size: 3384 bytes --]
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?
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 1/1] ACPI: fix acpi table use after free
2021-03-18 15:22 ` [Devel] " Rafael J. Wysocki
(?)
@ 2021-03-20 8:25 ` Mike Rapoport
2021-03-22 16:57 ` [Devel] " Rafael J. Wysocki
2021-03-23 19:26 ` [PATCH] ACPI: tables: x86: Reserve memory occupied by ACPI tables Rafael J. Wysocki
-1 siblings, 2 replies; 46+ messages in thread
From: Mike Rapoport @ 2021-03-20 8:25 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, Erik Kaneda, David Hildenbrand,
George Kennedy, Robert Moore, Rafael Wysocki, Len Brown,
ACPI Devel Maling List,
open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
Linux Kernel Mailing List, Konrad Rzeszutek Wilk, Dan Carpenter,
Dhaval Giani, Andrew Morton, Vlastimil Babka, Oscar Salvador,
Wei Yang, Pankaj Gupta, Michal Hocko
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@kernel.org> wrote:
> >
> > On Thu, Mar 18, 2021 at 8:25 AM Mike Rapoport <rppt@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@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?
> > > >
> > > > 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.
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.
--
Sincerely yours,
Mike.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 1/1] ACPI: fix acpi table use after free
@ 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: Mike Rapoport
Cc: Rafael J. Wysocki, Rafael J. Wysocki, Erik Kaneda,
David Hildenbrand, George Kennedy, Robert Moore, Rafael Wysocki,
Len Brown, ACPI Devel Maling List,
open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
Linux Kernel Mailing List, Konrad Rzeszutek Wilk, Dan Carpenter,
Dhaval Giani, Andrew Morton, Vlastimil Babka, Oscar Salvador,
Wei Yang, Pankaj Gupta, Michal Hocko
On Sat, Mar 20, 2021 at 9:25 AM Mike Rapoport <rppt@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@kernel.org> wrote:
> > >
> > > On Thu, Mar 18, 2021 at 8:25 AM Mike Rapoport <rppt@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@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?
> > > > >
> > > > > 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
* [Devel] Re: [PATCH 1/1] ACPI: fix acpi table use after free
@ 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
* [PATCH] ACPI: tables: x86: Reserve memory occupied by ACPI tables
2021-03-20 8:25 ` Mike Rapoport
2021-03-22 16:57 ` [Devel] " Rafael J. Wysocki
@ 2021-03-23 19:26 ` Rafael J. Wysocki
2021-03-24 8:24 ` Mike Rapoport
1 sibling, 1 reply; 46+ messages in thread
From: Rafael J. Wysocki @ 2021-03-23 19:26 UTC (permalink / raw)
To: ACPI Devel Maling List
Cc: Mike Rapoport, Rafael J. Wysocki, Rafael J. Wysocki, Erik Kaneda,
David Hildenbrand, George Kennedy, Robert Moore, Rafael Wysocki,
Len Brown, Linux Kernel Mailing List, Konrad Rzeszutek Wilk,
Dan Carpenter, Dhaval Giani, Andrew Morton, Vlastimil Babka,
Oscar Salvador, Wei Yang, Pankaj Gupta, Michal Hocko,
x86 Maintainers
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
The following problem has been reported by 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.
Apparently, on the affected system, the ACPI table in question is
not located in "reserved" memory, like ACPI NVS or ACPI Data, that
will not be used by the buddy allocator, so the memory occupied by
that table has to be explicitly reserved to prevent the buddy
allocator from using it.
In order to address this problem, rearrange the initialization of the
ACPI tables on x86 to locate the initial tables earlier and reserve
the memory occupied by them.
The other architectures using ACPI should not be affected by this
change.
Link: https://lore.kernel.org/linux-acpi/1614802160-29362-1-git-send-email-george.kennedy@oracle.com/
Reported-by: George Kennedy <george.kennedy@oracle.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
arch/x86/kernel/acpi/boot.c | 25 ++++++++++++-------------
arch/x86/kernel/setup.c | 8 +++-----
drivers/acpi/tables.c | 42 +++++++++++++++++++++++++++++++++++++++---
include/linux/acpi.h | 9 ++++++++-
4 files changed, 62 insertions(+), 22 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
@@ -1554,10 +1554,18 @@ void __init acpi_boot_table_init(void)
/*
* Initialize the ACPI boot-time table parser.
*/
- if (acpi_table_init()) {
+ if (acpi_locate_initial_tables())
disable_acpi();
- return;
- }
+ else
+ acpi_reserve_initial_tables();
+}
+
+int __init early_acpi_boot_init(void)
+{
+ if (acpi_disabled)
+ return 1;
+
+ acpi_table_init_complete();
acpi_table_parse(ACPI_SIG_BOOT, acpi_parse_sbf);
@@ -1570,18 +1578,9 @@ void __init acpi_boot_table_init(void)
} else {
printk(KERN_WARNING PREFIX "Disabling ACPI support\n");
disable_acpi();
- return;
+ return 1;
}
}
-}
-
-int __init early_acpi_boot_init(void)
-{
- /*
- * If acpi_disabled, bail out
- */
- if (acpi_disabled)
- return 1;
/*
* Process the Multiple APIC Description Table (MADT), if present
Index: linux-pm/arch/x86/kernel/setup.c
===================================================================
--- linux-pm.orig/arch/x86/kernel/setup.c
+++ linux-pm/arch/x86/kernel/setup.c
@@ -1045,6 +1045,9 @@ void __init setup_arch(char **cmdline_p)
cleanup_highmap();
+ /* Look for ACPI tables and reserve memory occupied by them. */
+ acpi_boot_table_init();
+
memblock_set_current_limit(ISA_END_ADDRESS);
e820__memblock_setup();
@@ -1136,11 +1139,6 @@ void __init setup_arch(char **cmdline_p)
early_platform_quirks();
- /*
- * Parse the ACPI tables for possible boot-time SMP configuration.
- */
- acpi_boot_table_init();
-
early_acpi_boot_init();
initmem_init();
Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -222,10 +222,14 @@ 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_locate_initial_tables (void);
+void acpi_reserve_initial_tables (void);
+void acpi_table_init_complete (void);
int 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,
@@ -814,9 +818,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
@@ -780,7 +780,7 @@ acpi_status acpi_os_table_override(struc
}
/*
- * acpi_table_init()
+ * acpi_locate_initial_tables()
*
* find RSDP, find and checksum SDT/XSDT.
* checksum all tables, print SDT/XSDT
@@ -788,7 +788,7 @@ acpi_status acpi_os_table_override(struc
* result: sdt_entry[] is initialized
*/
-int __init acpi_table_init(void)
+int __init acpi_locate_initial_tables(void)
{
acpi_status status;
@@ -803,9 +803,45 @@ 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();
+ return 0;
+}
+
+void __init acpi_reserve_initial_tables(void)
+{
+ int i;
+
+ for (i = 0; i < ACPI_MAX_TABLES; i++) {
+ struct acpi_table_desc *table_desc = &initial_tables[i];
+ u64 start = table_desc->address;
+ u64 size = table_desc->length;
+
+ if (!start || !size)
+ break;
+
+ pr_info("Reserving %4s table memory at [mem 0x%llx-0x%llx]\n",
+ table_desc->signature.ascii, start, start + size - 1);
+
+ memblock_reserve(start, size);
+ }
+}
+
+void __init acpi_table_init_complete(void)
+{
+ acpi_table_initrd_scan();
check_multiple_madt();
+}
+
+int __init acpi_table_init(void)
+{
+ int ret;
+
+ ret = acpi_locate_initial_tables();
+ if (ret)
+ return ret;
+
+ acpi_table_init_complete();
+
return 0;
}
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH] ACPI: tables: x86: Reserve memory occupied by ACPI tables
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
0 siblings, 1 reply; 46+ messages in thread
From: Mike Rapoport @ 2021-03-24 8:24 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: ACPI Devel Maling List, Rafael J. Wysocki, Erik Kaneda,
David Hildenbrand, George Kennedy, Robert Moore, Rafael Wysocki,
Len Brown, Linux Kernel Mailing List, Konrad Rzeszutek Wilk,
Dan Carpenter, Dhaval Giani, Andrew Morton, Vlastimil Babka,
Oscar Salvador, Wei Yang, Pankaj Gupta, Michal Hocko,
x86 Maintainers
On Tue, Mar 23, 2021 at 08:26:52PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The following problem has been reported by 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.
>
> Apparently, on the affected system, the ACPI table in question is
> not located in "reserved" memory, like ACPI NVS or ACPI Data, that
> will not be used by the buddy allocator, so the memory occupied by
> that table has to be explicitly reserved to prevent the buddy
> allocator from using it.
>
> In order to address this problem, rearrange the initialization of the
> ACPI tables on x86 to locate the initial tables earlier and reserve
> the memory occupied by them.
>
> The other architectures using ACPI should not be affected by this
> change.
>
> Link: https://lore.kernel.org/linux-acpi/1614802160-29362-1-git-send-email-george.kennedy@oracle.com/
> Reported-by: George Kennedy <george.kennedy@oracle.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
FWIW:
Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>
> ---
> arch/x86/kernel/acpi/boot.c | 25 ++++++++++++-------------
> arch/x86/kernel/setup.c | 8 +++-----
> drivers/acpi/tables.c | 42 +++++++++++++++++++++++++++++++++++++++---
> include/linux/acpi.h | 9 ++++++++-
> 4 files changed, 62 insertions(+), 22 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
> @@ -1554,10 +1554,18 @@ void __init acpi_boot_table_init(void)
> /*
> * Initialize the ACPI boot-time table parser.
> */
> - if (acpi_table_init()) {
> + if (acpi_locate_initial_tables())
> disable_acpi();
> - return;
> - }
> + else
> + acpi_reserve_initial_tables();
> +}
> +
> +int __init early_acpi_boot_init(void)
> +{
> + if (acpi_disabled)
> + return 1;
> +
> + acpi_table_init_complete();
>
> acpi_table_parse(ACPI_SIG_BOOT, acpi_parse_sbf);
>
> @@ -1570,18 +1578,9 @@ void __init acpi_boot_table_init(void)
> } else {
> printk(KERN_WARNING PREFIX "Disabling ACPI support\n");
> disable_acpi();
> - return;
> + return 1;
> }
> }
> -}
> -
> -int __init early_acpi_boot_init(void)
> -{
> - /*
> - * If acpi_disabled, bail out
> - */
> - if (acpi_disabled)
> - return 1;
>
> /*
> * Process the Multiple APIC Description Table (MADT), if present
> Index: linux-pm/arch/x86/kernel/setup.c
> ===================================================================
> --- linux-pm.orig/arch/x86/kernel/setup.c
> +++ linux-pm/arch/x86/kernel/setup.c
> @@ -1045,6 +1045,9 @@ void __init setup_arch(char **cmdline_p)
>
> cleanup_highmap();
>
> + /* Look for ACPI tables and reserve memory occupied by them. */
> + acpi_boot_table_init();
> +
> memblock_set_current_limit(ISA_END_ADDRESS);
> e820__memblock_setup();
>
> @@ -1136,11 +1139,6 @@ void __init setup_arch(char **cmdline_p)
>
> early_platform_quirks();
>
> - /*
> - * Parse the ACPI tables for possible boot-time SMP configuration.
> - */
> - acpi_boot_table_init();
> -
> early_acpi_boot_init();
>
> initmem_init();
> Index: linux-pm/include/linux/acpi.h
> ===================================================================
> --- linux-pm.orig/include/linux/acpi.h
> +++ linux-pm/include/linux/acpi.h
> @@ -222,10 +222,14 @@ 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_locate_initial_tables (void);
> +void acpi_reserve_initial_tables (void);
> +void acpi_table_init_complete (void);
> int 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,
> @@ -814,9 +818,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
> @@ -780,7 +780,7 @@ acpi_status acpi_os_table_override(struc
> }
>
> /*
> - * acpi_table_init()
> + * acpi_locate_initial_tables()
> *
> * find RSDP, find and checksum SDT/XSDT.
> * checksum all tables, print SDT/XSDT
> @@ -788,7 +788,7 @@ acpi_status acpi_os_table_override(struc
> * result: sdt_entry[] is initialized
> */
>
> -int __init acpi_table_init(void)
> +int __init acpi_locate_initial_tables(void)
> {
> acpi_status status;
>
> @@ -803,9 +803,45 @@ 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();
>
> + return 0;
> +}
> +
> +void __init acpi_reserve_initial_tables(void)
> +{
> + int i;
> +
> + for (i = 0; i < ACPI_MAX_TABLES; i++) {
> + struct acpi_table_desc *table_desc = &initial_tables[i];
> + u64 start = table_desc->address;
> + u64 size = table_desc->length;
> +
> + if (!start || !size)
> + break;
> +
> + pr_info("Reserving %4s table memory at [mem 0x%llx-0x%llx]\n",
> + table_desc->signature.ascii, start, start + size - 1);
> +
> + memblock_reserve(start, size);
> + }
> +}
> +
> +void __init acpi_table_init_complete(void)
> +{
> + acpi_table_initrd_scan();
> check_multiple_madt();
> +}
> +
> +int __init acpi_table_init(void)
> +{
> + int ret;
> +
> + ret = acpi_locate_initial_tables();
> + if (ret)
> + return ret;
> +
> + acpi_table_init_complete();
> +
> return 0;
> }
>
>
>
>
--
Sincerely yours,
Mike.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH] ACPI: tables: x86: Reserve memory occupied by ACPI tables
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
0 siblings, 2 replies; 46+ messages in thread
From: Rafael J. Wysocki @ 2021-03-24 13:27 UTC (permalink / raw)
To: Mike Rapoport, George Kennedy
Cc: Rafael J. Wysocki, ACPI Devel Maling List, Rafael J. Wysocki,
Erik Kaneda, David Hildenbrand, Robert Moore, Rafael Wysocki,
Len Brown, Linux Kernel Mailing List, Konrad Rzeszutek Wilk,
Dan Carpenter, Dhaval Giani, Andrew Morton, Vlastimil Babka,
Oscar Salvador, Wei Yang, Pankaj Gupta, Michal Hocko,
x86 Maintainers
On Wed, Mar 24, 2021 at 9:24 AM Mike Rapoport <rppt@linux.ibm.com> wrote:
>
> On Tue, Mar 23, 2021 at 08:26:52PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > The following problem has been reported by 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.
> >
> > Apparently, on the affected system, the ACPI table in question is
> > not located in "reserved" memory, like ACPI NVS or ACPI Data, that
> > will not be used by the buddy allocator, so the memory occupied by
> > that table has to be explicitly reserved to prevent the buddy
> > allocator from using it.
> >
> > In order to address this problem, rearrange the initialization of the
> > ACPI tables on x86 to locate the initial tables earlier and reserve
> > the memory occupied by them.
> >
> > The other architectures using ACPI should not be affected by this
> > change.
> >
> > Link: https://lore.kernel.org/linux-acpi/1614802160-29362-1-git-send-email-george.kennedy@oracle.com/
> > Reported-by: George Kennedy <george.kennedy@oracle.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> FWIW:
> Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>
Thank you!
George, can you please try this patch on the affected system?
> > ---
> > arch/x86/kernel/acpi/boot.c | 25 ++++++++++++-------------
> > arch/x86/kernel/setup.c | 8 +++-----
> > drivers/acpi/tables.c | 42 +++++++++++++++++++++++++++++++++++++++---
> > include/linux/acpi.h | 9 ++++++++-
> > 4 files changed, 62 insertions(+), 22 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
> > @@ -1554,10 +1554,18 @@ void __init acpi_boot_table_init(void)
> > /*
> > * Initialize the ACPI boot-time table parser.
> > */
> > - if (acpi_table_init()) {
> > + if (acpi_locate_initial_tables())
> > disable_acpi();
> > - return;
> > - }
> > + else
> > + acpi_reserve_initial_tables();
> > +}
> > +
> > +int __init early_acpi_boot_init(void)
> > +{
> > + if (acpi_disabled)
> > + return 1;
> > +
> > + acpi_table_init_complete();
> >
> > acpi_table_parse(ACPI_SIG_BOOT, acpi_parse_sbf);
> >
> > @@ -1570,18 +1578,9 @@ void __init acpi_boot_table_init(void)
> > } else {
> > printk(KERN_WARNING PREFIX "Disabling ACPI support\n");
> > disable_acpi();
> > - return;
> > + return 1;
> > }
> > }
> > -}
> > -
> > -int __init early_acpi_boot_init(void)
> > -{
> > - /*
> > - * If acpi_disabled, bail out
> > - */
> > - if (acpi_disabled)
> > - return 1;
> >
> > /*
> > * Process the Multiple APIC Description Table (MADT), if present
> > Index: linux-pm/arch/x86/kernel/setup.c
> > ===================================================================
> > --- linux-pm.orig/arch/x86/kernel/setup.c
> > +++ linux-pm/arch/x86/kernel/setup.c
> > @@ -1045,6 +1045,9 @@ void __init setup_arch(char **cmdline_p)
> >
> > cleanup_highmap();
> >
> > + /* Look for ACPI tables and reserve memory occupied by them. */
> > + acpi_boot_table_init();
> > +
> > memblock_set_current_limit(ISA_END_ADDRESS);
> > e820__memblock_setup();
> >
> > @@ -1136,11 +1139,6 @@ void __init setup_arch(char **cmdline_p)
> >
> > early_platform_quirks();
> >
> > - /*
> > - * Parse the ACPI tables for possible boot-time SMP configuration.
> > - */
> > - acpi_boot_table_init();
> > -
> > early_acpi_boot_init();
> >
> > initmem_init();
> > Index: linux-pm/include/linux/acpi.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/acpi.h
> > +++ linux-pm/include/linux/acpi.h
> > @@ -222,10 +222,14 @@ 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_locate_initial_tables (void);
> > +void acpi_reserve_initial_tables (void);
> > +void acpi_table_init_complete (void);
> > int 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,
> > @@ -814,9 +818,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
> > @@ -780,7 +780,7 @@ acpi_status acpi_os_table_override(struc
> > }
> >
> > /*
> > - * acpi_table_init()
> > + * acpi_locate_initial_tables()
> > *
> > * find RSDP, find and checksum SDT/XSDT.
> > * checksum all tables, print SDT/XSDT
> > @@ -788,7 +788,7 @@ acpi_status acpi_os_table_override(struc
> > * result: sdt_entry[] is initialized
> > */
> >
> > -int __init acpi_table_init(void)
> > +int __init acpi_locate_initial_tables(void)
> > {
> > acpi_status status;
> >
> > @@ -803,9 +803,45 @@ 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();
> >
> > + return 0;
> > +}
> > +
> > +void __init acpi_reserve_initial_tables(void)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ACPI_MAX_TABLES; i++) {
> > + struct acpi_table_desc *table_desc = &initial_tables[i];
> > + u64 start = table_desc->address;
> > + u64 size = table_desc->length;
> > +
> > + if (!start || !size)
> > + break;
> > +
> > + pr_info("Reserving %4s table memory at [mem 0x%llx-0x%llx]\n",
> > + table_desc->signature.ascii, start, start + size - 1);
> > +
> > + memblock_reserve(start, size);
> > + }
> > +}
> > +
> > +void __init acpi_table_init_complete(void)
> > +{
> > + acpi_table_initrd_scan();
> > check_multiple_madt();
> > +}
> > +
> > +int __init acpi_table_init(void)
> > +{
> > + int ret;
> > +
> > + ret = acpi_locate_initial_tables();
> > + if (ret)
> > + return ret;
> > +
> > + acpi_table_init_complete();
> > +
> > return 0;
> > }
> >
> >
> >
> >
>
> --
> Sincerely yours,
> Mike.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH] ACPI: tables: x86: Reserve memory occupied by ACPI tables
2021-03-24 13:27 ` Rafael J. Wysocki
@ 2021-03-24 13:49 ` George Kennedy
2021-03-24 15:42 ` George Kennedy
1 sibling, 0 replies; 46+ messages in thread
From: George Kennedy @ 2021-03-24 13:49 UTC (permalink / raw)
To: Rafael J. Wysocki, Mike Rapoport
Cc: Rafael J. Wysocki, ACPI Devel Maling List, Erik Kaneda,
David Hildenbrand, Robert Moore, Rafael Wysocki, Len Brown,
Linux Kernel Mailing List, Konrad Rzeszutek Wilk, Dan Carpenter,
Dhaval Giani, Andrew Morton, Vlastimil Babka, Oscar Salvador,
Wei Yang, Pankaj Gupta, Michal Hocko, x86 Maintainers
On 3/24/2021 9:27 AM, Rafael J. Wysocki wrote:
> On Wed, Mar 24, 2021 at 9:24 AM Mike Rapoport <rppt@linux.ibm.com> wrote:
>> On Tue, Mar 23, 2021 at 08:26:52PM +0100, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> The following problem has been reported by 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.
>>>
>>> Apparently, on the affected system, the ACPI table in question is
>>> not located in "reserved" memory, like ACPI NVS or ACPI Data, that
>>> will not be used by the buddy allocator, so the memory occupied by
>>> that table has to be explicitly reserved to prevent the buddy
>>> allocator from using it.
>>>
>>> In order to address this problem, rearrange the initialization of the
>>> ACPI tables on x86 to locate the initial tables earlier and reserve
>>> the memory occupied by them.
>>>
>>> The other architectures using ACPI should not be affected by this
>>> change.
>>>
>>> Link: https://lore.kernel.org/linux-acpi/1614802160-29362-1-git-send-email-george.kennedy@oracle.com/
>>> Reported-by: George Kennedy <george.kennedy@oracle.com>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> FWIW:
>> Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>
> Thank you!
>
> George, can you please try this patch on the affected system?
Will do.
George
>
>>> ---
>>> arch/x86/kernel/acpi/boot.c | 25 ++++++++++++-------------
>>> arch/x86/kernel/setup.c | 8 +++-----
>>> drivers/acpi/tables.c | 42 +++++++++++++++++++++++++++++++++++++++---
>>> include/linux/acpi.h | 9 ++++++++-
>>> 4 files changed, 62 insertions(+), 22 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
>>> @@ -1554,10 +1554,18 @@ void __init acpi_boot_table_init(void)
>>> /*
>>> * Initialize the ACPI boot-time table parser.
>>> */
>>> - if (acpi_table_init()) {
>>> + if (acpi_locate_initial_tables())
>>> disable_acpi();
>>> - return;
>>> - }
>>> + else
>>> + acpi_reserve_initial_tables();
>>> +}
>>> +
>>> +int __init early_acpi_boot_init(void)
>>> +{
>>> + if (acpi_disabled)
>>> + return 1;
>>> +
>>> + acpi_table_init_complete();
>>>
>>> acpi_table_parse(ACPI_SIG_BOOT, acpi_parse_sbf);
>>>
>>> @@ -1570,18 +1578,9 @@ void __init acpi_boot_table_init(void)
>>> } else {
>>> printk(KERN_WARNING PREFIX "Disabling ACPI support\n");
>>> disable_acpi();
>>> - return;
>>> + return 1;
>>> }
>>> }
>>> -}
>>> -
>>> -int __init early_acpi_boot_init(void)
>>> -{
>>> - /*
>>> - * If acpi_disabled, bail out
>>> - */
>>> - if (acpi_disabled)
>>> - return 1;
>>>
>>> /*
>>> * Process the Multiple APIC Description Table (MADT), if present
>>> Index: linux-pm/arch/x86/kernel/setup.c
>>> ===================================================================
>>> --- linux-pm.orig/arch/x86/kernel/setup.c
>>> +++ linux-pm/arch/x86/kernel/setup.c
>>> @@ -1045,6 +1045,9 @@ void __init setup_arch(char **cmdline_p)
>>>
>>> cleanup_highmap();
>>>
>>> + /* Look for ACPI tables and reserve memory occupied by them. */
>>> + acpi_boot_table_init();
>>> +
>>> memblock_set_current_limit(ISA_END_ADDRESS);
>>> e820__memblock_setup();
>>>
>>> @@ -1136,11 +1139,6 @@ void __init setup_arch(char **cmdline_p)
>>>
>>> early_platform_quirks();
>>>
>>> - /*
>>> - * Parse the ACPI tables for possible boot-time SMP configuration.
>>> - */
>>> - acpi_boot_table_init();
>>> -
>>> early_acpi_boot_init();
>>>
>>> initmem_init();
>>> Index: linux-pm/include/linux/acpi.h
>>> ===================================================================
>>> --- linux-pm.orig/include/linux/acpi.h
>>> +++ linux-pm/include/linux/acpi.h
>>> @@ -222,10 +222,14 @@ 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_locate_initial_tables (void);
>>> +void acpi_reserve_initial_tables (void);
>>> +void acpi_table_init_complete (void);
>>> int 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,
>>> @@ -814,9 +818,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
>>> @@ -780,7 +780,7 @@ acpi_status acpi_os_table_override(struc
>>> }
>>>
>>> /*
>>> - * acpi_table_init()
>>> + * acpi_locate_initial_tables()
>>> *
>>> * find RSDP, find and checksum SDT/XSDT.
>>> * checksum all tables, print SDT/XSDT
>>> @@ -788,7 +788,7 @@ acpi_status acpi_os_table_override(struc
>>> * result: sdt_entry[] is initialized
>>> */
>>>
>>> -int __init acpi_table_init(void)
>>> +int __init acpi_locate_initial_tables(void)
>>> {
>>> acpi_status status;
>>>
>>> @@ -803,9 +803,45 @@ 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();
>>>
>>> + return 0;
>>> +}
>>> +
>>> +void __init acpi_reserve_initial_tables(void)
>>> +{
>>> + int i;
>>> +
>>> + for (i = 0; i < ACPI_MAX_TABLES; i++) {
>>> + struct acpi_table_desc *table_desc = &initial_tables[i];
>>> + u64 start = table_desc->address;
>>> + u64 size = table_desc->length;
>>> +
>>> + if (!start || !size)
>>> + break;
>>> +
>>> + pr_info("Reserving %4s table memory at [mem 0x%llx-0x%llx]\n",
>>> + table_desc->signature.ascii, start, start + size - 1);
>>> +
>>> + memblock_reserve(start, size);
>>> + }
>>> +}
>>> +
>>> +void __init acpi_table_init_complete(void)
>>> +{
>>> + acpi_table_initrd_scan();
>>> check_multiple_madt();
>>> +}
>>> +
>>> +int __init acpi_table_init(void)
>>> +{
>>> + int ret;
>>> +
>>> + ret = acpi_locate_initial_tables();
>>> + if (ret)
>>> + return ret;
>>> +
>>> + acpi_table_init_complete();
>>> +
>>> return 0;
>>> }
>>>
>>>
>>>
>>>
>> --
>> Sincerely yours,
>> Mike.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH] ACPI: tables: x86: Reserve memory occupied by ACPI tables
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
1 sibling, 1 reply; 46+ messages in thread
From: George Kennedy @ 2021-03-24 15:42 UTC (permalink / raw)
To: Rafael J. Wysocki, Mike Rapoport
Cc: Rafael J. Wysocki, ACPI Devel Maling List, Erik Kaneda,
David Hildenbrand, Robert Moore, Rafael Wysocki, Len Brown,
Linux Kernel Mailing List, Konrad Rzeszutek Wilk, Dan Carpenter,
Dhaval Giani, Andrew Morton, Vlastimil Babka, Oscar Salvador,
Wei Yang, Pankaj Gupta, Michal Hocko, x86 Maintainers
On 3/24/2021 9:27 AM, Rafael J. Wysocki wrote:
> On Wed, Mar 24, 2021 at 9:24 AM Mike Rapoport <rppt@linux.ibm.com> wrote:
>> On Tue, Mar 23, 2021 at 08:26:52PM +0100, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> The following problem has been reported by 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.
>>>
>>> Apparently, on the affected system, the ACPI table in question is
>>> not located in "reserved" memory, like ACPI NVS or ACPI Data, that
>>> will not be used by the buddy allocator, so the memory occupied by
>>> that table has to be explicitly reserved to prevent the buddy
>>> allocator from using it.
>>>
>>> In order to address this problem, rearrange the initialization of the
>>> ACPI tables on x86 to locate the initial tables earlier and reserve
>>> the memory occupied by them.
>>>
>>> The other architectures using ACPI should not be affected by this
>>> change.
>>>
>>> Link: https://lore.kernel.org/linux-acpi/1614802160-29362-1-git-send-email-george.kennedy@oracle.com/
>>> Reported-by: George Kennedy <george.kennedy@oracle.com>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> FWIW:
>> Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>
> Thank you!
>
> George, can you please try this patch on the affected system?
Rafael,
10 for 10 successful reboots with your patch.
First, verified the failure is still there with latest 5.12.0-rc4.
George
P.S. Thanks Mike, Rafael & David
>
>>> ---
>>> arch/x86/kernel/acpi/boot.c | 25 ++++++++++++-------------
>>> arch/x86/kernel/setup.c | 8 +++-----
>>> drivers/acpi/tables.c | 42 +++++++++++++++++++++++++++++++++++++++---
>>> include/linux/acpi.h | 9 ++++++++-
>>> 4 files changed, 62 insertions(+), 22 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
>>> @@ -1554,10 +1554,18 @@ void __init acpi_boot_table_init(void)
>>> /*
>>> * Initialize the ACPI boot-time table parser.
>>> */
>>> - if (acpi_table_init()) {
>>> + if (acpi_locate_initial_tables())
>>> disable_acpi();
>>> - return;
>>> - }
>>> + else
>>> + acpi_reserve_initial_tables();
>>> +}
>>> +
>>> +int __init early_acpi_boot_init(void)
>>> +{
>>> + if (acpi_disabled)
>>> + return 1;
>>> +
>>> + acpi_table_init_complete();
>>>
>>> acpi_table_parse(ACPI_SIG_BOOT, acpi_parse_sbf);
>>>
>>> @@ -1570,18 +1578,9 @@ void __init acpi_boot_table_init(void)
>>> } else {
>>> printk(KERN_WARNING PREFIX "Disabling ACPI support\n");
>>> disable_acpi();
>>> - return;
>>> + return 1;
>>> }
>>> }
>>> -}
>>> -
>>> -int __init early_acpi_boot_init(void)
>>> -{
>>> - /*
>>> - * If acpi_disabled, bail out
>>> - */
>>> - if (acpi_disabled)
>>> - return 1;
>>>
>>> /*
>>> * Process the Multiple APIC Description Table (MADT), if present
>>> Index: linux-pm/arch/x86/kernel/setup.c
>>> ===================================================================
>>> --- linux-pm.orig/arch/x86/kernel/setup.c
>>> +++ linux-pm/arch/x86/kernel/setup.c
>>> @@ -1045,6 +1045,9 @@ void __init setup_arch(char **cmdline_p)
>>>
>>> cleanup_highmap();
>>>
>>> + /* Look for ACPI tables and reserve memory occupied by them. */
>>> + acpi_boot_table_init();
>>> +
>>> memblock_set_current_limit(ISA_END_ADDRESS);
>>> e820__memblock_setup();
>>>
>>> @@ -1136,11 +1139,6 @@ void __init setup_arch(char **cmdline_p)
>>>
>>> early_platform_quirks();
>>>
>>> - /*
>>> - * Parse the ACPI tables for possible boot-time SMP configuration.
>>> - */
>>> - acpi_boot_table_init();
>>> -
>>> early_acpi_boot_init();
>>>
>>> initmem_init();
>>> Index: linux-pm/include/linux/acpi.h
>>> ===================================================================
>>> --- linux-pm.orig/include/linux/acpi.h
>>> +++ linux-pm/include/linux/acpi.h
>>> @@ -222,10 +222,14 @@ 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_locate_initial_tables (void);
>>> +void acpi_reserve_initial_tables (void);
>>> +void acpi_table_init_complete (void);
>>> int 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,
>>> @@ -814,9 +818,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
>>> @@ -780,7 +780,7 @@ acpi_status acpi_os_table_override(struc
>>> }
>>>
>>> /*
>>> - * acpi_table_init()
>>> + * acpi_locate_initial_tables()
>>> *
>>> * find RSDP, find and checksum SDT/XSDT.
>>> * checksum all tables, print SDT/XSDT
>>> @@ -788,7 +788,7 @@ acpi_status acpi_os_table_override(struc
>>> * result: sdt_entry[] is initialized
>>> */
>>>
>>> -int __init acpi_table_init(void)
>>> +int __init acpi_locate_initial_tables(void)
>>> {
>>> acpi_status status;
>>>
>>> @@ -803,9 +803,45 @@ 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();
>>>
>>> + return 0;
>>> +}
>>> +
>>> +void __init acpi_reserve_initial_tables(void)
>>> +{
>>> + int i;
>>> +
>>> + for (i = 0; i < ACPI_MAX_TABLES; i++) {
>>> + struct acpi_table_desc *table_desc = &initial_tables[i];
>>> + u64 start = table_desc->address;
>>> + u64 size = table_desc->length;
>>> +
>>> + if (!start || !size)
>>> + break;
>>> +
>>> + pr_info("Reserving %4s table memory at [mem 0x%llx-0x%llx]\n",
>>> + table_desc->signature.ascii, start, start + size - 1);
>>> +
>>> + memblock_reserve(start, size);
>>> + }
>>> +}
>>> +
>>> +void __init acpi_table_init_complete(void)
>>> +{
>>> + acpi_table_initrd_scan();
>>> check_multiple_madt();
>>> +}
>>> +
>>> +int __init acpi_table_init(void)
>>> +{
>>> + int ret;
>>> +
>>> + ret = acpi_locate_initial_tables();
>>> + if (ret)
>>> + return ret;
>>> +
>>> + acpi_table_init_complete();
>>> +
>>> return 0;
>>> }
>>>
>>>
>>>
>>>
>> --
>> Sincerely yours,
>> Mike.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH] ACPI: tables: x86: Reserve memory occupied by ACPI tables
2021-03-24 15:42 ` George Kennedy
@ 2021-03-24 15:44 ` Rafael J. Wysocki
0 siblings, 0 replies; 46+ messages in thread
From: Rafael J. Wysocki @ 2021-03-24 15:44 UTC (permalink / raw)
To: George Kennedy
Cc: Rafael J. Wysocki, Mike Rapoport, Rafael J. Wysocki,
ACPI Devel Maling List, Erik Kaneda, David Hildenbrand,
Robert Moore, Rafael Wysocki, Len Brown,
Linux Kernel Mailing List, Konrad Rzeszutek Wilk, Dan Carpenter,
Dhaval Giani, Andrew Morton, Vlastimil Babka, Oscar Salvador,
Wei Yang, Pankaj Gupta, Michal Hocko, x86 Maintainers
On Wed, Mar 24, 2021 at 4:42 PM George Kennedy
<george.kennedy@oracle.com> wrote:
>
>
>
> On 3/24/2021 9:27 AM, Rafael J. Wysocki wrote:
> > On Wed, Mar 24, 2021 at 9:24 AM Mike Rapoport <rppt@linux.ibm.com> wrote:
> >> On Tue, Mar 23, 2021 at 08:26:52PM +0100, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>
> >>> The following problem has been reported by 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.
> >>>
> >>> Apparently, on the affected system, the ACPI table in question is
> >>> not located in "reserved" memory, like ACPI NVS or ACPI Data, that
> >>> will not be used by the buddy allocator, so the memory occupied by
> >>> that table has to be explicitly reserved to prevent the buddy
> >>> allocator from using it.
> >>>
> >>> In order to address this problem, rearrange the initialization of the
> >>> ACPI tables on x86 to locate the initial tables earlier and reserve
> >>> the memory occupied by them.
> >>>
> >>> The other architectures using ACPI should not be affected by this
> >>> change.
> >>>
> >>> Link: https://lore.kernel.org/linux-acpi/1614802160-29362-1-git-send-email-george.kennedy@oracle.com/
> >>> Reported-by: George Kennedy <george.kennedy@oracle.com>
> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> FWIW:
> >> Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>
> > Thank you!
> >
> > George, can you please try this patch on the affected system?
>
> Rafael,
>
> 10 for 10 successful reboots with your patch.
>
> First, verified the failure is still there with latest 5.12.0-rc4.
Thank you!
I'll add a Tested-by from you to it, then.
^ permalink raw reply [flat|nested] 46+ messages in thread