arm64/kernel: Always use level 2 or higher for early mappings
PATCH v2 1/1 arm64/kernel: Always use level 2 or higher for early mappings
From: Ard Biesheuvel <[email protected]>
The page table population code in map_range() uses a recursive algorithm to create the early mappings of the kernel, the DTB and the ID mapped text and data pages, and this fails to take into account that the way these page tables may be constructed is not precisely the same at each level. In particular, block mappings are not permitted at each level, and the code as it exists today might inadvertently create such a forbidden block mapping if it were used to map a region of the appropriate size and alignment.
This never happens in practice, given the limited size of the assets being mapped by the early boot code. Nonetheless, it would be better if this code would behave correctly in all circumstances.
So only permit block mappings at level 2, and page mappings at level 3, for any page size, and use table mappings exclusively at all other levels. This change should have no impact in practice, but it makes the code more robust.
---
v2: take the assignment of protval out of the loop again, so that
clearing a mapping works as expected wrt the PTE_CONT bit
arch/arm64/kernel/pi/map_range.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
arch/arm64/kernel/pi/map_range.c +2 -2
@@ -45,12 +45,12 @@ void __init map_range(u64 *pte, u64 start, u64 end, u64 pa, pgprot_t prot,* clearing the mapping */ if (protval) - protval |= (level <3 ) ?PMD_TYPE_SECT :PTE_TYPE_PAGE ; + protval |= (level ==2 ) ?PMD_TYPE_SECT :PTE_TYPE_PAGE ;while (start < end) {u64 next =min ((start | lmask) +1 ,PAGE_ALIGN (end)); -if (level <3 && (start | next | pa) & lmask) { +if (level <2 || (level ==2 && (start | next | pa) & lmask)) {/* * This chunk needs a finer grained mapping. Create a * table mapping if necessary and recurse.
Signature · 1 line
2.49.0.rc0.332.g42c0ae87b1-goog
Quoted 39 lines · Ard Biesheuvel
From: Ard Biesheuvel <[email protected]>
The page table population code in
map_range()uses a recursive algorithm to create the early mappings of the kernel, the DTB and the ID mapped text and data pages, and this fails to take into account that the way these page tables may be constructed is not precisely the same at each level. In particular, block mappings are not permitted at each level, and the code as it exists today might inadvertently create such a forbidden block mapping if it were used to map a region of the appropriate size and alignment.This never happens in practice, given the limited size of the assets being mapped by the early boot code. Nonetheless, it would be better if this code would behave correctly in all circumstances.
So only permit block mappings at level 2, and page mappings at level 3, for any page size, and use table mappings exclusively at all other levels. This change should have no impact in practice, but it makes the code more robust.
Cc Anshuman Khandual <[email protected]>Reported-by Ryan Roberts <[email protected]>Signed-off-by Ard Biesheuvel <[email protected]>---
v2: take the assignment of protval out of the loop again, so that
clearing a mapping works as expected wrt thePTE_CONTbitarch/arm64/kernel/pi/map_range.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)arch/arm64/kernel/pi/map_range.c +0
-1@@ -45,12 +45,12 @@ void __init map_range(u64 *pte, u64 start, u64 end, u64 pa, pgprot_t prot,* clearing the mapping */ if (protval) - protval |= (level <3 ) ?PMD_TYPE_SECT :PTE_TYPE_PAGE ;
arch/arm64/kernel/pi/map_range.c void __init map_range(u64 *pte, u64 start, u64 end, u64 pa, pgprot_t prot, · line 56
Quoted 10 lines
arch/arm64/kernel/pi/map_range.c +2
-1+ protval |= (level ==2 ) ?PMD_TYPE_SECT :PTE_TYPE_PAGE ;while (start < end) {u64 next =min ((start | lmask) +1 ,PAGE_ALIGN (end)); -if (level <3 && (start | next | pa) & lmask) { +if (level <2 || (level ==2 && (start | next | pa) & lmask)) {/* * This chunk needs a finer grained mapping. Create a * table mapping if necessary and recurse.
Again did not see any problem running this on various 4K/16K/64K page configs.
Quoted 20 lines · Ard Biesheuvel
From: Ard Biesheuvel <[email protected]>
The page table population code in
map_range()uses a recursive algorithm to create the early mappings of the kernel, the DTB and the ID mapped text and data pages, and this fails to take into account that the way these page tables may be constructed is not precisely the same at each level. In particular, block mappings are not permitted at each level, and the code as it exists today might inadvertently create such a forbidden block mapping if it were used to map a region of the appropriate size and alignment.This never happens in practice, given the limited size of the assets being mapped by the early boot code. Nonetheless, it would be better if this code would behave correctly in all circumstances.
So only permit block mappings at level 2, and page mappings at level 3, for any page size, and use table mappings exclusively at all other levels. This change should have no impact in practice, but it makes the code more robust.
Quoted 6 lines
Cc Anshuman Khandual <[email protected]>Reported-by Ryan Roberts <[email protected]>Signed-off-by Ard Biesheuvel <[email protected]>---
v2: take the assignment of protval out of the loop again, so that
clearing a mapping works as expected wrt thePTE_CONTbit
Ouch, totally missed that. Now that I've looked at the code properly, I wonder if it is worth supporting CONT_PMD mappings at level 2? That would be 32M for 4K pages, so is likely to actaually get used. I think it would just be something like:
u64 cmask = (level == 3) ? CONT_PTE_SIZE - 1 :
((level == 2) ? CONT_PMD_SIZE - 1 : U64_MAX) : U64_MAX;
Anyway, probably not worth the complexity for these boot-time tables, so regardless:
Quoted 23 lines
arch/arm64/kernel/pi/map_range.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)arch/arm64/kernel/pi/map_range.c +2
-2@@ -45,12 +45,12 @@ void __init map_range(u64 *pte, u64 start, u64 end, u64 pa, pgprot_t prot,* clearing the mapping */ if (protval) - protval |= (level <3 ) ?PMD_TYPE_SECT :PTE_TYPE_PAGE ; + protval |= (level ==2 ) ?PMD_TYPE_SECT :PTE_TYPE_PAGE ;while (start < end) {u64 next =min ((start | lmask) +1 ,PAGE_ALIGN (end)); -if (level <3 && (start | next | pa) & lmask) { +if (level <2 || (level ==2 && (start | next | pa) & lmask)) {/* * This chunk needs a finer grained mapping. Create a * table mapping if necessary and recurse.
Quoted 29 lines · Ryan Roberts
Quoted 20 lines · Ard Biesheuvel
From: Ard Biesheuvel <[email protected]>
The page table population code in
map_range()uses a recursive algorithm to create the early mappings of the kernel, the DTB and the ID mapped text and data pages, and this fails to take into account that the way these page tables may be constructed is not precisely the same at each level. In particular, block mappings are not permitted at each level, and the code as it exists today might inadvertently create such a forbidden block mapping if it were used to map a region of the appropriate size and alignment.This never happens in practice, given the limited size of the assets being mapped by the early boot code. Nonetheless, it would be better if this code would behave correctly in all circumstances.
So only permit block mappings at level 2, and page mappings at level 3, for any page size, and use table mappings exclusively at all other levels. This change should have no impact in practice, but it makes the code more robust.
Quoted 6 lines
Cc Anshuman Khandual <[email protected]>Reported-by Ryan Roberts <[email protected]>Signed-off-by Ard Biesheuvel <[email protected]>---
v2: take the assignment of protval out of the loop again, so that
clearing a mapping works as expected wrt thePTE_CONTbit
Quoted 7 lines
Ouch, totally missed that. Now that I've looked at the code properly, I wonder if it is worth supporting
CONT_PMDmappings at level 2? That would be 32M for 4K pages, so is likely to actaually get used. I think it would just be something like:u64 cmask = (level == 3) ?
CONT_PTE_SIZE- 1 :((level == 2) ? CONT_PMD_SIZE - 1 : U64_MAX) : U64_MAX;
With this patch applied, it is guaranteed that cmask is only referenced if level >= 2, so we can simplify this as
u64 cmask = (level < 3) ? CONT_PMD_SIZE - 1 : CONT_PTE_SIZE - 1;
(using the level < 3 for consistency with the protval assignment)
Quoted 2 lines
Anyway, probably not worth the complexity for these boot-time tables, so regardless:
They are created at boot time but these are not 'boot-time tables' - they are the ones that are used by the kernel at runtime. So I think it makes sense to add this.
Quoted 2 lines
Reviewed-by Ryan Roberts <[email protected]>
Thanks.
Quoted 34 lines · Ard Biesheuvel
Quoted 33 lines · Ryan Roberts
Quoted 20 lines · Ard Biesheuvel
From: Ard Biesheuvel <[email protected]>
The page table population code in
map_range()uses a recursive algorithm to create the early mappings of the kernel, the DTB and the ID mapped text and data pages, and this fails to take into account that the way these page tables may be constructed is not precisely the same at each level. In particular, block mappings are not permitted at each level, and the code as it exists today might inadvertently create such a forbidden block mapping if it were used to map a region of the appropriate size and alignment.This never happens in practice, given the limited size of the assets being mapped by the early boot code. Nonetheless, it would be better if this code would behave correctly in all circumstances.
So only permit block mappings at level 2, and page mappings at level 3, for any page size, and use table mappings exclusively at all other levels. This change should have no impact in practice, but it makes the code more robust.
Quoted 6 lines
Cc Anshuman Khandual <[email protected]>Reported-by Ryan Roberts <[email protected]>Signed-off-by Ard Biesheuvel <[email protected]>---
v2: take the assignment of protval out of the loop again, so that
clearing a mapping works as expected wrt thePTE_CONTbitOuch, totally missed that. Now that I've looked at the code properly, I wonder if it is worth supporting
CONT_PMDmappings at level 2? That would be 32M for 4K pages, so is likely to actaually get used. I think it would just be something like:
Quoted 8 lines
Quoted 3 lines
u64 cmask = (level == 3) ?
CONT_PTE_SIZE- 1 :((level == 2) ? CONT_PMD_SIZE - 1 : U64_MAX) : U64_MAX;With this patch applied, it is guaranteed that cmask is only referenced if level >= 2, so we can simplify this as
u64 cmask = (level < 3) ? CONT_PMD_SIZE - 1 : CONT_PTE_SIZE - 1;
Ahh good point.
Quoted 9 lines
(using the level < 3 for consistency with the protval assignment)
Quoted 2 lines
Anyway, probably not worth the complexity for these boot-time tables, so regardless:
They are created at boot time but these are not 'boot-time tables' - they are the ones that are used by the kernel at runtime. So I think it makes sense to add this.
Hmm, for some reason I thought everything was being remapped in map_mem(), but looking again, I guess the kernel image mappings put down by map_range() remain.
I'll add a todo to send this out when I get around to it. Although I guess the chances of having a portion of the text or data being 32M aligned and sized are quite slim given the image only needs to be loaded on a 2M boundary?
Quoted 5 lines
Quoted 3 lines
Reviewed-by Ryan Roberts <[email protected]>Thanks.
Quoted 10 lines · Ard Biesheuvel
The page table population code in
map_range()uses a recursive algorithm to create the early mappings of the kernel, the DTB and the ID mapped text and data pages, and this fails to take into account that the way these page tables may be constructed is not precisely the same at each level. In particular, block mappings are not permitted at each level, and the code as it exists today might inadvertently create such a forbidden block mapping if it were used to map a region of the appropriate size and alignment.[...]
Applied to arm64 (for-next/pgtable-cleanups), thanks!
[1/1] arm64/kernel: Always use level 2 or higher for early mappings
https://git.kernel.org/arm64/c/bf25266f8382
Signature · 1 line
Catalin