[edk2] BUG in properties table feature implementation

Discussion:

Ard Biesheuvel

2015-06-29 10:46:08 UTC

Hello all,

I am running into another problem with the implementation of the UEFI
2.5 Properties Table feature. It splits PE/COFF images into separate
but adjacent memory regions, only to be able to assign different
permissions to .text and .data sections. This is working fine at boot
time.

However, at runtime, after calling virtual address map, this breaks
down completely. Since the virtual mapping supplied to
SetVirtualAddressMap() does not have to guarantee adjacency between
code and data regions (of which the OS does not know whether they
belong together or not), reapplying the relocations corrupts the
memory image and breaks the runtime services.

For example, this region

0x00005eeb1000-0x00005eeb6fff [Runtime Code]
0x00005eeb7000-0x00005eec0fff [Runtime Data]

is mapped on AARCH64 as

EFI remap 0x000000005eeb1000 => 00000000440a1000
EFI remap 0x000000005eeb7000 => 00000000440b7000

which retains the relative alignment, but adds a 64 KB offset to the
second regions so that the regions can still be mapped with different
permissions when the OS is executing with a 64 KB page size.

As far as I can tell, this is in accordance with the spec, and was
working fine until I tried to enable the properties table feature.
With that enabled, the two above regions could actually describe one
single PE/COFF image in memory, and the 64 KB offset results in the
relocations to be applied incorrectly.

I looked at PeCoffLoaderRelocateImageForRuntime () but to me, it is
not very obvious how to solve this. Obviously, our PE/COFF
implementation is not complete since it assumes file offset == memory
offset for sections, but this does not hold anymore for UEFI 2.5

I would also like to point out again that this is another result of
the fact that this series was pushed through with any review or
testing outside of the Intel firmware team. For features of this
magnitude and complexity, more scrutiny and testing is obviously
required.

Kind regards,
Ard.

Laszlo Ersek

2015-06-29 11:33:01 UTC

Permalink

Post by Ard Biesheuvel
Hello all,
I am running into another problem with the implementation of the UEFI
2.5 Properties Table feature. It splits PE/COFF images into separate
but adjacent memory regions, only to be able to assign different
permissions to .text and .data sections. This is working fine at boot
time.
However, at runtime, after calling virtual address map, this breaks
down completely. Since the virtual mapping supplied to
SetVirtualAddressMap() does not have to guarantee adjacency between
code and data regions (of which the OS does not know whether they
belong together or not), reapplying the relocations corrupts the
memory image and breaks the runtime services.
For example, this region
0x00005eeb1000-0x00005eeb6fff [Runtime Code]
0x00005eeb7000-0x00005eec0fff [Runtime Data]
is mapped on AARCH64 as
EFI remap 0x000000005eeb1000 => 00000000440a1000
EFI remap 0x000000005eeb7000 => 00000000440b7000
which retains the relative alignment, but adds a 64 KB offset to the
second regions so that the regions can still be mapped with different
permissions when the OS is executing with a 64 KB page size.
As far as I can tell, this is in accordance with the spec, and was
working fine until I tried to enable the properties table feature.

Where / how? Did you enable it in the guest kernel (if that question
makes sense), or in the ArmVirtPkg build?

In the latter, I can only see the PropertiesTableEnable PCD, but (a) it
defaults to TRUE, (b) does it actually control the splitting?

Thanks
Laszlo

Post by Ard Biesheuvel
With that enabled, the two above regions could actually describe one
single PE/COFF image in memory, and the 64 KB offset results in the
relocations to be applied incorrectly.
I looked at PeCoffLoaderRelocateImageForRuntime () but to me, it is
not very obvious how to solve this. Obviously, our PE/COFF
implementation is not complete since it assumes file offset == memory
offset for sections, but this does not hold anymore for UEFI 2.5
I would also like to point out again that this is another result of
the fact that this series was pushed through with any review or
testing outside of the Intel firmware team. For features of this
magnitude and complexity, more scrutiny and testing is obviously
required.
Kind regards,
Ard.

Ard Biesheuvel

2015-06-29 11:37:48 UTC

Permalink

Post by Laszlo Ersek

Where / how? Did you enable it in the guest kernel (if that question
makes sense), or in the ArmVirtPkg build?

No, I enabled it in the ArmVirtPkg.

