Feedback-ID: ieff94742:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-221-gec32977366-fm-20230306.001-gec329773
Mime-Version: 1.0
Message-Id: <259a1148-faf0-4765-b777-6f36459c9307@app.fastmail.com>
In-Reply-To: <dab948690a74db1bb75d95aa7e0362deeca6dbf4.1678785672.git.baskov@ispras.ru>
References: <cover.1678785672.git.baskov@ispras.ru>
 <dab948690a74db1bb75d95aa7e0362deeca6dbf4.1678785672.git.baskov@ispras.ru>
Date:   Tue, 14 Mar 2023 13:33:21 -0700
From:   "Andy Lutomirski" <luto@kernel.org>
To:     "Evgeniy Baskov" <baskov@ispras.ru>,
        "Ard Biesheuvel" <ardb@kernel.org>
Cc:     "Borislav Petkov" <bp@alien8.de>,
        "Dave Hansen" <dave.hansen@linux.intel.com>,
        "Ingo Molnar" <mingo@redhat.com>,
        "Peter Zijlstra (Intel)" <peterz@infradead.org>,
        "Thomas Gleixner" <tglx@linutronix.de>,
        "Alexey Khoroshilov" <khoroshilov@ispras.ru>,
        "Peter Jones" <pjones@redhat.com>,
        "Gerd Hoffmann" <kraxel@redhat.com>,
        "Limonciello, Mario" <mario.limonciello@amd.com>,
        joeyli <jlee@suse.com>, lvc-project@linuxtesting.org,
        "the arch/x86 maintainers" <x86@kernel.org>,
        linux-efi@vger.kernel.org,
        "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
        linux-hardening@vger.kernel.org
Subject: Re: [PATCH v5 09/27] x86/boot: Remove mapping from page fault handler
Content-Type: text/plain
Precedence: bulk

On Tue, Mar 14, 2023, at 3:13 AM, Evgeniy Baskov wrote:
> After every implicit mapping is removed, this code is no longer needed.
>
> Remove memory mapping from page fault handler to ensure that there are
> no hidden invalid memory accesses.

This patch is *by far* the scariest of the bunch in my boot.  And it violates a basic principle of kernel development: it's better to run in degraded mode than to fail outright unless running in degraded mode is dangerous for some reason.

And this boot code is not actually meaningfully exposed to attack.  Anyone who can get the boot code to consume garbage likely *already* controls the system, including anything that we might write to TPM or any other verification mechanism.

So I think this should log an error, set a flag to make sure we print an even louder error after full boot, but still add the mapping and keep trying.

--Andy

>
> Tested-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
> ---
>  arch/x86/boot/compressed/ident_map_64.c | 26 ++++++++++---------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/ident_map_64.c 
> b/arch/x86/boot/compressed/ident_map_64.c
> index eb28ce9812c5..378f99b1d7e8 100644
> --- a/arch/x86/boot/compressed/ident_map_64.c
> +++ b/arch/x86/boot/compressed/ident_map_64.c
> @@ -393,27 +393,21 @@ void do_boot_page_fault(struct pt_regs *regs, 
> unsigned long error_code)
>  {
>  	unsigned long address = native_read_cr2();
>  	unsigned long end;
> -	bool ghcb_fault;
> +	char *msg;
> 
> -	ghcb_fault = sev_es_check_ghcb_fault(address);
> +	if (sev_es_check_ghcb_fault(address))
> +		msg = "Page-fault on GHCB page:";
> +	else
> +		msg = "Unexpected page-fault:";
> 
>  	address   &= PMD_MASK;
>  	end        = address + PMD_SIZE;
> 
>  	/*
> -	 * Check for unexpected error codes. Unexpected are:
> -	 *	- Faults on present pages
> -	 *	- User faults
> -	 *	- Reserved bits set
> -	 */
> -	if (error_code & (X86_PF_PROT | X86_PF_USER | X86_PF_RSVD))
> -		do_pf_error("Unexpected page-fault:", error_code, address, regs->ip);
> -	else if (ghcb_fault)
> -		do_pf_error("Page-fault on GHCB page:", error_code, address, regs->ip);
> -
> -	/*
> -	 * Error code is sane - now identity map the 2M region around
> -	 * the faulting address.
> +	 * Since all memory allocations are made explicit
> +	 * now, every page fault at this stage is an
> +	 * error and the error handler is there only
> +	 * for debug purposes.
>  	 */
> -	kernel_add_identity_map(address, end, MAP_WRITE);
> +	do_pf_error(msg, error_code, address, regs->ip);
>  }
> -- 
> 2.39.2