| 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
| ||
|
Message-ID: <20220418155545.a567xnxa6elglapl@box.shutemov.name>
Date: Mon, 18 Apr 2022 18:55:45 +0300
From: "Kirill A. Shutemov" <kirill@...temov.name>
To: Borislav Petkov <bp@...en8.de>
Cc: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
Andy Lutomirski <luto@...nel.org>,
Sean Christopherson <seanjc@...gle.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Joerg Roedel <jroedel@...e.de>,
Ard Biesheuvel <ardb@...nel.org>,
Andi Kleen <ak@...ux.intel.com>,
Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@...ux.intel.com>,
David Rientjes <rientjes@...gle.com>,
Vlastimil Babka <vbabka@...e.cz>,
Tom Lendacky <thomas.lendacky@....com>,
Thomas Gleixner <tglx@...utronix.de>,
Peter Zijlstra <peterz@...radead.org>,
Paolo Bonzini <pbonzini@...hat.com>,
Ingo Molnar <mingo@...hat.com>,
Varad Gautam <varad.gautam@...e.com>,
Dario Faggioli <dfaggioli@...e.com>,
Dave Hansen <dave.hansen@...el.com>,
Brijesh Singh <brijesh.singh@....com>,
Mike Rapoport <rppt@...nel.org>,
David Hildenbrand <david@...hat.com>, x86@...nel.org,
linux-mm@...ck.org, linux-coco@...ts.linux.dev,
linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCHv4 3/8] efi/x86: Implement support for unaccepted memory
On Sat, Apr 16, 2022 at 12:24:26AM +0200, Borislav Petkov wrote:
> On Wed, Apr 06, 2022 at 02:43:38AM +0300, Kirill A. Shutemov wrote:
> > diff --git a/Documentation/x86/zero-page.rst b/Documentation/x86/zero-page.rst
> > index f088f5881666..8e3447a4b373 100644
> > --- a/Documentation/x86/zero-page.rst
> > +++ b/Documentation/x86/zero-page.rst
> > @@ -42,4 +42,5 @@ Offset/Size Proto Name Meaning
> > 2D0/A00 ALL e820_table E820 memory map table
> > (array of struct e820_entry)
> > D00/1EC ALL eddbuf EDD data (array of struct edd_info)
> > +ECC/008 ALL unaccepted_memory Bitmap of unaccepted memory (1bit == 2M)
>
> There's a perfectly fine spot at 0x78:
>
> __u8 _pad3[8]; /* 0x078 */
>
> why not take that one?
Good point. Will do.
>
> > =========== ===== ======================= =================================================
> > diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> > index 8fd0e6ae2e1f..09993797efa2 100644
> > --- a/arch/x86/boot/compressed/Makefile
> > +++ b/arch/x86/boot/compressed/Makefile
> > @@ -102,6 +102,7 @@ endif
> >
> > vmlinux-objs-$(CONFIG_ACPI) += $(obj)/acpi.o
> > vmlinux-objs-$(CONFIG_INTEL_TDX_GUEST) += $(obj)/tdx.o $(obj)/tdcall.o
> > +vmlinux-objs-$(CONFIG_UNACCEPTED_MEMORY) += $(obj)/bitmap.o $(obj)/unaccepted_memory.o
> >
> > vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_thunk_$(BITS).o
> > efi-obj-$(CONFIG_EFI_STUB) = $(objtree)/drivers/firmware/efi/libstub/lib.a
> > diff --git a/arch/x86/boot/compressed/bitmap.c b/arch/x86/boot/compressed/bitmap.c
> > new file mode 100644
> > index 000000000000..bf58b259380a
> > --- /dev/null
> > +++ b/arch/x86/boot/compressed/bitmap.c
> > @@ -0,0 +1,24 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Taken from lib/string.c */
> > +
> > +#include <linux/bitmap.h>
>
> verify_include_paths: Warning: Kernel-proper include at arch/x86/boot/compressed/bitmap.c:4 [+#include <linux/bitmap.h>]
>
> Same game as before: put the stuff you need into a separate or a shared
> header and avoid the linux/ namespace include.
I'm confused here. What is wrong with linux/ include namespace?
Yes, we had story with <asm/io.h> that actually caused issue in
decompression code, but linux/ has a lot of perfectly portable
library-like stuff.
Could you explain what rules are?
> > @@ -0,0 +1,53 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +#include "error.h"
> > +#include "misc.h"
> > +
> > +static inline void __accept_memory(phys_addr_t start, phys_addr_t end)
> > +{
> > + /* Platform-specific memory-acceptance call goes here */
> > + error("Cannot accept memory");
> > +}
> > +
> > +void mark_unaccepted(struct boot_params *params, u64 start, u64 end)
>
> That name is kinda misleading? It is not only marking as unaccepted - it
> is also accepting weird 2M misaligned chunks...
Hm. accept_or_mark_unaccepted()?
> > +{
> > + /*
> > + * The accepted memory bitmap only works at PMD_SIZE granularity.
> > + * If a request comes in to mark memory as unaccepted which is not
> > + * PMD_SIZE-aligned, simply accept the memory now since it can not be
> > + * *marked* as unaccepted.
> > + */
>
> That comment goes over the function name.
>
> > + /*
> > + * Accept small regions that might not be able to be represented
> > + * in the bitmap:
> > + */
> > + if (end - start < 2 * PMD_SIZE) {
> > + __accept_memory(start, end);
> > + return;
> > + }
> > +
> > + /*
> > + * No matter how the start and end are aligned, at least one unaccepted
> > + * PMD_SIZE area will remain.
> > + */
> > +
> > + /* Immediately accept a <PMD_SIZE piece at the start: */
>
> Immediately? As opposed to delayed?
Yes. Otherwise accept is delayed until the first allocation of the memory.
> > + if (start & ~PMD_MASK) {
> > + __accept_memory(start, round_up(start, PMD_SIZE));
> > + start = round_up(start, PMD_SIZE);
> > + }
> > +
> > + /* Immediately accept a <PMD_SIZE piece at the end: */
> > + if (end & ~PMD_MASK) {
> > + __accept_memory(round_down(end, PMD_SIZE), end);
> > + end = round_down(end, PMD_SIZE);
> > + }
> > +
> > + /*
> > + * 'start' and 'end' are now both PMD-aligned.
> > + * Record the range as being unaccepted:
> > + */
> > + bitmap_set((unsigned long *)params->unaccepted_memory,
> > + start / PMD_SIZE, (end - start) / PMD_SIZE);
> > +}
> > diff --git a/arch/x86/include/asm/unaccepted_memory.h b/arch/x86/include/asm/unaccepted_memory.h
>
> Why do you need a separate header?
>
> We already have
>
> arch/x86/include/asm/mem_encrypt.h
>
> and this is kinda very much related...
I don't see it.
Memory encryption can be a reason to have unaccepted memory, but it is not
1:1 match. Unaccepted memory can be present without memory ecnryption if
data secruty and integrity guaranteed by other means.
<asm/mem_encrypt.h> is very AMD SME/SEV centric. I'm not sure it need to
exist in the way it is now.
> > + u64 max_addr = 0;
> > + int i;
> >
> > status = efi_get_memory_map(map);
> > if (status != EFI_SUCCESS)
> > @@ -589,9 +601,57 @@ static efi_status_t allocate_e820(struct boot_params *params,
> > if (status != EFI_SUCCESS)
> > goto out;
> > }
>
> This whole chunk you're adding here begs to be a separate function with
> the big fat comment placed over the function name.
>
> Might just as well call it after allocate_e820() has been called.
Okay, I will move it into a separate function, but it has to be called
from allocate_e820() because it allocates and free the map.
> > +
> > + if (!IS_ENABLED(CONFIG_UNACCEPTED_MEMORY))
> > + goto out;
> > +
> > + /* Check if there's any unaccepted memory and find the max address */
> > + for (i = 0; i < nr_desc; i++) {
> > + efi_memory_desc_t *d;
> > +
> > + d = efi_early_memdesc_ptr(*map->map, *map->desc_size, i);
> > + if (d->type == EFI_UNACCEPTED_MEMORY)
> > + unaccepted_memory_present = true;
> > + if (d->phys_addr + d->num_pages * PAGE_SIZE > max_addr)
> > + max_addr = d->phys_addr + d->num_pages * PAGE_SIZE;
> > + }
> > +
> > + /*
> > + * If unaccepted memory is present allocate a bitmap to track what
> > + * memory has to be accepted before access.
> > + *
> > + * One bit in the bitmap represents 2MiB in the address space:
> > + * A 4k bitmap can track 64GiB of physical address space.
> > + *
> > + * In the worst case scenario -- a huge hole in the middle of the
> > + * address space -- It needs 256MiB to handle 4PiB of the address
> > + * space.
>
> And you're saying that that efi_allocate_pages() below can really give a
> 256M contiguous chunk?
Yes, that's assumption. Is it too high ask to deal with 4PiB of PA?
--
Kirill A. Shutemov
Powered by blists - more mailing lists