Post by Laszlo Ersek
In the latter, I can only see the PropertiesTableEnable PCD, but (a) it
defaults to TRUE, (b) does it actually control the splitting?

Yes, it does. But it only works if
a) you are using a linker script that aligns the data section at 4 KB.
Otherwise, you get the runtime error message that you yourself
inquired about a couple of weeks ago, about the section alignment
being != 4 KB
b) EndOfDxe is being signalled correctly

So after putting all the pieces in place, the Linux AARCH64 kernel
boots, remaps the EFI runtime regions, and then crashes when trying to
read the EFI rtc, since the RealTimeClockRuntimeDxe is corrupted due
to the fact that its code and data regions are no longer mapped
adjacently.
--
Ard.

Post by Laszlo Ersek

Yao, Jiewen

2015-06-29 12:34:52 UTC

Permalink

Hi Ard
Yes, your are right. We do observe similar odd behavior in old Win7/Win8/Win8.1, and we have to disable it.
Win8 and Win8.1 have OS patch to set adjacent virtual memory map, to resolve this issue.
We also tested Win10, and Suse 11 GM. They are good.

Would you please share the information on which OS you are using?

All in all, if OS cannot guarantee the virtual memory map is adjacent, we have to disable this feature.

Thank you
Yao Jiewen

-----Original Message-----
From: Ard Biesheuvel [mailto:***@linaro.org]
Sent: Monday, June 29, 2015 6:46 PM
To: edk2-***@lists.sourceforge.net; Yao, Jiewen; Gao, Liming; Laszlo Ersek; Justen, Jordan L; Kinney, Michael D; Zeng, Star
Subject: BUG in properties table feature implementation

Hello all,

I am running into another problem with the implementation of the UEFI
2.5 Properties Table feature. It splits PE/COFF images into separate but adjacent memory regions, only to be able to assign different permissions to .text and .data sections. This is working fine at boot time.

However, at runtime, after calling virtual address map, this breaks down completely. Since the virtual mapping supplied to
SetVirtualAddressMap() does not have to guarantee adjacency between code and data regions (of which the OS does not know whether they belong together or not), reapplying the relocations corrupts the memory image and breaks the runtime services.

For example, this region

0x00005eeb1000-0x00005eeb6fff [Runtime Code]
0x00005eeb7000-0x00005eec0fff [Runtime Data]

is mapped on AARCH64 as

EFI remap 0x000000005eeb1000 => 00000000440a1000
EFI remap 0x000000005eeb7000 => 00000000440b7000

which retains the relative alignment, but adds a 64 KB offset to the second regions so that the regions can still be mapped with different permissions when the OS is executing with a 64 KB page size.

As far as I can tell, this is in accordance with the spec, and was working fine until I tried to enable the properties table feature.
With that enabled, the two above regions could actually describe one single PE/COFF image in memory, and the 64 KB offset results in the relocations to be applied incorrectly.

I looked at PeCoffLoaderRelocateImageForRuntime () but to me, it is not very obvious how to solve this. Obviously, our PE/COFF implementation is not complete since it assumes file offset == memory offset for sections, but this does not hold anymore for UEFI 2.5

I would also like to point out again that this is another result of the fact that this series was pushed through with any review or testing outside of the Intel firmware team. For features of this magnitude and complexity, more scrutiny and testing is obviously required.

Kind regards,
Ard.

Ard Biesheuvel

2015-06-29 12:44:23 UTC

Permalink

Post by Yao, Jiewen
Hi Ard
Yes, your are right. We do observe similar odd behavior in old Win7/Win8/Win8.1, and we have to disable it.
Win8 and Win8.1 have OS patch to set adjacent virtual memory map, to resolve this issue.

I see this as a big problem with the feature, as it breaks backward
compatibility. IOW, you won't be able to boot Windows 7 on a UEFI 2.5
system unless you manually disable the feature (in the setup screen, I
suppose?)

Post by Yao, Jiewen
We also tested Win10, and Suse 11 GM. They are good.
Would you please share the information on which OS you are using?

I am using the upstream Linux kernel for AArch64. I am not using a
distribution.

Post by Yao, Jiewen
All in all, if OS cannot guarantee the virtual memory map is adjacent, we have to disable this feature.

