All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] arm64: head: avoid over-mapping in map_memory
@ 2021-08-10 15:27 Mark Rutland
  2021-08-10 16:16 ` Ard Biesheuvel
  2021-08-11  9:44 ` Will Deacon
  0 siblings, 2 replies; 5+ messages in thread
From: Mark Rutland @ 2021-08-10 15:27 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: anshuman.khandual, ard.biesheuvel, catalin.marinas, mark.rutland,
	steve.capper, will

The `compute_indices` and `populate_entries` macros operate on inclusive
bounds, and thus the `map_memory` macro which uses them also operates
on inclusive bounds.

We pass `_end` and `_idmap_text_end` to `map_memory`, but these are
exclusive bounds, and if one of these is sufficiently aligned (as a
result of kernel configuration, physical placement, and KASLR), then:

* In `compute_indices`, the computed `iend` will be in the page/block *after*
  the final byte of the intended mapping.

* In `populate_entries`, an unnecessary entry will be created at the end
  of each level of table. At the leaf level, this entry will map up to
  SWAPPER_BLOCK_SIZE bytes of physical addresses that we did not intend
  to map.

As we may map up to SWAPPER_BLOCK_SIZE bytes more than intended, we may
violate the boot protocol and map physical address past the 2MiB-aligned
end address we are permitted to map. As we map these with Normal memory
attributes, this may result in further problems depending on what these
physical addresses correspond to.

Fix this by subtracting one from the end address in both cases, such
that we always use inclusive bounds. For clarity, comments are updated
to more clearly document that the macros expect inclusive bounds.

Fixes: 0370b31e48454d8c ("arm64: Extend early page table code to allow for larger kernel")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Steve Capper <steve.capper@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/head.S | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

I spotted this while working on some rework of the early page table code.
While the rest isn't ready yet, I thought I'd send this out on its own as it's
a fix.

Mark.

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index c5c994a73a64..f0826be4c104 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -176,8 +176,8 @@ SYM_CODE_END(preserve_boot_args)
  * were needed in the previous page table level then the next page table level is assumed
  * to be composed of multiple pages. (This effectively scales the end index).
  *
- *	vstart:	virtual address of start of range
- *	vend:	virtual address of end of range
+ *	vstart:	virtual address of start of range (inclusive)
+ *	vend:	virtual address of end of range (inclusive)
  *	shift:	shift used to transform virtual address into index
  *	ptrs:	number of entries in page table
  *	istart:	index in table corresponding to vstart
@@ -214,8 +214,8 @@ SYM_CODE_END(preserve_boot_args)
  *
  *	tbl:	location of page table
  *	rtbl:	address to be used for first level page table entry (typically tbl + PAGE_SIZE)
- *	vstart:	start address to map
- *	vend:	end address to map - we map [vstart, vend]
+ *	vstart:	virtual address of start of mapping (inclusive)
+ *	vend:	virtual address of end of mapping (inclusive)
  *	flags:	flags to use to map last level entries
  *	phys:	physical address corresponding to vstart - physical memory is contiguous
  *	pgds:	the number of pgd entries
@@ -355,6 +355,7 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
 1:
 	ldr_l	x4, idmap_ptrs_per_pgd
 	adr_l	x6, __idmap_text_end		// __pa(__idmap_text_end)
+	sub	x6, x6, #1
 
 	map_memory x0, x1, x3, x6, x7, x3, x4, x10, x11, x12, x13, x14
 
@@ -366,6 +367,7 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
 	add	x5, x5, x23			// add KASLR displacement
 	mov	x4, PTRS_PER_PGD
 	adrp	x6, _end			// runtime __pa(_end)
+	sub	x6, x6, #1
 	adrp	x3, _text			// runtime __pa(_text)
 	sub	x6, x6, x3			// _end - _text
 	add	x6, x6, x5			// runtime __va(_end)
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] arm64: head: avoid over-mapping in map_memory
  2021-08-10 15:27 [PATCH] arm64: head: avoid over-mapping in map_memory Mark Rutland
