| Message ID | 20221105225234.3089177-1-ardb@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: efi: Make runtime region misalignment warning less noisy | expand |
On 11/5/22 23:52, Ard Biesheuvel wrote: > The EFI spec requires that on arm64 systems, all runtime code and data > regions that share a 64k page can be mapped with the same memory type > attributes. Unfortunately, this does not take permission attributes into > account, and so the firmware is permitted to expose runtime code and > data regions that share 64k pages, and this may prevent the OS from > using restricted permissions in such cases, e.g., map data regions with > non-exec attributes. This is the relevant paragraph in the UEFI specification: <cite> The ARM architecture allows mapping pages at a variety of granularities, including 4KiB and 64KiB. If a 64KiB physical page contains any 4KiB page with any of the following types listed below, then all 4KiB pages in the 64KiB page must use identical ARM Memory Page Attributes (as described in Map EFI Cacheability Attributes to AArch64 Memory Types): - EfiRuntimeServicesCode - EfiRuntimeServicesData - EfiReserved - EfiACPIMemoryNVS Mixed attribute mappings within a larger page are not allowed. </cite> It remains unclear if only EFI Cacheability of also other page attributes are meant. The UEFI specification should be clarified in this respect. > > We currently emit a warning when hitting this at boot, but the warning > is problematic for a number of reasons: > - it uses WARN() which spews a lot of irrelevant information into the > log about the execution context where the issue was detected; > - it only takes the start of the region into account and not the size Is the occurrence of the warning specific to U-Boot or do you see the warning with EDK II too? > > Let's just drop the warning, as the condition does not strictly violate > the spec (although it only occurs with U-Boot), and fix the check to > take both the start and the end addresses into account. > > Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org> > Reported-by: Linus Torvalds <torvalds@linux-foundation.org> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > arch/arm64/kernel/efi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c > index e1be6c429810d0d5..3dd6f0c66f8aeb78 100644 > --- a/arch/arm64/kernel/efi.c > +++ b/arch/arm64/kernel/efi.c > @@ -25,8 +25,8 @@ static __init pteval_t create_mapping_protection(efi_memory_desc_t *md) > if (type == EFI_MEMORY_MAPPED_IO) > return PROT_DEVICE_nGnRE; > > - if (WARN_ONCE(!PAGE_ALIGNED(md->phys_addr), > - "UEFI Runtime regions are not aligned to 64 KB -- buggy firmware?")) > + if (!PAGE_ALIGNED(md->phys_addr) || > + !PAGE_ALIGNED(md->num_pages * EFI_PAGE_SIZE)) Enhancing the check is correct. The warning tells that Linux cannot establish secure settings for some pages. It would be preferable to keep it and fix the UEFI specification and the firmware instead. Best regards Heinrich > /* > * If the region is not aligned to the page size of the OS, we > * can not use strict permissions, since that would also affect
On 11/6/22 00:24, Heinrich Schuchardt wrote: > On 11/5/22 23:52, Ard Biesheuvel wrote: >> The EFI spec requires that on arm64 systems, all runtime code and data >> regions that share a 64k page can be mapped with the same memory type >> attributes. Unfortunately, this does not take permission attributes into >> account, and so the firmware is permitted to expose runtime code and >> data regions that share 64k pages, and this may prevent the OS from >> using restricted permissions in such cases, e.g., map data regions with >> non-exec attributes. > > This is the relevant paragraph in the UEFI specification: > > <cite> > The ARM architecture allows mapping pages at a variety of granularities, > including 4KiB and 64KiB. If a 64KiB physical page contains any 4KiB > page with any of the following types listed below, then all 4KiB pages > in the 64KiB page must use identical ARM Memory Page Attributes (as > described in Map EFI Cacheability Attributes to AArch64 Memory Types): > > - EfiRuntimeServicesCode > - EfiRuntimeServicesData > - EfiReserved > - EfiACPIMemoryNVS > > Mixed attribute mappings within a larger page are not allowed. > </cite> > > It remains unclear if only EFI Cacheability of also other page > attributes are meant. The UEFI specification should be clarified in this > respect. > >> >> We currently emit a warning when hitting this at boot, but the warning >> is problematic for a number of reasons: >> - it uses WARN() which spews a lot of irrelevant information into the >> log about the execution context where the issue was detected; >> - it only takes the start of the region into account and not the size > > Is the occurrence of the warning specific to U-Boot or do you see the > warning with EDK II too? > >> >> Let's just drop the warning, as the condition does not strictly violate >> the spec (although it only occurs with U-Boot), and fix the check to >> take both the start and the end addresses into account. >> >> Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org> >> Reported-by: Linus Torvalds <torvalds@linux-foundation.org> >> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> >> --- >> arch/arm64/kernel/efi.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c >> index e1be6c429810d0d5..3dd6f0c66f8aeb78 100644 >> --- a/arch/arm64/kernel/efi.c >> +++ b/arch/arm64/kernel/efi.c >> @@ -25,8 +25,8 @@ static __init pteval_t >> create_mapping_protection(efi_memory_desc_t *md) >> if (type == EFI_MEMORY_MAPPED_IO) >> return PROT_DEVICE_nGnRE; >> - if (WARN_ONCE(!PAGE_ALIGNED(md->phys_addr), >> - "UEFI Runtime regions are not aligned to 64 KB -- buggy >> firmware?")) >> + if (!PAGE_ALIGNED(md->phys_addr) || >> + !PAGE_ALIGNED(md->num_pages * EFI_PAGE_SIZE)) > > Enhancing the check is correct. The UEFI requirement is that within a 64 KiB page all memory descriptors shall use the same page attributes if any 4 KiB sub-page is of one of the following types. - EfiRuntimeServicesCode - EfiRuntimeServicesData - EfiReserved - EfiACPIMemoryNVS It is not required that memory descriptors shall be aligned to 64 KiB boundaries. So the following map should not pose any problem: 00000-00fff - EfiBootServicesData (not used at runtime) 01000-13fff - EfiRuntimeServicesData 14000-1ffff - EfiRuntimeServicesData 20000-24fff - EfiRuntimeServicesCode 25000-27fff - EfiBootServicesCode (not used at runtime) 28000-3ffff - EfiRuntimeServicesCode Evaluating each memory descriptor individually looks wrong. You first have to extend each memory descriptor of one of the four aforementioned memory types to the next 64 KiB boundary or within a 64 KiB boundary to the next descriptor of one of the aforementioned memory types. Next you have to merge adjacent descriptors with same attributes within the same 64 KiB page. So the map for which you set attributes would become 00000-1ffff - EfiRuntimeServicesData 20000-3ffff - EfiRuntimeServicesCode I guess all that alignment and merging should go into efi_virtmap_init(). Best regards Heinrich > > The warning tells that Linux cannot establish secure settings for some > pages. It would be preferable to keep it and fix the UEFI specification > and the firmware instead. > > Best regards > > Heinrich > >> /* >> * If the region is not aligned to the page size of the OS, we >> * can not use strict permissions, since that would also affect
On Sun, 6 Nov 2022 at 03:27, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > > > On 11/6/22 00:24, Heinrich Schuchardt wrote: > > On 11/5/22 23:52, Ard Biesheuvel wrote: > >> The EFI spec requires that on arm64 systems, all runtime code and data > >> regions that share a 64k page can be mapped with the same memory type > >> attributes. Unfortunately, this does not take permission attributes into > >> account, and so the firmware is permitted to expose runtime code and > >> data regions that share 64k pages, and this may prevent the OS from > >> using restricted permissions in such cases, e.g., map data regions with > >> non-exec attributes. > > > > This is the relevant paragraph in the UEFI specification: > > > > <cite> > > The ARM architecture allows mapping pages at a variety of granularities, > > including 4KiB and 64KiB. If a 64KiB physical page contains any 4KiB > > page with any of the following types listed below, then all 4KiB pages > > in the 64KiB page must use identical ARM Memory Page Attributes (as > > described in Map EFI Cacheability Attributes to AArch64 Memory Types): > > > > - EfiRuntimeServicesCode > > - EfiRuntimeServicesData > > - EfiReserved > > - EfiACPIMemoryNVS > > > > Mixed attribute mappings within a larger page are not allowed. > > </cite> > > > > It remains unclear if only EFI Cacheability of also other page > > attributes are meant. The UEFI specification should be clarified in this > > respect. > > > >> > >> We currently emit a warning when hitting this at boot, but the warning > >> is problematic for a number of reasons: > >> - it uses WARN() which spews a lot of irrelevant information into the > >> log about the execution context where the issue was detected; > >> - it only takes the start of the region into account and not the size > > > > Is the occurrence of the warning specific to U-Boot or do you see the > > warning with EDK II too? > > > >> > >> Let's just drop the warning, as the condition does not strictly violate > >> the spec (although it only occurs with U-Boot), and fix the check to > >> take both the start and the end addresses into account. > >> > >> Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > >> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org> > >> Reported-by: Linus Torvalds <torvalds@linux-foundation.org> > >> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > >> --- > >> arch/arm64/kernel/efi.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c > >> index e1be6c429810d0d5..3dd6f0c66f8aeb78 100644 > >> --- a/arch/arm64/kernel/efi.c > >> +++ b/arch/arm64/kernel/efi.c > >> @@ -25,8 +25,8 @@ static __init pteval_t > >> create_mapping_protection(efi_memory_desc_t *md) > >> if (type == EFI_MEMORY_MAPPED_IO) > >> return PROT_DEVICE_nGnRE; > >> - if (WARN_ONCE(!PAGE_ALIGNED(md->phys_addr), > >> - "UEFI Runtime regions are not aligned to 64 KB -- buggy > >> firmware?")) > >> + if (!PAGE_ALIGNED(md->phys_addr) || > >> + !PAGE_ALIGNED(md->num_pages * EFI_PAGE_SIZE)) > > > > Enhancing the check is correct. > > The UEFI requirement is that within a 64 KiB page all memory descriptors > shall use the same page attributes if any 4 KiB sub-page is of one of > the following types. > > - EfiRuntimeServicesCode > - EfiRuntimeServicesData > - EfiReserved > - EfiACPIMemoryNVS > > It is not required that memory descriptors shall be aligned to 64 KiB > boundaries. > Indeed, this is what I misremembered. > So the following map should not pose any problem: > > 00000-00fff - EfiBootServicesData (not used at runtime) > 01000-13fff - EfiRuntimeServicesData > 14000-1ffff - EfiRuntimeServicesData > 20000-24fff - EfiRuntimeServicesCode > 25000-27fff - EfiBootServicesCode (not used at runtime) > 28000-3ffff - EfiRuntimeServicesCode > > Evaluating each memory descriptor individually looks wrong. You first > have to extend each memory descriptor of one of the four aforementioned > memory types to the next 64 KiB boundary or within a 64 KiB boundary to > the next descriptor of one of the aforementioned memory types. Next you > have to merge adjacent descriptors with same attributes within the same > 64 KiB page. > So now we have to look at adjacent descriptors, which means we have to sort the memory map, as there is no guarantee that the descriptors appear in order. > So the map for which you set attributes would become > > 00000-1ffff - EfiRuntimeServicesData > 20000-3ffff - EfiRuntimeServicesCode > > I guess all that alignment and merging should go into efi_virtmap_init(). > U-boot does not provide a memory attributes table either, so we don't know which parts of the code regions should be mapped R-X and which parts RW- (Firmware implementations such as EDK2 that are based on PE/COFF images internally use code descriptors for each executable, which means they cover both the .text/.rodata and .data/.bss sections of the image. The data descriptors are used for dynamic allocations). This is why we use RWX for RTcode and RW- for RTdata in absence of the RO/XP attributes (which are passed via the memory attributes table usually). So in summary, I think the patch is fine. The warning is spurious given that the condition in question is actually permitted by the spec. On the uboot side, which already seems to align and round up RTcode sections to 64k, we might set the EFI_MEMORY_RO attribute on such regions if they really only contain .text and .rodata segments, and can tolerate being mapped without writable permissions. That way, the kernel will understand that it does not need to provide RWX permissions, which is really what all this code is trying to prevent. Thanks, Ard.
On Sun, 6 Nov 2022 at 10:48, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Sun, 6 Nov 2022 at 03:27, Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> wrote: > > > > > > > > On 11/6/22 00:24, Heinrich Schuchardt wrote: > > > On 11/5/22 23:52, Ard Biesheuvel wrote: > > >> The EFI spec requires that on arm64 systems, all runtime code and data > > >> regions that share a 64k page can be mapped with the same memory type > > >> attributes. Unfortunately, this does not take permission attributes into > > >> account, and so the firmware is permitted to expose runtime code and > > >> data regions that share 64k pages, and this may prevent the OS from > > >> using restricted permissions in such cases, e.g., map data regions with > > >> non-exec attributes. > > > > > > This is the relevant paragraph in the UEFI specification: > > > > > > <cite> > > > The ARM architecture allows mapping pages at a variety of granularities, > > > including 4KiB and 64KiB. If a 64KiB physical page contains any 4KiB > > > page with any of the following types listed below, then all 4KiB pages > > > in the 64KiB page must use identical ARM Memory Page Attributes (as > > > described in Map EFI Cacheability Attributes to AArch64 Memory Types): > > > > > > - EfiRuntimeServicesCode > > > - EfiRuntimeServicesData > > > - EfiReserved > > > - EfiACPIMemoryNVS > > > > > > Mixed attribute mappings within a larger page are not allowed. > > > </cite> > > > > > > It remains unclear if only EFI Cacheability of also other page > > > attributes are meant. The UEFI specification should be clarified in this > > > respect. > > > > > >> > > >> We currently emit a warning when hitting this at boot, but the warning > > >> is problematic for a number of reasons: > > >> - it uses WARN() which spews a lot of irrelevant information into the > > >> log about the execution context where the issue was detected; > > >> - it only takes the start of the region into account and not the size > > > > > > Is the occurrence of the warning specific to U-Boot or do you see the > > > warning with EDK II too? > > > > > >> > > >> Let's just drop the warning, as the condition does not strictly violate > > >> the spec (although it only occurs with U-Boot), and fix the check to > > >> take both the start and the end addresses into account. > > >> > > >> Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > > >> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > >> Reported-by: Linus Torvalds <torvalds@linux-foundation.org> > > >> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > >> --- > > >> arch/arm64/kernel/efi.c | 4 ++-- > > >> 1 file changed, 2 insertions(+), 2 deletions(-) > > >> > > >> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c > > >> index e1be6c429810d0d5..3dd6f0c66f8aeb78 100644 > > >> --- a/arch/arm64/kernel/efi.c > > >> +++ b/arch/arm64/kernel/efi.c > > >> @@ -25,8 +25,8 @@ static __init pteval_t > > >> create_mapping_protection(efi_memory_desc_t *md) > > >> if (type == EFI_MEMORY_MAPPED_IO) > > >> return PROT_DEVICE_nGnRE; > > >> - if (WARN_ONCE(!PAGE_ALIGNED(md->phys_addr), > > >> - "UEFI Runtime regions are not aligned to 64 KB -- buggy > > >> firmware?")) > > >> + if (!PAGE_ALIGNED(md->phys_addr) || > > >> + !PAGE_ALIGNED(md->num_pages * EFI_PAGE_SIZE)) > > > > > > Enhancing the check is correct. > > > > The UEFI requirement is that within a 64 KiB page all memory descriptors > > shall use the same page attributes if any 4 KiB sub-page is of one of > > the following types. > > > > - EfiRuntimeServicesCode > > - EfiRuntimeServicesData > > - EfiReserved > > - EfiACPIMemoryNVS > > > > It is not required that memory descriptors shall be aligned to 64 KiB > > boundaries. > > > > Indeed, this is what I misremembered. > > > So the following map should not pose any problem: > > > > 00000-00fff - EfiBootServicesData (not used at runtime) > > 01000-13fff - EfiRuntimeServicesData > > 14000-1ffff - EfiRuntimeServicesData > > 20000-24fff - EfiRuntimeServicesCode > > 25000-27fff - EfiBootServicesCode (not used at runtime) > > 28000-3ffff - EfiRuntimeServicesCode > > > > Evaluating each memory descriptor individually looks wrong. You first > > have to extend each memory descriptor of one of the four aforementioned > > memory types to the next 64 KiB boundary or within a 64 KiB boundary to > > the next descriptor of one of the aforementioned memory types. Next you > > have to merge adjacent descriptors with same attributes within the same > > 64 KiB page. > > > > So now we have to look at adjacent descriptors, which means we have to > sort the memory map, as there is no guarantee that the descriptors > appear in order. > > > So the map for which you set attributes would become > > > > 00000-1ffff - EfiRuntimeServicesData > > 20000-3ffff - EfiRuntimeServicesCode > > > > I guess all that alignment and merging should go into efi_virtmap_init(). > > > > U-boot does not provide a memory attributes table either, so we don't > know which parts of the code regions should be mapped R-X and which > parts RW- (Firmware implementations such as EDK2 that are based on > PE/COFF images internally use code descriptors for each executable, > which means they cover both the .text/.rodata and .data/.bss sections > of the image. The data descriptors are used for dynamic allocations). > > This is why we use RWX for RTcode and RW- for RTdata in absence of the > RO/XP attributes (which are passed via the memory attributes table > usually). > > So in summary, I think the patch is fine. The warning is spurious > given that the condition in question is actually permitted by the > spec. > > On the uboot side, which already seems to align and round up RTcode > sections to 64k, we might set the EFI_MEMORY_RO attribute on such > regions if they really only contain .text and .rodata segments, and > can tolerate being mapped without writable permissions. That way, the > kernel will understand that it does not need to provide RWX > permissions, which is really what all this code is trying to prevent. > OK, this is not entirely true. Setting EFI_MEMORY_RO on sufficiently aligned RTcode descriptors that don't require read-write permissions would definitely be an improvement, but the current code would still use RWX for RTdata sections that are not aligned to 64k (on 16k or 64k pagesize OS builds) because otherwise, we'd have to go over the EFI memory map again to check whether setting RW- on the RTdata region in question would not result in an adjacent RTcode region losing its executable permissions. So what we might do is detect this condition, and if it triggers, go over the memory map again and map all misaligned RTcode descriptors again. I'll go and code this up and send a v2.
On 11/6/22 10:48, Ard Biesheuvel wrote: > On Sun, 6 Nov 2022 at 03:27, Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> wrote: >> >> >> >> On 11/6/22 00:24, Heinrich Schuchardt wrote: >>> On 11/5/22 23:52, Ard Biesheuvel wrote: >>>> The EFI spec requires that on arm64 systems, all runtime code and data >>>> regions that share a 64k page can be mapped with the same memory type >>>> attributes. Unfortunately, this does not take permission attributes into >>>> account, and so the firmware is permitted to expose runtime code and >>>> data regions that share 64k pages, and this may prevent the OS from >>>> using restricted permissions in such cases, e.g., map data regions with >>>> non-exec attributes. >>> >>> This is the relevant paragraph in the UEFI specification: >>> >>> <cite> >>> The ARM architecture allows mapping pages at a variety of granularities, >>> including 4KiB and 64KiB. If a 64KiB physical page contains any 4KiB >>> page with any of the following types listed below, then all 4KiB pages >>> in the 64KiB page must use identical ARM Memory Page Attributes (as >>> described in Map EFI Cacheability Attributes to AArch64 Memory Types): >>> >>> - EfiRuntimeServicesCode >>> - EfiRuntimeServicesData >>> - EfiReserved >>> - EfiACPIMemoryNVS >>> >>> Mixed attribute mappings within a larger page are not allowed. >>> </cite> >>> >>> It remains unclear if only EFI Cacheability of also other page >>> attributes are meant. The UEFI specification should be clarified in this >>> respect. >>> >>>> >>>> We currently emit a warning when hitting this at boot, but the warning >>>> is problematic for a number of reasons: >>>> - it uses WARN() which spews a lot of irrelevant information into the >>>> log about the execution context where the issue was detected; >>>> - it only takes the start of the region into account and not the size >>> >>> Is the occurrence of the warning specific to U-Boot or do you see the >>> warning with EDK II too? >>> >>>> >>>> Let's just drop the warning, as the condition does not strictly violate >>>> the spec (although it only occurs with U-Boot), and fix the check to >>>> take both the start and the end addresses into account. >>>> >>>> Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >>>> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org> >>>> Reported-by: Linus Torvalds <torvalds@linux-foundation.org> >>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> >>>> --- >>>> arch/arm64/kernel/efi.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c >>>> index e1be6c429810d0d5..3dd6f0c66f8aeb78 100644 >>>> --- a/arch/arm64/kernel/efi.c >>>> +++ b/arch/arm64/kernel/efi.c >>>> @@ -25,8 +25,8 @@ static __init pteval_t >>>> create_mapping_protection(efi_memory_desc_t *md) >>>> if (type == EFI_MEMORY_MAPPED_IO) >>>> return PROT_DEVICE_nGnRE; >>>> - if (WARN_ONCE(!PAGE_ALIGNED(md->phys_addr), >>>> - "UEFI Runtime regions are not aligned to 64 KB -- buggy >>>> firmware?")) >>>> + if (!PAGE_ALIGNED(md->phys_addr) || >>>> + !PAGE_ALIGNED(md->num_pages * EFI_PAGE_SIZE)) >>> >>> Enhancing the check is correct. >> >> The UEFI requirement is that within a 64 KiB page all memory descriptors >> shall use the same page attributes if any 4 KiB sub-page is of one of >> the following types. >> >> - EfiRuntimeServicesCode >> - EfiRuntimeServicesData >> - EfiReserved >> - EfiACPIMemoryNVS >> >> It is not required that memory descriptors shall be aligned to 64 KiB >> boundaries. >> > > Indeed, this is what I misremembered. > >> So the following map should not pose any problem: >> >> 00000-00fff - EfiBootServicesData (not used at runtime) >> 01000-13fff - EfiRuntimeServicesData >> 14000-1ffff - EfiRuntimeServicesData >> 20000-24fff - EfiRuntimeServicesCode >> 25000-27fff - EfiBootServicesCode (not used at runtime) >> 28000-3ffff - EfiRuntimeServicesCode >> >> Evaluating each memory descriptor individually looks wrong. You first >> have to extend each memory descriptor of one of the four aforementioned >> memory types to the next 64 KiB boundary or within a 64 KiB boundary to >> the next descriptor of one of the aforementioned memory types. Next you >> have to merge adjacent descriptors with same attributes within the same >> 64 KiB page. >> > > So now we have to look at adjacent descriptors, which means we have to > sort the memory map, as there is no guarantee that the descriptors > appear in order. > >> So the map for which you set attributes would become >> >> 00000-1ffff - EfiRuntimeServicesData >> 20000-3ffff - EfiRuntimeServicesCode >> >> I guess all that alignment and merging should go into efi_virtmap_init(). >> > > U-boot does not provide a memory attributes table either, so we don't > know which parts of the code regions should be mapped R-X and which > parts RW- (Firmware implementations such as EDK2 that are based on > PE/COFF images internally use code descriptors for each executable, > which means they cover both the .text/.rodata and .data/.bss sections > of the image. The data descriptors are used for dynamic allocations). > > This is why we use RWX for RTcode and RW- for RTdata in absence of the > RO/XP attributes (which are passed via the memory attributes table > usually). > > So in summary, I think the patch is fine. The warning is spurious > given that the condition in question is actually permitted by the > spec. > > On the uboot side, which already seems to align and round up RTcode > sections to 64k, we might set the EFI_MEMORY_RO attribute on such > regions if they really only contain .text and .rodata segments, and > can tolerate being mapped without writable permissions. That way, the > kernel will understand that it does not need to provide RWX > permissions, which is really what all this code is trying to prevent. Shouldn't EFI_MEMORY_RO only be set if the UEFI firmware actually sets up the MMU to make the corresponding memory read only? Best regards Heinrich
On Sun, 6 Nov 2022 at 11:44, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > > > On 11/6/22 10:48, Ard Biesheuvel wrote: > > On Sun, 6 Nov 2022 at 03:27, Heinrich Schuchardt > > <heinrich.schuchardt@canonical.com> wrote: > >> > >> > >> > >> On 11/6/22 00:24, Heinrich Schuchardt wrote: > >>> On 11/5/22 23:52, Ard Biesheuvel wrote: > >>>> The EFI spec requires that on arm64 systems, all runtime code and data > >>>> regions that share a 64k page can be mapped with the same memory type > >>>> attributes. Unfortunately, this does not take permission attributes into > >>>> account, and so the firmware is permitted to expose runtime code and > >>>> data regions that share 64k pages, and this may prevent the OS from > >>>> using restricted permissions in such cases, e.g., map data regions with > >>>> non-exec attributes. > >>> > >>> This is the relevant paragraph in the UEFI specification: > >>> > >>> <cite> > >>> The ARM architecture allows mapping pages at a variety of granularities, > >>> including 4KiB and 64KiB. If a 64KiB physical page contains any 4KiB > >>> page with any of the following types listed below, then all 4KiB pages > >>> in the 64KiB page must use identical ARM Memory Page Attributes (as > >>> described in Map EFI Cacheability Attributes to AArch64 Memory Types): > >>> > >>> - EfiRuntimeServicesCode > >>> - EfiRuntimeServicesData > >>> - EfiReserved > >>> - EfiACPIMemoryNVS > >>> > >>> Mixed attribute mappings within a larger page are not allowed. > >>> </cite> > >>> > >>> It remains unclear if only EFI Cacheability of also other page > >>> attributes are meant. The UEFI specification should be clarified in this > >>> respect. > >>> > >>>> > >>>> We currently emit a warning when hitting this at boot, but the warning > >>>> is problematic for a number of reasons: > >>>> - it uses WARN() which spews a lot of irrelevant information into the > >>>> log about the execution context where the issue was detected; > >>>> - it only takes the start of the region into account and not the size > >>> > >>> Is the occurrence of the warning specific to U-Boot or do you see the > >>> warning with EDK II too? > >>> > >>>> > >>>> Let's just drop the warning, as the condition does not strictly violate > >>>> the spec (although it only occurs with U-Boot), and fix the check to > >>>> take both the start and the end addresses into account. > >>>> > >>>> Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > >>>> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org> > >>>> Reported-by: Linus Torvalds <torvalds@linux-foundation.org> > >>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > >>>> --- > >>>> arch/arm64/kernel/efi.c | 4 ++-- > >>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c > >>>> index e1be6c429810d0d5..3dd6f0c66f8aeb78 100644 > >>>> --- a/arch/arm64/kernel/efi.c > >>>> +++ b/arch/arm64/kernel/efi.c > >>>> @@ -25,8 +25,8 @@ static __init pteval_t > >>>> create_mapping_protection(efi_memory_desc_t *md) > >>>> if (type == EFI_MEMORY_MAPPED_IO) > >>>> return PROT_DEVICE_nGnRE; > >>>> - if (WARN_ONCE(!PAGE_ALIGNED(md->phys_addr), > >>>> - "UEFI Runtime regions are not aligned to 64 KB -- buggy > >>>> firmware?")) > >>>> + if (!PAGE_ALIGNED(md->phys_addr) || > >>>> + !PAGE_ALIGNED(md->num_pages * EFI_PAGE_SIZE)) > >>> > >>> Enhancing the check is correct. > >> > >> The UEFI requirement is that within a 64 KiB page all memory descriptors > >> shall use the same page attributes if any 4 KiB sub-page is of one of > >> the following types. > >> > >> - EfiRuntimeServicesCode > >> - EfiRuntimeServicesData > >> - EfiReserved > >> - EfiACPIMemoryNVS > >> > >> It is not required that memory descriptors shall be aligned to 64 KiB > >> boundaries. > >> > > > > Indeed, this is what I misremembered. > > > >> So the following map should not pose any problem: > >> > >> 00000-00fff - EfiBootServicesData (not used at runtime) > >> 01000-13fff - EfiRuntimeServicesData > >> 14000-1ffff - EfiRuntimeServicesData > >> 20000-24fff - EfiRuntimeServicesCode > >> 25000-27fff - EfiBootServicesCode (not used at runtime) > >> 28000-3ffff - EfiRuntimeServicesCode > >> > >> Evaluating each memory descriptor individually looks wrong. You first > >> have to extend each memory descriptor of one of the four aforementioned > >> memory types to the next 64 KiB boundary or within a 64 KiB boundary to > >> the next descriptor of one of the aforementioned memory types. Next you > >> have to merge adjacent descriptors with same attributes within the same > >> 64 KiB page. > >> > > > > So now we have to look at adjacent descriptors, which means we have to > > sort the memory map, as there is no guarantee that the descriptors > > appear in order. > > > >> So the map for which you set attributes would become > >> > >> 00000-1ffff - EfiRuntimeServicesData > >> 20000-3ffff - EfiRuntimeServicesCode > >> > >> I guess all that alignment and merging should go into efi_virtmap_init(). > >> > > > > U-boot does not provide a memory attributes table either, so we don't > > know which parts of the code regions should be mapped R-X and which > > parts RW- (Firmware implementations such as EDK2 that are based on > > PE/COFF images internally use code descriptors for each executable, > > which means they cover both the .text/.rodata and .data/.bss sections > > of the image. The data descriptors are used for dynamic allocations). > > > > This is why we use RWX for RTcode and RW- for RTdata in absence of the > > RO/XP attributes (which are passed via the memory attributes table > > usually). > > > > So in summary, I think the patch is fine. The warning is spurious > > given that the condition in question is actually permitted by the > > spec. > > > > On the uboot side, which already seems to align and round up RTcode > > sections to 64k, we might set the EFI_MEMORY_RO attribute on such > > regions if they really only contain .text and .rodata segments, and > > can tolerate being mapped without writable permissions. That way, the > > kernel will understand that it does not need to provide RWX > > permissions, which is really what all this code is trying to prevent. > > Shouldn't EFI_MEMORY_RO only be set if the UEFI firmware actually sets > up the MMU to make the corresponding memory read only? > No. The EFI_MEMORY_RO and XP attributes describe the nature of the contents of the regions, i.e., if they support being mapped with read-only resp. non-executable permissions. The same applies to the memory type attributes, btw: on bare metal, the memory is usually described as WC|WT|WB and it is up to the OS to choose between memory types when it creates the mapping - how the firmware maps it is irrelevant. In general, the OS does not care or even tries to determine how the firmware has programmed the MMU and the page tables.
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c index e1be6c429810d0d5..3dd6f0c66f8aeb78 100644 --- a/arch/arm64/kernel/efi.c +++ b/arch/arm64/kernel/efi.c @@ -25,8 +25,8 @@ static __init pteval_t create_mapping_protection(efi_memory_desc_t *md) if (type == EFI_MEMORY_MAPPED_IO) return PROT_DEVICE_nGnRE; - if (WARN_ONCE(!PAGE_ALIGNED(md->phys_addr), - "UEFI Runtime regions are not aligned to 64 KB -- buggy firmware?")) + if (!PAGE_ALIGNED(md->phys_addr) || + !PAGE_ALIGNED(md->num_pages * EFI_PAGE_SIZE)) /* * If the region is not aligned to the page size of the OS, we * can not use strict permissions, since that would also affect
The EFI spec requires that on arm64 systems, all runtime code and data regions that share a 64k page can be mapped with the same memory type attributes. Unfortunately, this does not take permission attributes into account, and so the firmware is permitted to expose runtime code and data regions that share 64k pages, and this may prevent the OS from using restricted permissions in such cases, e.g., map data regions with non-exec attributes. We currently emit a warning when hitting this at boot, but the warning is problematic for a number of reasons: - it uses WARN() which spews a lot of irrelevant information into the log about the execution context where the issue was detected; - it only takes the start of the region into account and not the size Let's just drop the warning, as the condition does not strictly violate the spec (although it only occurs with U-Boot), and fix the check to take both the start and the end addresses into account. Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org> Reported-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- arch/arm64/kernel/efi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)