This is backwards: the feature should be implemented such that it
cannot be trivially broken by a well-behaving OS. Note that nothing in
the SetVirtualAddressMap () implementation suggests that the regions
should be adjacent.

Perhaps it would have been better to introduce a new memory region
type EfiRuntimeServicesPecoffData, so the OS at least knows which
regions it should keep adjacent, (i.e., Code and PecoffData regions
should be kept together)

So does PE/COFF even allow the memory image to deviate from the file
image? If so, the correct way is to update the PE/COFF loader and the
relocation logic can deal with sections being laid out differently in
memory than they are in the file.
--
Ard.

Post by Yao, Jiewen
-----Original Message-----
Sent: Monday, June 29, 2015 6:46 PM
Subject: BUG in properties table feature implementation
Hello all,
I am running into another problem with the implementation of the UEFI
2.5 Properties Table feature. It splits PE/COFF images into separate but adjacent memory regions, only to be able to assign different permissions to .text and .data sections. This is working fine at boot time.
However, at runtime, after calling virtual address map, this breaks down completely. Since the virtual mapping supplied to
SetVirtualAddressMap() does not have to guarantee adjacency between code and data regions (of which the OS does not know whether they belong together or not), reapplying the relocations corrupts the memory image and breaks the runtime services.
For example, this region
0x00005eeb1000-0x00005eeb6fff [Runtime Code]
0x00005eeb7000-0x00005eec0fff [Runtime Data]
is mapped on AARCH64 as
EFI remap 0x000000005eeb1000 => 00000000440a1000
EFI remap 0x000000005eeb7000 => 00000000440b7000
which retains the relative alignment, but adds a 64 KB offset to the second regions so that the regions can still be mapped with different permissions when the OS is executing with a 64 KB page size.
As far as I can tell, this is in accordance with the spec, and was working fine until I tried to enable the properties table feature.
With that enabled, the two above regions could actually describe one single PE/COFF image in memory, and the 64 KB offset results in the relocations to be applied incorrectly.
I looked at PeCoffLoaderRelocateImageForRuntime () but to me, it is not very obvious how to solve this. Obviously, our PE/COFF implementation is not complete since it assumes file offset == memory offset for sections, but this does not hold anymore for UEFI 2.5
I would also like to point out again that this is another result of the fact that this series was pushed through with any review or testing outside of the Intel firmware team. For features of this magnitude and complexity, more scrutiny and testing is obviously required.
Kind regards,
Ard.

Yao, Jiewen

2015-06-29 12:54:56 UTC

Permalink

Thanks for the sharing. Yes. I agree with you that this breaks backward compatibility.
Right, I do not think we can boot Win7 or old Win8 with this feature enabled.

Both UEFI OS and Firmware need support this UEFI2.5 Properties Table feature. I believe it is known by UEFI forum, when it is added to UEFI spec, and it is published.

UEFI 2.5 properties table is optional feature. It can be enabled or disabled.
The benefit of this feature is that: a UEFI2.5 OS can do memory protection based on UEFI memory map.
But the old UEFI OS cannot get any benefit.

So a platform can made judgment based on its need, as well as the UEFI 2.5 OS availability.

That is why we do not enable this PE/COFF 4K alignment in BaseTool as standard configuration.
So in other word, it is disabled by tool by default. A platform may also decide to not publish PropertiesTable by below PCD:
## Publish PropertiesTable or not.
#
# If this PCD is TRUE, DxeCore publishs PropertiesTable.
# DxeCore evaluates if all runtime drivers has 4K aligned PE sections. If all
# PE sections in runtime drivers are 4K aligned, DxeCore sets BIT0 in
# PropertiesTable. Or DxeCore clears BIT0 in PropertiesTable.
# If this PCD is FALSE, DxeCore does not publish PropertiesTable.
#
# If PropertiesTable has BIT0 set, DxeCore uses below policy in UEFI memory map:
# 1) Use EfiRuntimeServicesCode for runtime driver PE image code section and
# use EfiRuntimeServicesData for runtime driver PE image header and other section.
# 2) Set EfiRuntimeServicesCode to be EFI_MEMORY_RO.
# 3) Set EfiRuntimeServicesData to be EFI_MEMORY_XP.
# 4) Set EfiMemoryMappedIO and EfiMemoryMappedIOPortSpace to be EFI_MEMORY_XP.
#
# NOTE: Platform need gurantee this PCD is set correctly. Platform should set
# this PCD to be TURE if and only if all runtime driver has seperated Code/Data
# section. If PE code/data sections are merged, the result is unpredictable.
#
# @Prompt Publish UEFI PropertiesTable.
gEfiMdeModulePkgTokenSpaceGuid.PropertiesTableEnable|TRUE|BOOLEAN|0x0000006e