@ 2021-08-10 16:16 ` Ard Biesheuvel
  2021-08-10 16:28   ` Mark Rutland
  2021-08-11  9:44 ` Will Deacon
  1 sibling, 1 reply; 5+ messages in thread
From: Ard Biesheuvel @ 2021-08-10 16:16 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Linux ARM, Anshuman Khandual, Ard Biesheuvel, Catalin Marinas,
	Steve Capper, Will Deacon

On Tue, 10 Aug 2021 at 17:29, Mark Rutland <mark.rutland@arm.com> wrote:
>
> The `compute_indices` and `populate_entries` macros operate on inclusive
> bounds, and thus the `map_memory` macro which uses them also operates
> on inclusive bounds.
>
> We pass `_end` and `_idmap_text_end` to `map_memory`, but these are
> exclusive bounds, and if one of these is sufficiently aligned (as a
> result of kernel configuration, physical placement, and KASLR), then:
>
> * In `compute_indices`, the computed `iend` will be in the page/block *after*
>   the final byte of the intended mapping.
>
> * In `populate_entries`, an unnecessary entry will be created at the end
>   of each level of table. At the leaf level, this entry will map up to
>   SWAPPER_BLOCK_SIZE bytes of physical addresses that we did not intend
>   to map.
>
> As we may map up to SWAPPER_BLOCK_SIZE bytes more than intended, we may
> violate the boot protocol and map physical address past the 2MiB-aligned
> end address we are permitted to map. As we map these with Normal memory
> attributes, this may result in further problems depending on what these
> physical addresses correspond to.
>
> Fix this by subtracting one from the end address in both cases, such
> that we always use inclusive bounds. For clarity, comments are updated
> to more clearly document that the macros expect inclusive bounds.
>
> Fixes: 0370b31e48454d8c ("arm64: Extend early page table code to allow for larger kernel")
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Steve Capper <steve.capper@arm.com>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kernel/head.S | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> I spotted this while working on some rework of the early page table code.
> While the rest isn't ready yet, I thought I'd send this out on its own as it's
> a fix.
>
> Mark.
>
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index c5c994a73a64..f0826be4c104 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -176,8 +176,8 @@ SYM_CODE_END(preserve_boot_args)
>   * were needed in the previous page table level then the next page table level is assumed
>   * to be composed of multiple pages. (This effectively scales the end index).
>   *
> - *     vstart: virtual address of start of range
> - *     vend:   virtual address of end of range
> + *     vstart: virtual address of start of range (inclusive)
> + *     vend:   virtual address of end of range (inclusive)
>   *     shift:  shift used to transform virtual address into index
>   *     ptrs:   number of entries in page table
>   *     istart: index in table corresponding to vstart
> @@ -214,8 +214,8 @@ SYM_CODE_END(preserve_boot_args)
>   *
>   *     tbl:    location of page table
>   *     rtbl:   address to be used for first level page table entry (typically tbl + PAGE_SIZE)
> - *     vstart: start address to map
> - *     vend:   end address to map - we map [vstart, vend]
> + *     vstart: virtual address of start of mapping (inclusive)
> + *     vend:   virtual address of end of mapping (inclusive)
>   *     flags:  flags to use to map last level entries
>   *     phys:   physical address corresponding to vstart - physical memory is contiguous
>   *     pgds:   the number of pgd entries
> @@ -355,6 +355,7 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
>  1:
>         ldr_l   x4, idmap_ptrs_per_pgd
>         adr_l   x6, __idmap_text_end            // __pa(__idmap_text_end)
> +       sub     x6, x6, #1
>

__idmap_text_end-1 should do the trick as well, no?

>         map_memory x0, x1, x3, x6, x7, x3, x4, x10, x11, x12, x13, x14
>
> @@ -366,6 +367,7 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
>         add     x5, x5, x23                     // add KASLR displacement
>         mov     x4, PTRS_PER_PGD
>         adrp    x6, _end                        // runtime __pa(_end)
> +       sub     x6, x6, #1
>         adrp    x3, _text                       // runtime __pa(_text)
>         sub     x6, x6, x3                      // _end - _text
>         add     x6, x6, x5                      // runtime __va(_end)
> --
> 2.11.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] arm64: head: avoid over-mapping in map_memory
  2021-08-10 16:16 ` Ard Biesheuvel
