| 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: <20170427103112.GA2627@x1>
Date: Thu, 27 Apr 2017 18:31:12 +0800
From: Baoquan He <bhe@...hat.com>
To: Thomas Garnier <thgarnie@...gle.com>
Cc: LKML <linux-kernel@...r.kernel.org>,
Ingo Molnar <mingo@...hat.com>,
Kees Cook <keescook@...omium.org>,
Dave Young <dyoung@...hat.com>,
Xunlei Pang <xlpang@...hat.com>,
Matt Fleming <matt@...eblueprint.co.uk>,
Ard Biesheuvel <ard.biesheuvel@...aro.org>,
Thomas Gleixner <tglx@...utronix.de>,
"H. Peter Anvin" <hpa@...or.com>,
the arch/x86 maintainers <x86@...nel.org>,
linux-efi@...r.kernel.org
Subject: Re: [PATCH 1/2] x86/efi: Correct ident mapping of efi old_map when
kalsr enabled
Hi Thomas,
Thanks for reviewing!
On 04/26/17 at 07:49am, Thomas Garnier wrote:
> On Wed, Apr 26, 2017 at 3:43 AM, Baoquan He <bhe@...hat.com> wrote:
> > > arch/x86/platform/efi/efi_64.c | 35 +++++++++++++++++++++++++++--------
> > > 1 file changed, 27 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> > > index 2ee7694..2e7baff 100644
> > > --- a/arch/x86/platform/efi/efi_64.c
> > > +++ b/arch/x86/platform/efi/efi_64.c
> > > @@ -71,11 +71,12 @@ static void __init early_code_mapping_set_exec(int executable)
> > >
> > > pgd_t * __init efi_call_phys_prolog(void)
> > > {
> > > - unsigned long vaddress;
> > > + unsigned long vaddr, left_vaddr;
> > > + unsigned int num_entries;
> > > pgd_t *save_pgd;
> > > -
> > > - int pgd;
> > > + pud_t *pud, *pud_k;
> > > int n_pgds;
> > > + int i;
> > >
> > > if (!efi_enabled(EFI_OLD_MEMMAP)) {
> > > save_pgd = (pgd_t *)read_cr3();
> > > @@ -88,10 +89,22 @@ pgd_t * __init efi_call_phys_prolog(void)
> > > n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
> > > save_pgd = kmalloc_array(n_pgds, sizeof(*save_pgd), GFP_KERNEL);
> > >
> > > - for (pgd = 0; pgd < n_pgds; pgd++) {
> > > - save_pgd[pgd] = *pgd_offset_k(pgd * PGDIR_SIZE);
> > > - vaddress = (unsigned long)__va(pgd * PGDIR_SIZE);
> > > - set_pgd(pgd_offset_k(pgd * PGDIR_SIZE), *pgd_offset_k(vaddress));
> > > + for (i = 0; i < n_pgds; i++) {
> > > + save_pgd[i] = *pgd_offset_k(i * PGDIR_SIZE);
> > > +
> > > + vaddr = (unsigned long)__va(i * PGDIR_SIZE);
> > > + pud = pud_alloc_one(NULL, 0);
>
> Please check if pud is NULL.
I considered it a while. I didn't check because I thought it's still in
kernel init stage, and at most 128 page frames are cost for 64TB,
namely 512KB. If kernel can't give 512KB at this time, it will die soon.
I would like to hear what people are suggesting. Since you have pointed
it out, I will add checking here.
However I think we can keep those allocated page and try our best to
build as much ident mapping as possible. E.g if we have 10TB memory, but
failed to allocate page for 11th pud table, we can break the for loop,
leave those built ident mapping there since efi region could be located
inside those 0~5TB region.
Then inefi_call_phys_epilog() only free these allocated pud tables in
efi_call_phys_prolog, check and avoid freeing those pud tables from
direct mapping which still existed because of allocation failure in
efi_call_phys_prolog.
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 2e7baff..67920d4 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -94,6 +94,11 @@ pgd_t * __init efi_call_phys_prolog(void)
vaddr = (unsigned long)__va(i * PGDIR_SIZE);
pud = pud_alloc_one(NULL, 0);
+ if (!pud) {
+ pr_err("Failed to allocate page for %d-th pud table "
+ "to build 1:1 mapping!\n", i);
+ break;
+ }
num_entries = PTRS_PER_PUD - pud_index(vaddr);
pud_k = pud_offset(pgd_offset_k(vaddr), vaddr);
@@ -132,8 +137,10 @@ void __init efi_call_phys_epilog(pgd_t *save_pgd)
for (pgd_idx = 0; pgd_idx < nr_pgds; pgd_idx++) {
pgd = pgd_offset_k(pgd_idx * PGDIR_SIZE);
- pud = (pud_t *)pgd_page_vaddr(*pgd);
- pud_free(NULL, pud);
+ if (*pgd != save_pgd[pgd_idx]) {
+ pud = (pud_t *)pgd_page_vaddr(*pgd);
+ pud_free(NULL, pud);
+ }
set_pgd(pgd_offset_k(pgd_idx * PGDIR_SIZE), save_pgd[pgd_idx]);
}
>
> > > +
> > > + num_entries = PTRS_PER_PUD - pud_index(vaddr);
> > > + pud_k = pud_offset(pgd_offset_k(vaddr), vaddr);
> > > + memcpy(pud, pud_k, num_entries);
> > > + if (pud_index(vaddr) > 0) {
>
> You are using pud_index(vaddr) 3 times, might be worth using a local variable.
Sure, will do, thanks.
>
> > > + left_vaddr = vaddr + (num_entries * PUD_SIZE);
> > > + pud_k = pud_offset(pgd_offset_k(left_vaddr),
> > > + left_vaddr);
> > > + memcpy(pud + num_entries, pud_k, pud_index(vaddr));
>
> I think this section (or the overall for loop) would benefit with a
> comment explaining explaining why you are shifting the new PUD like
> this.
Will write a paragraph.
>
> > > + }
> > > + pgd_populate(NULL, pgd_offset_k(i * PGDIR_SIZE), pud);
> > > }
> > > out:
> > > __flush_tlb_all();
> > > @@ -106,6 +119,8 @@ void __init efi_call_phys_epilog(pgd_t *save_pgd)
> > > */
> > > int pgd_idx;
> > > int nr_pgds;
> > > + pud_t *pud;
> > > + pgd_t *pgd;
> > >
> > > if (!efi_enabled(EFI_OLD_MEMMAP)) {
> > > write_cr3((unsigned long)save_pgd);
> > > @@ -115,8 +130,12 @@ void __init efi_call_phys_epilog(pgd_t *save_pgd)
> > >
> > > nr_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT) , PGDIR_SIZE);
> > >
> > > - for (pgd_idx = 0; pgd_idx < nr_pgds; pgd_idx++)
> > > + for (pgd_idx = 0; pgd_idx < nr_pgds; pgd_idx++) {
> > > + pgd = pgd_offset_k(pgd_idx * PGDIR_SIZE);
> > > + pud = (pud_t *)pgd_page_vaddr(*pgd);
> > > + pud_free(NULL, pud);
> > > set_pgd(pgd_offset_k(pgd_idx * PGDIR_SIZE), save_pgd[pgd_idx]);
> > > + }
> > >
> > > kfree(save_pgd);
> > >
> > > --
> > > 2.5.5
> > >
>
>
>
>
> --
> Thomas
Powered by blists - more mailing lists