Thank you
Yao Jiewen

-----Original Message-----
From: Ard Biesheuvel [mailto:***@linaro.org]
Sent: Monday, June 29, 2015 8:44 PM
To: Yao, Jiewen
Cc: edk2-***@lists.sourceforge.net; Gao, Liming; Laszlo Ersek; Justen, Jordan L; Kinney, Michael D; Zeng, Star
Subject: Re: BUG in properties table feature implementation

I see this as a big problem with the feature, as it breaks backward compatibility. IOW, you won't be able to boot Windows 7 on a UEFI 2.5 system unless you manually disable the feature (in the setup screen, I
suppose?)

Post by Yao, Jiewen
We also tested Win10, and Suse 11 GM. They are good.
Would you please share the information on which OS you are using?

I am using the upstream Linux kernel for AArch64. I am not using a distribution.

Post by Yao, Jiewen
All in all, if OS cannot guarantee the virtual memory map is adjacent, we have to disable this feature.

This is backwards: the feature should be implemented such that it cannot be trivially broken by a well-behaving OS. Note that nothing in the SetVirtualAddressMap () implementation suggests that the regions should be adjacent.

Perhaps it would have been better to introduce a new memory region type EfiRuntimeServicesPecoffData, so the OS at least knows which regions it should keep adjacent, (i.e., Code and PecoffData regions should be kept together)

So does PE/COFF even allow the memory image to deviate from the file image? If so, the correct way is to update the PE/COFF loader and the relocation logic can deal with sections being laid out differently in memory than they are in the file.

--
Ard.

Post by Yao, Jiewen
-----Original Message-----
Sent: Monday, June 29, 2015 6:46 PM
Ersek; Justen, Jordan L; Kinney, Michael D; Zeng, Star
Subject: BUG in properties table feature implementation
Hello all,
I am running into another problem with the implementation of the UEFI
2.5 Properties Table feature. It splits PE/COFF images into separate but adjacent memory regions, only to be able to assign different permissions to .text and .data sections. This is working fine at boot time.
However, at runtime, after calling virtual address map, this breaks
down completely. Since the virtual mapping supplied to
SetVirtualAddressMap() does not have to guarantee adjacency between code and data regions (of which the OS does not know whether they belong together or not), reapplying the relocations corrupts the memory image and breaks the runtime services.
For example, this region
0x00005eeb1000-0x00005eeb6fff [Runtime Code]
0x00005eeb7000-0x00005eec0fff [Runtime Data]
is mapped on AARCH64 as
EFI remap 0x000000005eeb1000 => 00000000440a1000
EFI remap 0x000000005eeb7000 => 00000000440b7000
which retains the relative alignment, but adds a 64 KB offset to the second regions so that the regions can still be mapped with different permissions when the OS is executing with a 64 KB page size.
As far as I can tell, this is in accordance with the spec, and was working fine until I tried to enable the properties table feature.
With that enabled, the two above regions could actually describe one single PE/COFF image in memory, and the 64 KB offset results in the relocations to be applied incorrectly.
I looked at PeCoffLoaderRelocateImageForRuntime () but to me, it is
not very obvious how to solve this. Obviously, our PE/COFF
implementation is not complete since it assumes file offset == memory
offset for sections, but this does not hold anymore for UEFI 2.5
I would also like to point out again that this is another result of the fact that this series was pushed through with any review or testing outside of the Intel firmware team. For features of this magnitude and complexity, more scrutiny and testing is obviously required.
Kind regards,
Ard.

Ard Biesheuvel

2015-06-29 13:42:18 UTC

Permalink

Post by Yao, Jiewen
Thanks for the sharing. Yes. I agree with you that this breaks backward compatibility.
Right, I do not think we can boot Win7 or old Win8 with this feature enabled.
Both UEFI OS and Firmware need support this UEFI2.5 Properties Table feature. I believe it is known by UEFI forum, when it is added to UEFI spec, and it is published.