@ 2021-08-10 16:28   ` Mark Rutland
  0 siblings, 0 replies; 5+ messages in thread
From: Mark Rutland @ 2021-08-10 16:28 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Linux ARM, Anshuman Khandual, Ard Biesheuvel, Catalin Marinas,
	Steve Capper, Will Deacon

On Tue, Aug 10, 2021 at 06:16:49PM +0200, Ard Biesheuvel wrote:
> On Tue, 10 Aug 2021 at 17:29, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > The `compute_indices` and `populate_entries` macros operate on inclusive
> > bounds, and thus the `map_memory` macro which uses them also operates
> > on inclusive bounds.
> >
> > We pass `_end` and `_idmap_text_end` to `map_memory`, but these are
> > exclusive bounds, and if one of these is sufficiently aligned (as a
> > result of kernel configuration, physical placement, and KASLR), then:
> >
> > * In `compute_indices`, the computed `iend` will be in the page/block *after*
> >   the final byte of the intended mapping.
> >
> > * In `populate_entries`, an unnecessary entry will be created at the end
> >   of each level of table. At the leaf level, this entry will map up to
> >   SWAPPER_BLOCK_SIZE bytes of physical addresses that we did not intend
> >   to map.
> >
> > As we may map up to SWAPPER_BLOCK_SIZE bytes more than intended, we may
> > violate the boot protocol and map physical address past the 2MiB-aligned
> > end address we are permitted to map. As we map these with Normal memory
> > attributes, this may result in further problems depending on what these
> > physical addresses correspond to.
> >
> > Fix this by subtracting one from the end address in both cases, such
> > that we always use inclusive bounds. For clarity, comments are updated
> > to more clearly document that the macros expect inclusive bounds.
> >
> > Fixes: 0370b31e48454d8c ("arm64: Extend early page table code to allow for larger kernel")
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Steve Capper <steve.capper@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > ---
> >  arch/arm64/kernel/head.S | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > I spotted this while working on some rework of the early page table code.
> > While the rest isn't ready yet, I thought I'd send this out on its own as it's
> > a fix.
> >
> > Mark.
> >
> > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > index c5c994a73a64..f0826be4c104 100644
> > --- a/arch/arm64/kernel/head.S
> > +++ b/arch/arm64/kernel/head.S
> > @@ -176,8 +176,8 @@ SYM_CODE_END(preserve_boot_args)
> >   * were needed in the previous page table level then the next page table level is assumed
> >   * to be composed of multiple pages. (This effectively scales the end index).
> >   *
> > - *     vstart: virtual address of start of range
> > - *     vend:   virtual address of end of range
> > + *     vstart: virtual address of start of range (inclusive)
> > + *     vend:   virtual address of end of range (inclusive)
> >   *     shift:  shift used to transform virtual address into index
> >   *     ptrs:   number of entries in page table
> >   *     istart: index in table corresponding to vstart
> > @@ -214,8 +214,8 @@ SYM_CODE_END(preserve_boot_args)
> >   *
> >   *     tbl:    location of page table
> >   *     rtbl:   address to be used for first level page table entry (typically tbl + PAGE_SIZE)
> > - *     vstart: start address to map
> > - *     vend:   end address to map - we map [vstart, vend]
> > + *     vstart: virtual address of start of mapping (inclusive)
> > + *     vend:   virtual address of end of mapping (inclusive)
> >   *     flags:  flags to use to map last level entries
> >   *     phys:   physical address corresponding to vstart - physical memory is contiguous
> >   *     pgds:   the number of pgd entries
> > @@ -355,6 +355,7 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
> >  1:
> >         ldr_l   x4, idmap_ptrs_per_pgd
> >         adr_l   x6, __idmap_text_end            // __pa(__idmap_text_end)
> > +       sub     x6, x6, #1
> >
> 
> __idmap_text_end-1 should do the trick as well, no?

Yup. If you want, I can make that:

	adr_l   x6, __idmap_text_end - 1	// __pa(__idmap_text_end - 1)

> > @@ -366,6 +367,7 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
> >         add     x5, x5, x23                     // add KASLR displacement
> >         mov     x4, PTRS_PER_PGD
> >         adrp    x6, _end                        // runtime __pa(_end)
> > +       sub     x6, x6, #1

... and likewise here:

	adr_l x6, _end - 1			// runtime __pa(_end  - 1)

Thanks,
Mark.

> >         adrp    x3, _text                       // runtime __pa(_text)
> >         sub     x6, x6, x3                      // _end - _text
> >         add     x6, x6, x5                      // runtime __va(_end)
> > --
> > 2.11.0
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] arm64: head: avoid over-mapping in map_memory
  2021-08-10 15:27 [PATCH] arm64: head: avoid over-mapping in map_memory Mark Rutland
  2021-08-10 16:16 ` Ard Biesheuvel
@ 2021-08-11  9:44 ` Will Deacon
  2021-08-11 10:09   ` Mark Rutland
  1 sibling, 1 reply; 5+ messages in thread
From: Will Deacon @ 2021-08-11  9:44 UTC (permalink / raw)
  To: Mark Rutland
  Cc: linux-arm-kernel, anshuman.khandual, ard.biesheuvel,
	catalin.marinas, steve.capper

Hi Mark,

