Re: [PATCH v5 09/27] x86/boot: Remove mapping from page fault handler

From: Evgeniy Baskov <baskov@ispras.ru>
To: Andy Lutomirski <luto@kernel.org>
Cc: Ard Biesheuvel <ardb@kernel.org>, 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
Date: Wed, 15 Mar 2023 16:25:45 +0300	[thread overview]
Message-ID: <909bd8fed79cfc295a269539e7c77a98@ispras.ru> (raw)
In-Reply-To: <259a1148-faf0-4765-b777-6f36459c9307@app.fastmail.com>

On 2023-03-14 23:33, Andy Lutomirski wrote:
> 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.

Good point. This patch can be dropped and replaced by the loud warning,
since it is not required for the functioning of the rest of the series
it is here mainly to indicate bugs in the kernel rather than for the
increased protection. But I would not expect anything in the working
systems, I made my best to map all the things explicitly. And since
no code but the extraction code is supposed to be run (interrupts
are disabled and we are not using any UEFI services there), this should
be practically save to remove the implicit mapping.

And at least this allowed me to find out about the insufficient size of
the boot page tables which did not account for the ACPI and UEFI
mappings. (patch 4 "x86/boot: Increase boot page table size")

If this patch is dropped now, I can send the follow up patch later
adding the warning.

Thanks,
Evgeniy Baskov

> 
> --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(-)
>> 
...