Yes, but I don't think it is documented anywhere that the OS needs to
map all runtime regions adjacently even if it does not actually care
about the EFI_MEMORY_RO attributes. Essentially, the OS always need to
map adjacently to be compatible with UEFI 2.5, and the current AArch64
Linux kernel crashes badly on any UEFI 2,5 implementation that has
this feature enabled.

Post by Yao, Jiewen
UEFI 2.5 properties table is optional feature. It can be enabled or disabled.
The benefit of this feature is that: a UEFI2.5 OS can do memory protection based on UEFI memory map.
But the old UEFI OS cannot get any benefit.
So a platform can made judgment based on its need, as well as the UEFI 2.5 OS availability.

Unfortunately, this makes it an opt-in setting, and system will ship
with it disabled by default, in order to prevent boot failures on
older OSes. That is not typically how you want to deploy a security
feature.

There is another issue I would like your opinion about:
for AArch64, MdeModulePkg/Core/Dxe/Mem/Imem.h defines the following

#define EFI_ACPI_RUNTIME_PAGE_ALLOCATION_ALIGNMENT (SIZE_64KB)

and in CoreTerminateMemoryMap(), a final check is done that no Runtime
regions are aligned incorrectly.
Yet, with the Properties Table feature enabled, memory regions are
split without taking this into account. Strangely enough,
CoreTerminateMemoryMap () does not complain about this.

This is documented in the UEFI spec 2.3.6:
"""
All 4KiB memory pages allocated for use by runtime services (of types
EfiRuntimeServicesCode, EfiRuntimeServicesData and
EfiACPIMemoryNVS) must use identical ARM Memory Page Attributes (as described in
Table 8) within the same physical 64KiB page. Mixed attribute mappings
within a larger page
are not allowed.
"""

So I suppose the Properties table feature should use 64 KB not 4 KB
when executing on AArch64. However, the documentation for the
Properties Table feature mentions 4 KB explicitly, so that should go
into the errata as well.

Regards,
Ard.

Post by Yao, Jiewen
That is why we do not enable this PE/COFF 4K alignment in BaseTool as standard configuration.
## Publish PropertiesTable or not.
#
# If this PCD is TRUE, DxeCore publishs PropertiesTable.
# DxeCore evaluates if all runtime drivers has 4K aligned PE sections. If all
# PE sections in runtime drivers are 4K aligned, DxeCore sets BIT0 in
# PropertiesTable. Or DxeCore clears BIT0 in PropertiesTable.
# If this PCD is FALSE, DxeCore does not publish PropertiesTable.
#
# 1) Use EfiRuntimeServicesCode for runtime driver PE image code section and
# use EfiRuntimeServicesData for runtime driver PE image header and other section.
# 2) Set EfiRuntimeServicesCode to be EFI_MEMORY_RO.
# 3) Set EfiRuntimeServicesData to be EFI_MEMORY_XP.
# 4) Set EfiMemoryMappedIO and EfiMemoryMappedIOPortSpace to be EFI_MEMORY_XP.
#
# NOTE: Platform need gurantee this PCD is set correctly. Platform should set
# this PCD to be TURE if and only if all runtime driver has seperated Code/Data
# section. If PE code/data sections are merged, the result is unpredictable.
#
gEfiMdeModulePkgTokenSpaceGuid.PropertiesTableEnable|TRUE|BOOLEAN|0x0000006e
Thank you
Yao Jiewen
-----Original Message-----
Sent: Monday, June 29, 2015 8:44 PM
To: Yao, Jiewen
Subject: Re: BUG in properties table feature implementation

I see this as a big problem with the feature, as it breaks backward compatibility. IOW, you won't be able to boot Windows 7 on a UEFI 2.5 system unless you manually disable the feature (in the setup screen, I
suppose?)

Post by Yao, Jiewen
We also tested Win10, and Suse 11 GM. They are good.
Would you please share the information on which OS you are using?

I am using the upstream Linux kernel for AArch64. I am not using a distribution.

Post by Yao, Jiewen
All in all, if OS cannot guarantee the virtual memory map is adjacent, we have to disable this feature.

