linux-kernel - RE: [PATCH V2 1/2] x86/efi: Unmap EFI boot services code/data regions from efi

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 29 Oct 2018 18:01:27 +0000
From:   "Prakhya, Sai Praneeth" <sai.praneeth.prakhya@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>
CC:     "linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>, Borislav Petkov <bp@...en8.de>,
        "Ingo Molnar" <mingo@...nel.org>,
        Andy Lutomirski <luto@...nel.org>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        Bhupesh Sharma <bhsharma@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>
Subject: RE: [PATCH V2 1/2] x86/efi: Unmap EFI boot services code/data
 regions from efi_pgd

Hi Thomas and Peter,

[off the mailing list because I wasn't sure if it's a good idea to spam others with my questions]

> > > +	/*
> > > +	 * The typical sequence for unmapping is to find a pte through
> > > +	 * lookup_address_in_pgd() (ideally, it should never return NULL
> because
> > > +	 * the address is already mapped) and change it's protections.
> > > +	 * As pfn is the *target* of a mapping, it's not useful while unmapping.
> > > +	 */
> > > +	struct cpa_data cpa = {
> > > +		.vaddr		= &address,
> > > +		.pgd		= pgd,
> > > +		.numpages	= numpages,
> > > +		.mask_set	= __pgprot(0),
> > > +		.mask_clr	= __pgprot(_PAGE_PRESENT | _PAGE_RW),
> > > +		.flags		= 0,
> > > +	};
> > > +
> > > +	retval = __change_page_attr_set_clr(&cpa, 0);
> > > +	__flush_tlb_all();
> >
> > So this looks like you copied it from kernel_map_pages_in_pgd() which
> > has been discussed before to be not sufficient, but it can't be
> > changed right now due to locking issues.
>
> Managed to confuse myself. The place which cannot be changed is a different
> one, but still for your call site __flush_tlb_all() might not be sufficient.

Since the mappings are changed, I thought a flush_tlb() might be needed.
But, (to be honest), I don't completely understand the x86/mm code. So, could you 
please elaborate the issue more for my better understanding?

So some questions I have are,
1. What did Peter Z mean here? "How is that not a TLB invalidation bug ?"
2. I assumed kernel_map_pages_in_pgd() to be a good code but you said that 
it has some locking issues. So, could you please elaborate more on that.

Or, could you please provide me some pointers in the source code that I can take a look at so that I could understand the issue much better.

Regards,
Sai