From: Mark Rutland <mark.rutland@arm.com> Date: 2021-08-10 15:30:14
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 <redacted>
Cc: Ard Biesheuvel <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Steve Capper <redacted>
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.
On Tue, 10 Aug 2021 at 17:29, Mark Rutland [off-list ref] wrote:
quoted hunk
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 <redacted>
Cc: Ard Biesheuvel <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Steve Capper <redacted>
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.
From: Mark Rutland <mark.rutland@arm.com> Date: 2021-08-10 16:31:26
On Tue, Aug 10, 2021 at 06:16:49PM +0200, Ard Biesheuvel wrote:
On Tue, 10 Aug 2021 at 17:29, Mark Rutland [off-list ref] wrote:
quoted
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 <redacted>
Cc: Ard Biesheuvel <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Steve Capper <redacted>
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.
From: Will Deacon <will@kernel.org> Date: 2021-08-11 09:55:02
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
From: Mark Rutland <mark.rutland@arm.com> Date: 2021-08-11 10:11:20
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:
quoted
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.