linux-kernel - Re: [PATCH v3] x86/acpi: fix panic while AP online later with kernel parameter maxcpus=1

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <41ee5438e8059c299f5722e386cdc1457ddc16eb.camel@intel.com>
Date: Tue, 2 Jul 2024 12:05:38 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "tglx@...utronix.de" <tglx@...utronix.de>,
	"kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>, "Li,
 Zhiquan1" <zhiquan1.li@...el.com>, "mingo@...hat.com" <mingo@...hat.com>,
	"x86@...nel.org" <x86@...nel.org>, "bp@...en8.de" <bp@...en8.de>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>
CC: "hpa@...or.com" <hpa@...or.com>, "rafael@...nel.org" <rafael@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>
Subject: Re: [PATCH v3] x86/acpi: fix panic while AP online later with kernel
 parameter maxcpus=1

On Tue, 2024-07-02 at 08:58 +0800, Zhiquan Li wrote:
> The issue was found on the platform that using "Multiprocessor Wakeup
> Structure"[1] to startup secondary CPU, which is usually used by
> encrypted guest.  When restrict boot time CPU to 1 with the kernel
> parameter "maxcpus=1" and bring other CPUs online later, there will be
> a kernel panic.
> 
> The variable acpi_mp_wake_mailbox, which holds the virtual address of
> the MP Wakeup Structure mailbox, will be set as read-only after init.
> If the first AP gets online later, after init, the attempt to update
> the variable results in panic.
> 
> The memremap() call that initializes the variable cannot be moved into
> acpi_parse_mp_wake() because memremap() is not functional at that point
> in the boot process.
> 
> [1] Details about the MP Wakeup structure can be found in ACPI v6.4, in
>     the "Multiprocessor Wakeup Structure" section.
> 
> Signed-off-by: Zhiquan Li <zhiquan1.li@...el.com>
> Reviewed-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>

Seems this changelog only mentions the problem, but doesn't say how to fix:

  Remove the __ro_after_init annotation of acpi_mp_wake_mailbox to fix.

[...]

> 
>  /* Virtual address of the Multiprocessor Wakeup Structure mailbox */
> -static struct acpi_madt_multiproc_wakeup_mailbox *acpi_mp_wake_mailbox __ro_after_init;
> +static struct acpi_madt_multiproc_wakeup_mailbox *acpi_mp_wake_mailbox;
>  
> 

But if memremap() isn't available in acpi_parse_mp_wake() is the concern, could
we change to do it before smp_init() but after memremap() is available?

E.g, we should be able to use early_initcall() (only compile tested):

diff --git a/arch/x86/kernel/acpi/madt_wakeup.c
b/arch/x86/kernel/acpi/madt_wakeup.c
index 6cfe762be28b..ca0aea0832ac 100644
--- a/arch/x86/kernel/acpi/madt_wakeup.c
+++ b/arch/x86/kernel/acpi/madt_wakeup.c
@@ -176,18 +176,6 @@ static int acpi_wakeup_cpu(u32 apicid, unsigned long
start_ip)
                return -EOPNOTSUPP;
        }
 
-       /*
-        * Remap mailbox memory only for the first call to acpi_wakeup_cpu().
-        *
-        * Wakeup of secondary CPUs is fully serialized in the core code.
-        * No need to protect acpi_mp_wake_mailbox from concurrent accesses.
-        */
-       if (!acpi_mp_wake_mailbox) {
-               acpi_mp_wake_mailbox = memremap(acpi_mp_wake_mailbox_paddr,
-                                               sizeof(*acpi_mp_wake_mailbox),
-                                               MEMREMAP_WB);
-       }
-
        /*
         * Mailbox memory is shared between the firmware and OS. Firmware will
         * listen on mailbox command address, and once it receives the wakeup
@@ -290,3 +278,29 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers
*header,
 
        return 0;
 }
+
+static int __init acpi_mp_wake_map_mailbox(void)
+{
+       /*
+        * Nothing to do if the "Multiprocessor Wakeup Structure" is
+        * not present.  acpi_mp_wake_mailbox_paddr could also be 0
+        * if the kernel is from kexec.
+        */
+       if (!acpi_mp_wake_mailbox_paddr)
+               return 0;
+
+       /* memremap() isn't available in acpi_parse_mp_wake() */
+       acpi_mp_wake_mailbox = memremap(acpi_mp_wake_mailbox_paddr,
+                                       sizeof(*acpi_mp_wake_mailbox),
+                                       MEMREMAP_WB);
+       /*
+        * When memremap() fails, clear acpi_mp_wake_mailbox_paddr
+        * to prevent NULL dereference of acpi_mp_wake_mailbox
+        * when bringing up secondary CPUs.
+        */
+       if (WARN_ON(!acpi_mp_wake_mailbox))
+               acpi_mp_wake_mailbox_paddr = 0;
+
+       return 0;
+}
+early_initcall(acpi_mp_wake_map_mailbox);