| 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: <20170427104701.GB2627@x1>
Date: Thu, 27 Apr 2017 18:47:01 +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
On 04/27/17 at 06:31pm, Baoquan He wrote:
> 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;
> > > > -
> > > > + 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.
Or panic if pud_alloc_one failed since kernel won't function well
anyway.
>
> 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