This is backwards: the feature should be implemented such that it cannot be trivially broken by a well-behaving OS. Note that nothing in the SetVirtualAddressMap () implementation suggests that the regions should be adjacent.
Perhaps it would have been better to introduce a new memory region type EfiRuntimeServicesPecoffData, so the OS at least knows which regions it should keep adjacent, (i.e., Code and PecoffData regions should be kept together)
So does PE/COFF even allow the memory image to deviate from the file image? If so, the correct way is to update the PE/COFF loader and the relocation logic can deal with sections being laid out differently in memory than they are in the file.
--
Ard.

Yao, Jiewen

2015-06-29 13:58:51 UTC

Permalink

Yes. It seems we need some special handling for AArch64.
If 64KiB is minimal paging unit, I think we need check 64KiB PE section alignment for AArch64.

I confess that I only validated IA32 and X64, and I did not take AArch64 into account.

BTW: There is no 64KiB alignment requirement for AArch32, right? I cannot find the word. So I just want to double confirm.

Thank you
Yao Jiewen

-----Original Message-----
From: Ard Biesheuvel [mailto:***@linaro.org]
Sent: Monday, June 29, 2015 9:42 PM
To: Yao, Jiewen
Cc: edk2-***@lists.sourceforge.net; Gao, Liming; Laszlo Ersek; Justen, Jordan L; Kinney, Michael D; Zeng, Star; Zimmer, Vincent; Fleming, Matt
Subject: Re: BUG in properties table feature implementation

Yes, but I don't think it is documented anywhere that the OS needs to map all runtime regions adjacently even if it does not actually care about the EFI_MEMORY_RO attributes. Essentially, the OS always need to map adjacently to be compatible with UEFI 2.5, and the current AArch64 Linux kernel crashes badly on any UEFI 2,5 implementation that has this feature enabled.

Unfortunately, this makes it an opt-in setting, and system will ship with it disabled by default, in order to prevent boot failures on older OSes. That is not typically how you want to deploy a security feature.

There is another issue I would like your opinion about:
for AArch64, MdeModulePkg/Core/Dxe/Mem/Imem.h defines the following

#define EFI_ACPI_RUNTIME_PAGE_ALLOCATION_ALIGNMENT (SIZE_64KB)

and in CoreTerminateMemoryMap(), a final check is done that no Runtime regions are aligned incorrectly.
Yet, with the Properties Table feature enabled, memory regions are split without taking this into account. Strangely enough, CoreTerminateMemoryMap () does not complain about this.

This is documented in the UEFI spec 2.3.6:
"""
All 4KiB memory pages allocated for use by runtime services (of types EfiRuntimeServicesCode, EfiRuntimeServicesData and
EfiACPIMemoryNVS) must use identical ARM Memory Page Attributes (as described in Table 8) within the same physical 64KiB page. Mixed attribute mappings within a larger page are not allowed.
"""

So I suppose the Properties table feature should use 64 KB not 4 KB when executing on AArch64. However, the documentation for the Properties Table feature mentions 4 KB explicitly, so that should go into the errata as well.

Regards,
Ard.

Post by Yao, Jiewen
That is why we do not enable this PE/COFF 4K alignment in BaseTool as standard configuration.
## Publish PropertiesTable or not.
#
# If this PCD is TRUE, DxeCore publishs PropertiesTable.
# DxeCore evaluates if all runtime drivers has 4K aligned PE sections. If all
# PE sections in runtime drivers are 4K aligned, DxeCore sets BIT0 in
# PropertiesTable. Or DxeCore clears BIT0 in PropertiesTable.
# If this PCD is FALSE, DxeCore does not publish PropertiesTable.
#
# 1) Use EfiRuntimeServicesCode for runtime driver PE image code section and
# use EfiRuntimeServicesData for runtime driver PE image header and other section.
# 2) Set EfiRuntimeServicesCode to be EFI_MEMORY_RO.
# 3) Set EfiRuntimeServicesData to be EFI_MEMORY_XP.
# 4) Set EfiMemoryMappedIO and EfiMemoryMappedIOPortSpace to be EFI_MEMORY_XP.
#
# NOTE: Platform need gurantee this PCD is set correctly. Platform should set
# this PCD to be TURE if and only if all runtime driver has seperated Code/Data
# section. If PE code/data sections are merged, the result is unpredictable.
#
gEfiMdeModulePkgTokenSpaceGuid.PropertiesTableEnable|TRUE|BOOLEAN|0x00
00006e
Thank you
Yao Jiewen
-----Original Message-----
Sent: Monday, June 29, 2015 8:44 PM
To: Yao, Jiewen
Justen, Jordan L; Kinney, Michael D; Zeng, Star
Subject: Re: BUG in properties table feature implementation

