Re: [PATCH v2 1/5] x86/mm/pageattr: Ensure cpa->pfn only contains page frame numbers

[Borislav Petkov <bp@suse.de>]

On Mon, Nov 16, 2015 at 09:19:01PM +0100, Thomas Gleixner wrote:
> On Sat, 14 Nov 2015, Matt Fleming wrote:
> > The x86 pageattr code is confused about the data that is stored
> > cpa->pfn, sometimes it's treated as a page frame number, sometimes
> > it's treated as an unshifted physical address, and in one place it's
> > treated as a pte.
> 
> Yuck.

This paragraph should read like this instead:

"Boris used cpa->pfn as a scratch variable to contain the physical
address. He realizes now that he should've added a separate
cpa_data.phys_addr then, instead of confusing everybody."

> > The result of this is that the mapping functions do not map the
> > intended physical address.
> > 
> > This isn't a problem in practice because most of the addresses we're
> > mapping in the EFI code paths are already mapped in 'trampoline_pgd'
> > and so the pageattr mappings functions don't actually do anything in
> > this case. But when we move to using a separate page table for the EFI
> > runtime this will be an issue.
> 
> Are you sure that this does not affect existing kernel versions?

Shouldn't because with this new patchset we're copying all the PGDs from
the kernel page table before doing an EFI call, see
efi_sync_low_kernel_mappings() in patch 5.

> >  	while (num_pages-- && start < end) {
> > -
> > -		/* deal with the NX bit */
> > -		if (!(pgprot_val(pgprot) & _PAGE_NX))
> > -			cpa->pfn &= ~_PAGE_NX;
> 
> That should be a seperate patch because this is just bogus code and
> has nothing to do with the pfn confusion.

Why bogus?

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--