linux-kernel - Re: [PATCH] x86/reboot: Don't corrupt memory on non-BIOS systems

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] [day] [month] [year] [list]

Message-ID: <9766201A-7C6B-4E80-A6A9-3593780F68F0@zytor.com>
Date: Thu, 09 Jan 2025 19:25:35 -0800
From: "H. Peter Anvin" <hpa@...or.com>
To: Roman Kisel <romank@...ux.microsoft.com>, akpm@...ux-foundation.org,
        bhe@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
        kai.huang@...el.com, kirill.shutemov@...ux.intel.com, mingo@...hat.com,
        pbonzini@...hat.com, tglx@...utronix.de, x86@...nel.org,
        linux-kernel@...r.kernel.org
CC: apais@...rosoft.com, benhill@...rosoft.com, ssengar@...rosoft.com,
        sunilmut@...rosoft.com, vdso@...bites.dev
Subject: Re: [PATCH] x86/reboot: Don't corrupt memory on non-BIOS systems

On January 9, 2025 12:43:52 PM PST, Roman Kisel <romank@...ux.microsoft.com> wrote:
>native_machine_emergency_restart() writes unconditionally
>to the physical address of 0x472 to pass the warm reboot
>flags to BIOS. The BIOS reads this on booting to bypass memory
>test and do the warm boot. On the non-BIOS systems, other
>means have to be employed, and this write is a memory corruption.
>
>Fix that by moving the offending write into the case where
>the machine is rebooted via BIOS.
>
>Signed-off-by: Roman Kisel <romank@...ux.microsoft.com>
>---
> arch/x86/kernel/reboot.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
>index 615922838c51..6eec8653493f 100644
>--- a/arch/x86/kernel/reboot.c
>+++ b/arch/x86/kernel/reboot.c
>@@ -637,9 +637,8 @@ static void native_machine_emergency_restart(void)
> 
> 	tboot_shutdown(TB_SHUTDOWN_REBOOT);
> 
>-	/* Tell the BIOS if we want cold or warm reboot */
>+	/* Tell the firmware if we want cold or warm reboot */
> 	mode = reboot_mode == REBOOT_WARM ? 0x1234 : 0;
>-	*((unsigned short *)__va(0x472)) = mode;
> 
> 	/*
> 	 * If an EFI capsule has been registered with the firmware then
>@@ -681,6 +680,7 @@ static void native_machine_emergency_restart(void)
> 			break;
> 
> 		case BOOT_BIOS:
>+			*((unsigned short *)__va(0x472)) = mode;
> 			machine_real_restart(MRR_BIOS);
> 
> 			/* We're probably dead after this, but... */
>
>base-commit: eea6e4b4dfb8859446177c32961c96726d0117be

I should say: this patch is unambiguously *wrong*. It conflates the invocation mechanism with the desired post state, and they are not coupled. Calling the BIOS reboot entry point is not the normal way to reboot even on BIOS systems.