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