| Message ID | 1488450976-16257-2-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ArmPkg, ArmVirtpkg ARM: enable strict memory protection | expand |
On Thu, Mar 02, 2017 at 10:36:13AM +0000, Ard Biesheuvel wrote: > Currently, any range passed to CpuArchProtocol::SetMemoryAttributes is > fully broken down into page mappings if the start or the size of the > region happens to be misaliged relative to the section size of 1 MB. > > This is going to hurt when we enable strict memory permissions, given "Hurt" -> "cause unnecessary performance penalties" or "hurt" -> "break everything"? > that we remap the entire RAM space non-executable (modulo the code > bits) when the CpuArchProtocol is installed. > > So refactor the code to iterate over the range in a way that ensures > that all naturally aligned section sized subregions are not broken up. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Many thanks for getting rid of the magic values, and in general making the code more logical. One question below: > --- > ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 47 ++++++++++++++++---- > 1 file changed, 39 insertions(+), 8 deletions(-) > > diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c > index 89e429925ba9..ce4d529bda67 100644 > --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c > +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c > @@ -679,6 +679,7 @@ SetMemoryAttributes ( > ) > { > EFI_STATUS Status; > + UINT64 ChunkLength; > > // > // Ignore invocations that only modify permission bits > @@ -687,14 +688,44 @@ SetMemoryAttributes ( > return EFI_SUCCESS; > } > > - if(((BaseAddress & 0xFFFFF) == 0) && ((Length & 0xFFFFF) == 0)) { > - // Is the base and length a multiple of 1 MB? > - DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU section 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes)); > - Status = UpdateSectionEntries (BaseAddress, Length, Attributes, VirtualMask); > - } else { > - // Base and/or length is not a multiple of 1 MB > - DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU page 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes)); > - Status = UpdatePageEntries (BaseAddress, Length, Attributes, VirtualMask); > + while (Length > 0) { Would this not end up returning an uninitialized Status if called with Length == 0? / Leif > + if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) && > + Length >= TT_DESCRIPTOR_SECTION_SIZE) { > + > + ChunkLength = Length - Length % TT_DESCRIPTOR_SECTION_SIZE; > + > + DEBUG ((DEBUG_PAGE, > + "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n", > + BaseAddress, ChunkLength, Attributes)); > + > + Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes, > + VirtualMask); > + } else { > + > + // > + // Process page by page until the next section boundary, but only if > + // we have more than a section's worth of area to deal with after that. > + // > + ChunkLength = TT_DESCRIPTOR_SECTION_SIZE - > + (BaseAddress % TT_DESCRIPTOR_SECTION_SIZE); > + if (ChunkLength + TT_DESCRIPTOR_SECTION_SIZE > Length) { > + ChunkLength = Length; > + } > + > + DEBUG ((DEBUG_PAGE, > + "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n", > + BaseAddress, ChunkLength, Attributes)); > + > + Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes, > + VirtualMask); > + } > + > + if (EFI_ERROR (Status)) { > + break; > + } > + > + BaseAddress += ChunkLength; > + Length -= ChunkLength; > } > > // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks > -- > 2.7.4 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 6 March 2017 at 15:12, Leif Lindholm <leif.lindholm@linaro.org> wrote: > On Thu, Mar 02, 2017 at 10:36:13AM +0000, Ard Biesheuvel wrote: >> Currently, any range passed to CpuArchProtocol::SetMemoryAttributes is >> fully broken down into page mappings if the start or the size of the >> region happens to be misaliged relative to the section size of 1 MB. >> >> This is going to hurt when we enable strict memory permissions, given > > "Hurt" -> "cause unnecessary performance penalties" or "hurt" -> > "break everything"? > The former. It will map all of RAM using page mappings, which uses more space unnecessarily >> that we remap the entire RAM space non-executable (modulo the code >> bits) when the CpuArchProtocol is installed. >> >> So refactor the code to iterate over the range in a way that ensures >> that all naturally aligned section sized subregions are not broken up. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Many thanks for getting rid of the magic values, and in general making > the code more logical. One question below: > >> --- >> ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 47 ++++++++++++++++---- >> 1 file changed, 39 insertions(+), 8 deletions(-) >> >> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c >> index 89e429925ba9..ce4d529bda67 100644 >> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c >> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c >> @@ -679,6 +679,7 @@ SetMemoryAttributes ( >> ) >> { >> EFI_STATUS Status; >> + UINT64 ChunkLength; >> >> // >> // Ignore invocations that only modify permission bits >> @@ -687,14 +688,44 @@ SetMemoryAttributes ( >> return EFI_SUCCESS; >> } >> >> - if(((BaseAddress & 0xFFFFF) == 0) && ((Length & 0xFFFFF) == 0)) { >> - // Is the base and length a multiple of 1 MB? >> - DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU section 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes)); >> - Status = UpdateSectionEntries (BaseAddress, Length, Attributes, VirtualMask); >> - } else { >> - // Base and/or length is not a multiple of 1 MB >> - DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU page 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes)); >> - Status = UpdatePageEntries (BaseAddress, Length, Attributes, VirtualMask); >> + while (Length > 0) { > > Would this not end up returning an uninitialized Status if called with > Length == 0? > Yes, well spotted. I will just add an early 'return EFI_SUCCESS' for this case. >> + if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) && >> + Length >= TT_DESCRIPTOR_SECTION_SIZE) { >> + >> + ChunkLength = Length - Length % TT_DESCRIPTOR_SECTION_SIZE; >> + >> + DEBUG ((DEBUG_PAGE, >> + "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n", >> + BaseAddress, ChunkLength, Attributes)); >> + >> + Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes, >> + VirtualMask); >> + } else { >> + >> + // >> + // Process page by page until the next section boundary, but only if >> + // we have more than a section's worth of area to deal with after that. >> + // >> + ChunkLength = TT_DESCRIPTOR_SECTION_SIZE - >> + (BaseAddress % TT_DESCRIPTOR_SECTION_SIZE); >> + if (ChunkLength + TT_DESCRIPTOR_SECTION_SIZE > Length) { >> + ChunkLength = Length; >> + } >> + >> + DEBUG ((DEBUG_PAGE, >> + "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n", >> + BaseAddress, ChunkLength, Attributes)); >> + >> + Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes, >> + VirtualMask); >> + } >> + >> + if (EFI_ERROR (Status)) { >> + break; >> + } >> + >> + BaseAddress += ChunkLength; >> + Length -= ChunkLength; >> } >> >> // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks >> -- >> 2.7.4 >> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Mon, Mar 06, 2017 at 03:55:42PM +0100, Ard Biesheuvel wrote: > On 6 March 2017 at 15:12, Leif Lindholm <leif.lindholm@linaro.org> wrote: > > On Thu, Mar 02, 2017 at 10:36:13AM +0000, Ard Biesheuvel wrote: > >> Currently, any range passed to CpuArchProtocol::SetMemoryAttributes is > >> fully broken down into page mappings if the start or the size of the > >> region happens to be misaliged relative to the section size of 1 MB. > >> > >> This is going to hurt when we enable strict memory permissions, given > > > > "Hurt" -> "cause unnecessary performance penalties" or "hurt" -> > > "break everything"? > > > > The former. It will map all of RAM using page mappings, which uses > more space unnecessarily OK, can you expand the statement to say "hurt performance" then? > >> that we remap the entire RAM space non-executable (modulo the code > >> bits) when the CpuArchProtocol is installed. > >> > >> So refactor the code to iterate over the range in a way that ensures > >> that all naturally aligned section sized subregions are not broken up. > >> > >> Contributed-under: TianoCore Contribution Agreement 1.0 > >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > > Many thanks for getting rid of the magic values, and in general making > > the code more logical. One question below: > > > >> --- > >> ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 47 ++++++++++++++++---- > >> 1 file changed, 39 insertions(+), 8 deletions(-) > >> > >> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c > >> index 89e429925ba9..ce4d529bda67 100644 > >> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c > >> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c > >> @@ -679,6 +679,7 @@ SetMemoryAttributes ( > >> ) > >> { > >> EFI_STATUS Status; > >> + UINT64 ChunkLength; > >> > >> // > >> // Ignore invocations that only modify permission bits > >> @@ -687,14 +688,44 @@ SetMemoryAttributes ( > >> return EFI_SUCCESS; > >> } > >> > >> - if(((BaseAddress & 0xFFFFF) == 0) && ((Length & 0xFFFFF) == 0)) { > >> - // Is the base and length a multiple of 1 MB? > >> - DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU section 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes)); > >> - Status = UpdateSectionEntries (BaseAddress, Length, Attributes, VirtualMask); > >> - } else { > >> - // Base and/or length is not a multiple of 1 MB > >> - DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU page 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes)); > >> - Status = UpdatePageEntries (BaseAddress, Length, Attributes, VirtualMask); > >> + while (Length > 0) { > > > > Would this not end up returning an uninitialized Status if called with > > Length == 0? > > > > Yes, well spotted. I will just add an early 'return EFI_SUCCESS' for this case. Works for me. If you also tweak the commit message: Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org> > >> + if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) && > >> + Length >= TT_DESCRIPTOR_SECTION_SIZE) { > >> + > >> + ChunkLength = Length - Length % TT_DESCRIPTOR_SECTION_SIZE; > >> + > >> + DEBUG ((DEBUG_PAGE, > >> + "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n", > >> + BaseAddress, ChunkLength, Attributes)); > >> + > >> + Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes, > >> + VirtualMask); > >> + } else { > >> + > >> + // > >> + // Process page by page until the next section boundary, but only if > >> + // we have more than a section's worth of area to deal with after that. > >> + // > >> + ChunkLength = TT_DESCRIPTOR_SECTION_SIZE - > >> + (BaseAddress % TT_DESCRIPTOR_SECTION_SIZE); > >> + if (ChunkLength + TT_DESCRIPTOR_SECTION_SIZE > Length) { > >> + ChunkLength = Length; > >> + } > >> + > >> + DEBUG ((DEBUG_PAGE, > >> + "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n", > >> + BaseAddress, ChunkLength, Attributes)); > >> + > >> + Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes, > >> + VirtualMask); > >> + } > >> + > >> + if (EFI_ERROR (Status)) { > >> + break; > >> + } > >> + > >> + BaseAddress += ChunkLength; > >> + Length -= ChunkLength; > >> } > >> > >> // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks > >> -- > >> 2.7.4 > >> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c index 89e429925ba9..ce4d529bda67 100644 --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c @@ -679,6 +679,7 @@ SetMemoryAttributes ( ) { EFI_STATUS Status; + UINT64 ChunkLength; // // Ignore invocations that only modify permission bits @@ -687,14 +688,44 @@ SetMemoryAttributes ( return EFI_SUCCESS; } - if(((BaseAddress & 0xFFFFF) == 0) && ((Length & 0xFFFFF) == 0)) { - // Is the base and length a multiple of 1 MB? - DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU section 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes)); - Status = UpdateSectionEntries (BaseAddress, Length, Attributes, VirtualMask); - } else { - // Base and/or length is not a multiple of 1 MB - DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU page 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes)); - Status = UpdatePageEntries (BaseAddress, Length, Attributes, VirtualMask); + while (Length > 0) { + if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) && + Length >= TT_DESCRIPTOR_SECTION_SIZE) { + + ChunkLength = Length - Length % TT_DESCRIPTOR_SECTION_SIZE; + + DEBUG ((DEBUG_PAGE, + "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n", + BaseAddress, ChunkLength, Attributes)); + + Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes, + VirtualMask); + } else { + + // + // Process page by page until the next section boundary, but only if + // we have more than a section's worth of area to deal with after that. + // + ChunkLength = TT_DESCRIPTOR_SECTION_SIZE - + (BaseAddress % TT_DESCRIPTOR_SECTION_SIZE); + if (ChunkLength + TT_DESCRIPTOR_SECTION_SIZE > Length) { + ChunkLength = Length; + } + + DEBUG ((DEBUG_PAGE, + "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n", + BaseAddress, ChunkLength, Attributes)); + + Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes, + VirtualMask); + } + + if (EFI_ERROR (Status)) { + break; + } + + BaseAddress += ChunkLength; + Length -= ChunkLength; } // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks
Currently, any range passed to CpuArchProtocol::SetMemoryAttributes is fully broken down into page mappings if the start or the size of the region happens to be misaliged relative to the section size of 1 MB. This is going to hurt when we enable strict memory permissions, given that we remap the entire RAM space non-executable (modulo the code bits) when the CpuArchProtocol is installed. So refactor the code to iterate over the range in a way that ensures that all naturally aligned section sized subregions are not broken up. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 47 ++++++++++++++++---- 1 file changed, 39 insertions(+), 8 deletions(-) -- 2.7.4 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel