Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1269545 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 1/5] x86/mm/pageattr: Ensure cpa->pfn only contains page frame numbers Matt Fleming <matt@codeblueprint.co.uk> - 2015-11-14 23:10 +0100
    Re: [PATCH v2 1/5] x86/mm/pageattr: Ensure cpa->pfn only contains  page frame numbers Dave Hansen <dave.hansen@intel.com> - 2015-11-16 17:00 +0100
      Re: [PATCH v2 1/5] x86/mm/pageattr: Ensure cpa->pfn only contains  page frame numbers Matt Fleming <matt@codeblueprint.co.uk> - 2015-11-17 10:50 +0100
    Re: [PATCH v2 1/5] x86/mm/pageattr: Ensure cpa->pfn only contains  page frame numbers Thomas Gleixner <tglx@linutronix.de> - 2015-11-16 21:30 +0100
      Re: [PATCH v2 1/5] x86/mm/pageattr: Ensure cpa->pfn only contains  page frame numbers Borislav Petkov <bp@suse.de> - 2015-11-16 22:30 +0100
        Re: [PATCH v2 1/5] x86/mm/pageattr: Ensure cpa->pfn only contains  page frame numbers Thomas Gleixner <tglx@linutronix.de> - 2015-11-16 22:50 +0100
          Re: [PATCH v2 1/5] x86/mm/pageattr: Ensure cpa->pfn only contains  page frame numbers Thomas Gleixner <tglx@linutronix.de> - 2015-11-17 10:00 +0100
      Re: [PATCH v2 1/5] x86/mm/pageattr: Ensure cpa->pfn only contains  page frame numbers Matt Fleming <matt@codeblueprint.co.uk> - 2015-11-17 10:50 +0100
        Re: [PATCH v2 1/5] x86/mm/pageattr: Ensure cpa->pfn only contains  page frame numbers Ingo Molnar <mingo@kernel.org> - 2015-11-18 09:20 +0100
          Re: [PATCH v2 1/5] x86/mm/pageattr: Ensure cpa->pfn only contains  page frame numbers Matt Fleming <matt@codeblueprint.co.uk> - 2015-11-20 13:10 +0100

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

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.

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.

Reviewed-by: Borislav Petkov <bp@suse.de>
Cc: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---

Changes in v2:
 - Folded the deletion of the _PAGE_NX code into this patch.

 arch/x86/mm/pageattr.c         | 17 ++++++-----------
 arch/x86/platform/efi/efi_64.c | 33 ++++++++++++++++++++++-----------
 2 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 9abe0c9b1098..d5240be55915 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -885,15 +885,10 @@ static void populate_pte(struct cpa_data *cpa,
 	pte = pte_offset_kernel(pmd, start);
 
 	while (num_pages-- && start < end) {
-
-		/* deal with the NX bit */
-		if (!(pgprot_val(pgprot) & _PAGE_NX))
-			cpa->pfn &= ~_PAGE_NX;
-
-		set_pte(pte, pfn_pte(cpa->pfn >> PAGE_SHIFT, pgprot));
+		set_pte(pte, pfn_pte(cpa->pfn, pgprot));
 
 		start	 += PAGE_SIZE;
-		cpa->pfn += PAGE_SIZE;
+		cpa->pfn++;
 		pte++;
 	}
 }