Post by Yao, Jiewen
We also tested Win10, and Suse 11 GM. They are good.
Would you please share the information on which OS you are using?

I am using the upstream Linux kernel for AArch64. I am not using a distribution.

Post by Yao, Jiewen
All in all, if OS cannot guarantee the virtual memory map is adjacent, we have to disable this feature.

This is backwards: the feature should be implemented such that it cannot be trivially broken by a well-behaving OS. Note that nothing in the SetVirtualAddressMap () implementation suggests that the regions should be adjacent.
Perhaps it would have been better to introduce a new memory region
type EfiRuntimeServicesPecoffData, so the OS at least knows which
regions it should keep adjacent, (i.e., Code and PecoffData regions
should be kept together)
So does PE/COFF even allow the memory image to deviate from the file image? If so, the correct way is to update the PE/COFF loader and the relocation logic can deal with sections being laid out differently in memory than they are in the file.
--
Ard.

Post by Yao, Jiewen
-----Original Message-----
Sent: Monday, June 29, 2015 6:46 PM
Laszlo Ersek; Justen, Jordan L; Kinney, Michael D; Zeng, Star
Subject: BUG in properties table feature implementation
Hello all,
I am running into another problem with the implementation of the UEFI
2.5 Properties Table feature. It splits PE/COFF images into separate but adjacent memory regions, only to be able to assign different permissions to .text and .data sections. This is working fine at boot time.
However, at runtime, after calling virtual address map, this breaks
down completely. Since the virtual mapping supplied to
SetVirtualAddressMap() does not have to guarantee adjacency between code and data regions (of which the OS does not know whether they belong together or not), reapplying the relocations corrupts the memory image and breaks the runtime services.
For example, this region
0x00005eeb1000-0x00005eeb6fff [Runtime Code]
0x00005eeb7000-0x00005eec0fff [Runtime Data]
is mapped on AARCH64 as
EFI remap 0x000000005eeb1000 => 00000000440a1000
EFI remap 0x000000005eeb7000 => 00000000440b7000
which retains the relative alignment, but adds a 64 KB offset to the second regions so that the regions can still be mapped with different permissions when the OS is executing with a 64 KB page size.
As far as I can tell, this is in accordance with the spec, and was working fine until I tried to enable the properties table feature.
With that enabled, the two above regions could actually describe one single PE/COFF image in memory, and the 64 KB offset results in the relocations to be applied incorrectly.
I looked at PeCoffLoaderRelocateImageForRuntime () but to me, it is
not very obvious how to solve this. Obviously, our PE/COFF
implementation is not complete since it assumes file offset == memory
offset for sections, but this does not hold anymore for UEFI 2.5
I would also like to point out again that this is another result of the fact that this series was pushed through with any review or testing outside of the Intel firmware team. For features of this magnitude and complexity, more scrutiny and testing is obviously required.
Kind regards,
Ard.

Ard Biesheuvel

2015-06-29 14:05:37 UTC

Permalink

Post by Yao, Jiewen
Yes. It seems we need some special handling for AArch64.
If 64KiB is minimal paging unit, I think we need check 64KiB PE section alignment for AArch64.
I confess that I only validated IA32 and X64, and I did not take AArch64 into account.
BTW: There is no 64KiB alignment requirement for AArch32, right? I cannot find the word. So I just want to double confirm.

No there is not. Only 64-bit ARM can execute with 64 KB page size.
--
Ard.

Post by Yao, Jiewen
-----Original Message-----
Sent: Monday, June 29, 2015 9:42 PM
To: Yao, Jiewen
Subject: Re: BUG in properties table feature implementation

Yes, but I don't think it is documented anywhere that the OS needs to map all runtime regions adjacently even if it does not actually care about the EFI_MEMORY_RO attributes. Essentially, the OS always need to map adjacently to be compatible with UEFI 2.5, and the current AArch64 Linux kernel crashes badly on any UEFI 2,5 implementation that has this feature enabled.