On Tue, Aug 10, 2021 at 04:27:56PM +0100, Mark Rutland wrote:
> The `compute_indices` and `populate_entries` macros operate on inclusive
> bounds, and thus the `map_memory` macro which uses them also operates
> on inclusive bounds.
> 
> We pass `_end` and `_idmap_text_end` to `map_memory`, but these are
> exclusive bounds, and if one of these is sufficiently aligned (as a
> result of kernel configuration, physical placement, and KASLR), then:
> 
> * In `compute_indices`, the computed `iend` will be in the page/block *after*
>   the final byte of the intended mapping.
> 
> * In `populate_entries`, an unnecessary entry will be created at the end
>   of each level of table. At the leaf level, this entry will map up to
>   SWAPPER_BLOCK_SIZE bytes of physical addresses that we did not intend
>   to map.
> 
> As we may map up to SWAPPER_BLOCK_SIZE bytes more than intended, we may
> violate the boot protocol and map physical address past the 2MiB-aligned
> end address we are permitted to map. As we map these with Normal memory
> attributes, this may result in further problems depending on what these
> physical addresses correspond to.
> 
> Fix this by subtracting one from the end address in both cases, such
> that we always use inclusive bounds. For clarity, comments are updated
> to more clearly document that the macros expect inclusive bounds.

I think the comments are already clear for populate_entries and map_memory,
so just adding something to compute_indices should suffice. As-is, your
patch leaves populate_entries inconsistent with the others.

That aside, I don't think any amount of comments will help because it's
just plain weird for vend to be inclusive for map_memory, no? Given that
both (all) callers got this wrong, I'd be inclined to say that map_memory
should take an exclusive 'vend' as you'd normally expect and it can adjust
it before using the helper macros (which should perhaps have 'inclusive' in
their names).

Although we currently preserve 'vend' in map_memory, that doesn't appear to
be necessary so we can just move it to the list of clobbers in the comment
nobody reads.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] arm64: head: avoid over-mapping in map_memory
  2021-08-11  9:44 ` Will Deacon
@ 2021-08-11 10:09   ` Mark Rutland
  0 siblings, 0 replies; 5+ messages in thread
From: Mark Rutland @ 2021-08-11 10:09 UTC (permalink / raw)
  To: Will Deacon
  Cc: linux-arm-kernel, anshuman.khandual, ard.biesheuvel,
	catalin.marinas, steve.capper

On Wed, Aug 11, 2021 at 10:44:58AM +0100, Will Deacon wrote:
> Hi Mark,
> 
> On Tue, Aug 10, 2021 at 04:27:56PM +0100, Mark Rutland wrote:
> > The `compute_indices` and `populate_entries` macros operate on inclusive
> > bounds, and thus the `map_memory` macro which uses them also operates
> > on inclusive bounds.
> > 
> > We pass `_end` and `_idmap_text_end` to `map_memory`, but these are
> > exclusive bounds, and if one of these is sufficiently aligned (as a
> > result of kernel configuration, physical placement, and KASLR), then:
> > 
> > * In `compute_indices`, the computed `iend` will be in the page/block *after*
> >   the final byte of the intended mapping.
> > 
> > * In `populate_entries`, an unnecessary entry will be created at the end
> >   of each level of table. At the leaf level, this entry will map up to
> >   SWAPPER_BLOCK_SIZE bytes of physical addresses that we did not intend
> >   to map.
> > 
> > As we may map up to SWAPPER_BLOCK_SIZE bytes more than intended, we may
> > violate the boot protocol and map physical address past the 2MiB-aligned
> > end address we are permitted to map. As we map these with Normal memory
> > attributes, this may result in further problems depending on what these
> > physical addresses correspond to.
> > 
> > Fix this by subtracting one from the end address in both cases, such
> > that we always use inclusive bounds. For clarity, comments are updated
> > to more clearly document that the macros expect inclusive bounds.
> 
> I think the comments are already clear for populate_entries and map_memory,
> so just adding something to compute_indices should suffice. As-is, your
> patch leaves populate_entries inconsistent with the others.

Sorry, I'd left that in a separate patch that also renamed index/eindex
to istart/iend for consistency with compute_indices. I should have
folded that change in.

> That aside, I don't think any amount of comments will help because it's
> just plain weird for vend to be inclusive for map_memory, no?

I guess; I have no strong feeling either way.

> Given that both (all) callers got this wrong, I'd be inclined to say
> that map_memory should take an exclusive 'vend' as you'd normally
> expect and it can adjust it before using the helper macros (which
> should perhaps have 'inclusive' in their names).

Sure, I'm also happy with that.

> Although we currently preserve 'vend' in map_memory, that doesn't
> appear to be necessary so we can just move it to the list of clobbers
> in the comment nobody reads.

Works for me.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-08-11 10:11 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-10 15:27 [PATCH] arm64: head: avoid over-mapping in map_memory Mark Rutland
2021-08-10 16:16 ` Ard Biesheuvel
2021-08-10 16:28   ` Mark Rutland
2021-08-11  9:44 ` Will Deacon
2021-08-11 10:09   ` Mark Rutland

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.