@@ -949,11 +944,11 @@ static int populate_pmd(struct cpa_data *cpa,
 
 		pmd = pmd_offset(pud, start);
 
-		set_pmd(pmd, __pmd(cpa->pfn | _PAGE_PSE |
+		set_pmd(pmd, __pmd(cpa->pfn << PAGE_SHIFT | _PAGE_PSE |
 				   massage_pgprot(pmd_pgprot)));
 
 		start	  += PMD_SIZE;
-		cpa->pfn  += PMD_SIZE;
+		cpa->pfn  += PMD_SIZE >> PAGE_SHIFT;
 		cur_pages += PMD_SIZE >> PAGE_SHIFT;
 	}
 
@@ -1022,11 +1017,11 @@ static int populate_pud(struct cpa_data *cpa, unsigned long start, pgd_t *pgd,
 	 * Map everything starting from the Gb boundary, possibly with 1G pages
 	 */
 	while (end - start >= PUD_SIZE) {
-		set_pud(pud, __pud(cpa->pfn | _PAGE_PSE |
+		set_pud(pud, __pud(cpa->pfn << PAGE_SHIFT | _PAGE_PSE |
 				   massage_pgprot(pud_pgprot)));
 
 		start	  += PUD_SIZE;
-		cpa->pfn  += PUD_SIZE;
+		cpa->pfn  += PUD_SIZE >> PAGE_SHIFT;
 		cur_pages += PUD_SIZE >> PAGE_SHIFT;
 		pud++;
 	}
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index a0ac0f9c307f..c8b58ac47b77 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -143,7 +143,7 @@ void efi_sync_low_kernel_mappings(void)
 
 int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 {
-	unsigned long text;
+	unsigned long pfn, text;
 	struct page *page;
 	unsigned npages;
 	pgd_t *pgd;
@@ -160,7 +160,8 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 	 * and ident-map those pages containing the map before calling
 	 * phys_efi_set_virtual_address_map().
 	 */
-	if (kernel_map_pages_in_pgd(pgd, pa_memmap, pa_memmap, num_pages, _PAGE_NX)) {
+	pfn = pa_memmap >> PAGE_SHIFT;
+	if (kernel_map_pages_in_pgd(pgd, pfn, pa_memmap, num_pages, _PAGE_NX)) {
 		pr_err("Error ident-mapping new memmap (0x%lx)!\n", pa_memmap);
 		return 1;
 	}
@@ -176,21 +177,29 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 	if (!IS_ENABLED(CONFIG_EFI_MIXED))
 		return 0;
 
+	npages = (_end - _text) >> PAGE_SHIFT;
+	text = __pa(_text);
+	pfn = text >> PAGE_SHIFT;
+
+	if (kernel_map_pages_in_pgd(pgd, pfn, text, npages, 0)) {
+		pr_err("Failed to map kernel text 1:1\n");
+		return 1;
+	}
+
 	page = alloc_page(GFP_KERNEL|__GFP_DMA32);
 	if (!page)
 		panic("Unable to allocate EFI runtime stack < 4GB\n");
 
 	efi_scratch.phys_stack = virt_to_phys(page_address(page));
-	efi_scratch.phys_stack += PAGE_SIZE; /* stack grows down */
-
-	npages = (_end - _text) >> PAGE_SHIFT;
-	text = __pa(_text);
+	pfn = page_to_pfn(page);
 
-	if (kernel_map_pages_in_pgd(pgd, text >> PAGE_SHIFT, text, npages, 0)) {
-		pr_err("Failed to map kernel text 1:1\n");
+	if (kernel_map_pages_in_pgd(pgd, pfn, efi_scratch.phys_stack, 1, 0)) {
+		pr_err("Failed to map mixed mode stack\n");
 		return 1;
 	}
 
+	efi_scratch.phys_stack += PAGE_SIZE; /* stack grows down */
+
 	return 0;
 }
 
@@ -204,12 +213,14 @@ void __init efi_cleanup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 static void __init __map_region(efi_memory_desc_t *md, u64 va)
 {
 	pgd_t *pgd = (pgd_t *)__va(real_mode_header->trampoline_pgd);
-	unsigned long pf = 0;
+	unsigned long flags = 0;
+	unsigned long pfn;
 
 	if (!(md->attribute & EFI_MEMORY_WB))
-		pf |= _PAGE_PCD;
+		flags |= _PAGE_PCD;
 
-	if (kernel_map_pages_in_pgd(pgd, md->phys_addr, va, md->num_pages, pf))
+	pfn = md->phys_addr >> PAGE_SHIFT;
+	if (kernel_map_pages_in_pgd(pgd, pfn, va, md->num_pages, flags))
 		pr_warn("Error mapping PA 0x%llx -> VA 0x%llx!\n",
 			   md->phys_addr, va);
 }
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

I'm glad you're looking at this. It obviously needed some love. :)

On 11/14/2015 02:00 PM, Matt Fleming wrote:
> +	npages = (_end - _text) >> PAGE_SHIFT;
> +	text = __pa(_text);
> +	pfn = text >> PAGE_SHIFT;
> +
> +	if (kernel_map_pages_in_pgd(pgd, pfn, text, npages, 0)) {
> +		pr_err("Failed to map kernel text 1:1\n");
> +		return 1;
> +	}

Are _end and _text guaranteed to be aligned?  If not, I think the
calculation might be wrong.  Just for fun, imagine that _end=0xfff and
_text=0x1001.  npages would be 0.

Some other code like set_kernel_text_rw() does alignment on _text.

One nit is that there's quite a bit going on here, like rearranging the
phys_stack arithmetic ordering that is far beyond just simplifying the
paddr vs. pfn issue, but that isn't called out in the changelog at all.

Your fixes all look correct to me, fwiw.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On Mon, 16 Nov, at 07:56:17AM, Dave Hansen wrote:
> I'm glad you're looking at this. It obviously needed some love. :)
> 
> On 11/14/2015 02:00 PM, Matt Fleming wrote:
> > +	npages = (_end - _text) >> PAGE_SHIFT;
> > +	text = __pa(_text);
> > +	pfn = text >> PAGE_SHIFT;
> > +
> > +	if (kernel_map_pages_in_pgd(pgd, pfn, text, npages, 0)) {
> > +		pr_err("Failed to map kernel text 1:1\n");
> > +		return 1;
> > +	}
> 
> Are _end and _text guaranteed to be aligned?  If not, I think the
> calculation might be wrong.  Just for fun, imagine that _end=0xfff and
> _text=0x1001.  npages would be 0.
 
Bugger. Good catch, thanks.

> Some other code like set_kernel_text_rw() does alignment on _text.
> 
> One nit is that there's quite a bit going on here, like rearranging the
> phys_stack arithmetic ordering that is far beyond just simplifying the
> paddr vs. pfn issue, but that isn't called out in the changelog at all.
 
Yeah, the phys_stack hunk actually slipped into this patch by
accident. It ensures the stack is mapped into the EFI page tables.
I'll split this out.

> Your fixes all look correct to me, fwiw.

Thanks! If you could respond to the next version with an ACK or
Reviewed-by tag, that'd be great.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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.
 
> 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?

>  	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.

> -
> -		set_pte(pte, pfn_pte(cpa->pfn >> PAGE_SHIFT, pgprot));
> +		set_pte(pte, pfn_pte(cpa->pfn, pgprot));

> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index a0ac0f9c307f..c8b58ac47b77 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -143,7 +143,7 @@ void efi_sync_low_kernel_mappings(void)
>  
>  int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
>  {
> -	unsigned long text;
> +	unsigned long pfn, text;
>  	struct page *page;
>  	unsigned npages;
>  	pgd_t *pgd;
> @@ -160,7 +160,8 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
>  	 * and ident-map those pages containing the map before calling
>  	 * phys_efi_set_virtual_address_map().
>  	 */
> -	if (kernel_map_pages_in_pgd(pgd, pa_memmap, pa_memmap, num_pages, _PAGE_NX)) {
> +	pfn = pa_memmap >> PAGE_SHIFT;
> +	if (kernel_map_pages_in_pgd(pgd, pfn, pa_memmap, num_pages, _PAGE_NX)) {
>  		pr_err("Error ident-mapping new memmap (0x%lx)!\n", pa_memmap);
>  		return 1;
>  	}
> @@ -176,21 +177,29 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
>  	if (!IS_ENABLED(CONFIG_EFI_MIXED))
>  		return 0;
>  
> +	npages = (_end - _text) >> PAGE_SHIFT;

You really need to PFN_ALIGN _end and _text. Has been wrong in the
existing code as well.

> +	text = __pa(_text);
> +	pfn = text >> PAGE_SHIFT;
> +
> +	if (kernel_map_pages_in_pgd(pgd, pfn, text, npages, 0)) {
> +		pr_err("Failed to map kernel text 1:1\n");
> +		return 1;
> +	}
> +
>  	page = alloc_page(GFP_KERNEL|__GFP_DMA32);
>  	if (!page)
>  		panic("Unable to allocate EFI runtime stack < 4GB\n");
>  
>  	efi_scratch.phys_stack = virt_to_phys(page_address(page));
> -	efi_scratch.phys_stack += PAGE_SIZE; /* stack grows down */
> -
> -	npages = (_end - _text) >> PAGE_SHIFT;
> -	text = __pa(_text);
> +	pfn = page_to_pfn(page);
>  
> -	if (kernel_map_pages_in_pgd(pgd, text >> PAGE_SHIFT, text, npages, 0)) {
> -		pr_err("Failed to map kernel text 1:1\n");
> +	if (kernel_map_pages_in_pgd(pgd, pfn, efi_scratch.phys_stack, 1, 0)) {

This looks like an unrelated change, hmm?

> +		pr_err("Failed to map mixed mode stack\n");
>  		return 1;
>  	}
>  
> +	efi_scratch.phys_stack += PAGE_SIZE; /* stack grows down */
> +
>  	return 0;
>  }

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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)
-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On Mon, 16 Nov 2015, Borislav Petkov wrote:
> 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?

Even with cpa->pfn used as an address it cannot ever be set as the
address is page aligned ....

Thanks,

	tglx


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On Mon, 16 Nov 2015, Thomas Gleixner wrote:
> On Mon, 16 Nov 2015, Borislav Petkov wrote:
> > 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?
> 
> Even with cpa->pfn used as an address it cannot ever be set as the
> address is page aligned ....

Gah. Misread it. _PAGE_NX is bit 63 and it can be set when cpa->pfn is
abused as an address. So yes, it should go away with that patch.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On Mon, 16 Nov, at 09:19:01PM, 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.
>  
> > 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?
 
This code only gets called for the EFI code paths (because it's
guarded by the "if (cpa->pgd)" calls).

The code is so wrong that if people were hitting it we would have seen
reports of weird boot and runtime crashes. I'm not aware of any that
could be caused by this, which is why I didn't mark it for stable.

> >  	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.
 
I'm OK either way, but Boris asked me to fold this hunk into this
patch. It was originally a separate patch,

  https://lkml.kernel.org/r/1447342823-3612-3-git-send-email-matt@codeblueprint.co.uk

> > -
> > -		set_pte(pte, pfn_pte(cpa->pfn >> PAGE_SHIFT, pgprot));
> > +		set_pte(pte, pfn_pte(cpa->pfn, pgprot));
> 
> > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> > index a0ac0f9c307f..c8b58ac47b77 100644
> > --- a/arch/x86/platform/efi/efi_64.c
> > +++ b/arch/x86/platform/efi/efi_64.c
> > @@ -143,7 +143,7 @@ void efi_sync_low_kernel_mappings(void)
> >  
> >  int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
> >  {
> > -	unsigned long text;
> > +	unsigned long pfn, text;
> >  	struct page *page;
> >  	unsigned npages;
> >  	pgd_t *pgd;
> > @@ -160,7 +160,8 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
> >  	 * and ident-map those pages containing the map before calling
> >  	 * phys_efi_set_virtual_address_map().
> >  	 */
> > -	if (kernel_map_pages_in_pgd(pgd, pa_memmap, pa_memmap, num_pages, _PAGE_NX)) {
> > +	pfn = pa_memmap >> PAGE_SHIFT;
> > +	if (kernel_map_pages_in_pgd(pgd, pfn, pa_memmap, num_pages, _PAGE_NX)) {
> >  		pr_err("Error ident-mapping new memmap (0x%lx)!\n", pa_memmap);
> >  		return 1;
> >  	}
> > @@ -176,21 +177,29 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
> >  	if (!IS_ENABLED(CONFIG_EFI_MIXED))
> >  		return 0;
> >  
> > +	npages = (_end - _text) >> PAGE_SHIFT;
> 
> You really need to PFN_ALIGN _end and _text. Has been wrong in the
> existing code as well.
 
Hmm... very good point.

> > +	text = __pa(_text);
> > +	pfn = text >> PAGE_SHIFT;
> > +
> > +	if (kernel_map_pages_in_pgd(pgd, pfn, text, npages, 0)) {
> > +		pr_err("Failed to map kernel text 1:1\n");
> > +		return 1;
> > +	}
> > +
> >  	page = alloc_page(GFP_KERNEL|__GFP_DMA32);
> >  	if (!page)
> >  		panic("Unable to allocate EFI runtime stack < 4GB\n");
> >  
> >  	efi_scratch.phys_stack = virt_to_phys(page_address(page));
> > -	efi_scratch.phys_stack += PAGE_SIZE; /* stack grows down */
> > -
> > -	npages = (_end - _text) >> PAGE_SHIFT;
> > -	text = __pa(_text);
> > +	pfn = page_to_pfn(page);
> >  
> > -	if (kernel_map_pages_in_pgd(pgd, text >> PAGE_SHIFT, text, npages, 0)) {
> > -		pr_err("Failed to map kernel text 1:1\n");
> > +	if (kernel_map_pages_in_pgd(pgd, pfn, efi_scratch.phys_stack, 1, 0)) {
> 
> This looks like an unrelated change, hmm?

Dave picked up on this too. Yeah, this hunk should really be part of
PATCH 2 (or a separate patch entirely) because it ensures that the
stack is mapped into the EFI page tables instead of relying on it
being around in 'trampoline_pgd'.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

* Matt Fleming <matt@codeblueprint.co.uk> wrote:

> > > +	npages = (_end - _text) >> PAGE_SHIFT;
> > 
> > You really need to PFN_ALIGN _end and _text. Has been wrong in the
> > existing code as well.
>  
> Hmm... very good point.

So I think we should instead guarantee that _end and _text are page aligned.

_text is already page aligned:

SECTIONS
{
#ifdef CONFIG_X86_32
        . = LOAD_OFFSET + LOAD_PHYSICAL_ADDR;
        phys_startup_32 = startup_32 - LOAD_OFFSET;
#else
        . = __START_KERNEL;
        phys_startup_64 = startup_64 - LOAD_OFFSET;
#endif

        /* Text and read-only data */
        .text :  AT(ADDR(.text) - LOAD_OFFSET) {
                _text = .;

The reason for aligning _end as well is that we already page-align the BSS and BRK 
sections of the kernel and its various section boundary symbols:

        /* BSS */
        . = ALIGN(PAGE_SIZE);
        .bss : AT(ADDR(.bss) - LOAD_OFFSET) {
                __bss_start = .;
                *(.bss..page_aligned)
                *(.bss)
                . = ALIGN(PAGE_SIZE);
                __bss_stop = .;
        }

        . = ALIGN(PAGE_SIZE);
        .brk : AT(ADDR(.brk) - LOAD_OFFSET) {
                __brk_base = .;
                . += 64 * 1024;         /* 64k alignment slop space */
                *(.brk_reservation)     /* areas brk users have reserved */
                __brk_limit = .;
        }

        _end = .;

        STABS_DEBUG
        DWARF_DEBUG

_end is the only odd one out, so we should align it as well - because it's easy to 
make such pfn conversion bugs.

This will also make it easier to mark STABS_DEBUG and DWARF_DEBUG as read-only, 
which they should fundamentally be I think. Alternatively they could be moved to 
the read-only section - at which point _end becomes page aligned 'for free'.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On Wed, 18 Nov, at 09:14:23AM, Ingo Molnar wrote:
> 
> * Matt Fleming <matt@codeblueprint.co.uk> wrote:
> 
> > > > +	npages = (_end - _text) >> PAGE_SHIFT;
> > > 
> > > You really need to PFN_ALIGN _end and _text. Has been wrong in the
> > > existing code as well.
> >  
> > Hmm... very good point.
> 
> So I think we should instead guarantee that _end and _text are page aligned.
> 
> _text is already page aligned:
> 
> SECTIONS
> {
> #ifdef CONFIG_X86_32
>         . = LOAD_OFFSET + LOAD_PHYSICAL_ADDR;
>         phys_startup_32 = startup_32 - LOAD_OFFSET;
> #else
>         . = __START_KERNEL;
>         phys_startup_64 = startup_64 - LOAD_OFFSET;
> #endif
> 
>         /* Text and read-only data */
>         .text :  AT(ADDR(.text) - LOAD_OFFSET) {
>                 _text = .;
> 
> The reason for aligning _end as well is that we already page-align the BSS and BRK 
> sections of the kernel and its various section boundary symbols:
> 
>         /* BSS */
>         . = ALIGN(PAGE_SIZE);
>         .bss : AT(ADDR(.bss) - LOAD_OFFSET) {
>                 __bss_start = .;
>                 *(.bss..page_aligned)
>                 *(.bss)
>                 . = ALIGN(PAGE_SIZE);
>                 __bss_stop = .;
>         }
> 
>         . = ALIGN(PAGE_SIZE);
>         .brk : AT(ADDR(.brk) - LOAD_OFFSET) {
>                 __brk_base = .;
>                 . += 64 * 1024;         /* 64k alignment slop space */
>                 *(.brk_reservation)     /* areas brk users have reserved */
>                 __brk_limit = .;
>         }
> 
>         _end = .;
> 
>         STABS_DEBUG
>         DWARF_DEBUG
> 
> _end is the only odd one out, so we should align it as well - because it's easy to 
> make such pfn conversion bugs.

FWIW, I saw no changes in either 32-bit or 64-bit vmlinux size when
building with the following patch, so it seems like a pretty easy win,

---

From 25ad518fa52e589f110376ae06e42fb20b3e4188 Mon Sep 17 00:00:00 2001
From: Matt Fleming <matt@codeblueprint.co.uk>
Date: Fri, 20 Nov 2015 11:46:11 +0000
Subject: [PATCH] x86: Page align _end to avoid pfn conversion bugs

Ingo noted that if we can guarantee _end is aligned to PAGE_SIZE we
can automatically avoid bugs along the lines of,

	size = _end - _text >> PAGE_SHIFT

which is missing a call to PFN_ALIGN(). The EFI mixed mode contains
this bug, for example.

_text is already aligned to PAGE_SIZE through the use of
LOAD_PHYSICAL_ADDR, and the BSS and BRK sections are explicitly
aligned in the linker script, so it makes sense to align _end to
match.

Reported-by: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "H . Peter Anvin" <hpa@zytor.com>
Cc: Toshi Kani <toshi.kani@hp.com>
Cc: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 arch/x86/kernel/vmlinux.lds.S | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index 74e4bf11f562..4f1994257a18 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -325,6 +325,7 @@ SECTIONS
 		__brk_limit = .;
 	}
 
+	. = ALIGN(PAGE_SIZE);
 	_end = .;
 
         STABS_DEBUG
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web