Unfortunately, this makes it an opt-in setting, and system will ship with it disabled by default, in order to prevent boot failures on older OSes. That is not typically how you want to deploy a security feature.
for AArch64, MdeModulePkg/Core/Dxe/Mem/Imem.h defines the following
#define EFI_ACPI_RUNTIME_PAGE_ALLOCATION_ALIGNMENT (SIZE_64KB)
and in CoreTerminateMemoryMap(), a final check is done that no Runtime regions are aligned incorrectly.
Yet, with the Properties Table feature enabled, memory regions are split without taking this into account. Strangely enough, CoreTerminateMemoryMap () does not complain about this.
"""
All 4KiB memory pages allocated for use by runtime services (of types EfiRuntimeServicesCode, EfiRuntimeServicesData and
EfiACPIMemoryNVS) must use identical ARM Memory Page Attributes (as described in Table 8) within the same physical 64KiB page. Mixed attribute mappings within a larger page are not allowed.
"""
So I suppose the Properties table feature should use 64 KB not 4 KB when executing on AArch64. However, the documentation for the Properties Table feature mentions 4 KB explicitly, so that should go into the errata as well.
Regards,
Ard.

Post by Yao, Jiewen
That is why we do not enable this PE/COFF 4K alignment in BaseTool as standard configuration.
## Publish PropertiesTable or not.
#
# If this PCD is TRUE, DxeCore publishs PropertiesTable.
# DxeCore evaluates if all runtime drivers has 4K aligned PE sections. If all
# PE sections in runtime drivers are 4K aligned, DxeCore sets BIT0 in
# PropertiesTable. Or DxeCore clears BIT0 in PropertiesTable.
# If this PCD is FALSE, DxeCore does not publish PropertiesTable.
#
# 1) Use EfiRuntimeServicesCode for runtime driver PE image code section and
# use EfiRuntimeServicesData for runtime driver PE image header and other section.
# 2) Set EfiRuntimeServicesCode to be EFI_MEMORY_RO.
# 3) Set EfiRuntimeServicesData to be EFI_MEMORY_XP.
# 4) Set EfiMemoryMappedIO and EfiMemoryMappedIOPortSpace to be EFI_MEMORY_XP.
#
# NOTE: Platform need gurantee this PCD is set correctly. Platform should set
# this PCD to be TURE if and only if all runtime driver has seperated Code/Data
# section. If PE code/data sections are merged, the result is unpredictable.
#
gEfiMdeModulePkgTokenSpaceGuid.PropertiesTableEnable|TRUE|BOOLEAN|0x00
00006e
Thank you
Yao Jiewen
-----Original Message-----
Sent: Monday, June 29, 2015 8:44 PM
To: Yao, Jiewen
Justen, Jordan L; Kinney, Michael D; Zeng, Star
Subject: Re: BUG in properties table feature implementation

Post by Yao, Jiewen
We also tested Win10, and Suse 11 GM. They are good.
Would you please share the information on which OS you are using?

I am using the upstream Linux kernel for AArch64. I am not using a distribution.

Post by Yao, Jiewen
All in all, if OS cannot guarantee the virtual memory map is adjacent, we have to disable this feature.

This is backwards: the feature should be implemented such that it cannot be trivially broken by a well-behaving OS. Note that nothing in the SetVirtualAddressMap () implementation suggests that the regions should be adjacent.
Perhaps it would have been better to introduce a new memory region
type EfiRuntimeServicesPecoffData, so the OS at least knows which
regions it should keep adjacent, (i.e., Code and PecoffData regions
should be kept together)
So does PE/COFF even allow the memory image to deviate from the file image? If so, the correct way is to update the PE/COFF loader and the relocation logic can deal with sections being laid out differently in memory than they are in the file.
--
Ard.

Yao, Jiewen

2015-06-29 14:08:27 UTC

Permalink

Good to know. Thanks!

-----Original Message-----
From: Ard Biesheuvel [mailto:***@linaro.org]
Sent: Monday, June 29, 2015 10:06 PM
To: Yao, Jiewen
Cc: edk2-***@lists.sourceforge.net; Gao, Liming; Laszlo Ersek; Justen, Jordan L; Kinney, Michael D; Zeng, Star; Zimmer, Vincent; Fleming, Matt
Subject: Re: BUG in properties table feature implementation