MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Instead of assuming that a PE/COFF section of type EFI_IMAGE_SCN_CNT_CODE
can always be mapped read-only, classify a section as a code section only
if it has the executable attribute set and the writable attribute cleared.
This adheres more closely to the PE/COFF spec, and avoids issues with
Linux OS loaders that consists of a single read/write/execute section.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 1142dcc5a83d..3e037607a6be 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -533,7 +533,7 @@ ProtectUefiImageCommon (
Name[7]
));
- if ((Section[Index].Characteristics & EFI_IMAGE_SCN_CNT_CODE) != 0) {
+ if ((Section[Index].Characteristics & (EFI_IMAGE_SCN_MEM_WRITE | EFI_IMAGE_SCN_MEM_EXECUTE)) == EFI_IMAGE_SCN_MEM_EXECUTE) {
DEBUG ((DEBUG_VERBOSE, " VirtualSize - 0x%08x\n", Section[Index].Misc.VirtualSize));
DEBUG ((DEBUG_VERBOSE, " VirtualAddress - 0x%08x\n", Section[Index].VirtualAddress));
DEBUG ((DEBUG_VERBOSE, " SizeOfRawData - 0x%08x\n", Section[Index].SizeOfRawData));
--
2.7.4
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Hi Ard
I agree with you on this enhancement.
I prefer to adding the description as comment in the code, so that people can get clear picture when he/she reads the code.
//
// Instead of assuming that a PE/COFF section of type EFI_IMAGE_SCN_CNT_CODE
// can always be mapped read-only, classify a section as a code section only
// if it has the executable attribute set and the writable attribute cleared.
//
// This adheres more closely to the PE/COFF spec, and avoids issues with
// Linux OS loaders that consists of a single read/write/execute section.
//
if ((Section[Index].Characteristics & (EFI_IMAGE_SCN_MEM_WRITE | EFI_IMAGE_SCN_MEM_EXECUTE)) == EFI_IMAGE_SCN_MEM_EXECUTE) {
With comment update, reviewed-by: Jiewen.yao@intel.com
Thank you
Yao Jiewen
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Saturday, February 25, 2017 1:51 AM
> To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Gao, Liming <liming.gao@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: [PATCH] MdeModulePkg/DxeCore: base code protection on permission
> attributes
>
> Instead of assuming that a PE/COFF section of type EFI_IMAGE_SCN_CNT_CODE
> can always be mapped read-only, classify a section as a code section only
> if it has the executable attribute set and the writable attribute cleared.
>
> This adheres more closely to the PE/COFF spec, and avoids issues with
> Linux OS loaders that consists of a single read/write/execute section.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index 1142dcc5a83d..3e037607a6be 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -533,7 +533,7 @@ ProtectUefiImageCommon (
> Name[7]
> ));
>
> - if ((Section[Index].Characteristics & EFI_IMAGE_SCN_CNT_CODE) != 0) {
> + if ((Section[Index].Characteristics & (EFI_IMAGE_SCN_MEM_WRITE |
> EFI_IMAGE_SCN_MEM_EXECUTE)) == EFI_IMAGE_SCN_MEM_EXECUTE) {
> DEBUG ((DEBUG_VERBOSE, " VirtualSize - 0x%08x\n",
> Section[Index].Misc.VirtualSize));
> DEBUG ((DEBUG_VERBOSE, " VirtualAddress - 0x%08x\n",
> Section[Index].VirtualAddress));
> DEBUG ((DEBUG_VERBOSE, " SizeOfRawData - 0x%08x\n",
> Section[Index].SizeOfRawData));
> --
> 2.7.4
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 25 February 2017 at 04:04, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> Hi Ard
> I agree with you on this enhancement.
>
> I prefer to adding the description as comment in the code, so that people
> can get clear picture when he/she reads the code.
>
> //
> // Instead of assuming that a PE/COFF section of type EFI_IMAGE_SCN_CNT_CODE
> // can always be mapped read-only, classify a section as a code section only
> // if it has the executable attribute set and the writable attribute
> cleared.
> //
> // This adheres more closely to the PE/COFF spec, and avoids issues with
> // Linux OS loaders that consists of a single read/write/execute section.
> //
> if ((Section[Index].Characteristics & (EFI_IMAGE_SCN_MEM_WRITE |
> EFI_IMAGE_SCN_MEM_EXECUTE)) == EFI_IMAGE_SCN_MEM_EXECUTE) {
>
> With comment update, reviewed-by: Jiewen.yao@intel.com
>
Thanks Jiewen
Pushed as a2ed40c02bf2
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Saturday, February 25, 2017 1:51 AM
>> To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>
>> Cc: Gao, Liming <liming.gao@intel.com>; Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>
>> Subject: [PATCH] MdeModulePkg/DxeCore: base code protection on permission
>> attributes
>
>
>>
>> Instead of assuming that a PE/COFF section of type EFI_IMAGE_SCN_CNT_CODE
>> can always be mapped read-only, classify a section as a code section only
>> if it has the executable attribute set and the writable attribute cleared.
>>
>> This adheres more closely to the PE/COFF spec, and avoids issues with
>> Linux OS loaders that consists of a single read/write/execute section.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>> index 1142dcc5a83d..3e037607a6be 100644
>> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>> @@ -533,7 +533,7 @@ ProtectUefiImageCommon (
>> Name[7]
>> ));
>>
>> - if ((Section[Index].Characteristics & EFI_IMAGE_SCN_CNT_CODE) != 0) {
>> + if ((Section[Index].Characteristics & (EFI_IMAGE_SCN_MEM_WRITE |
>> EFI_IMAGE_SCN_MEM_EXECUTE)) == EFI_IMAGE_SCN_MEM_EXECUTE) {
>> DEBUG ((DEBUG_VERBOSE, " VirtualSize - 0x%08x\n",
>> Section[Index].Misc.VirtualSize));
>> DEBUG ((DEBUG_VERBOSE, " VirtualAddress - 0x%08x\n",
>> Section[Index].VirtualAddress));
>> DEBUG ((DEBUG_VERBOSE, " SizeOfRawData - 0x%08x\n",
>> Section[Index].SizeOfRawData));
>> --
>> 2.7.4
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 02/26/17 15:00, Ard Biesheuvel wrote:
> On 25 February 2017 at 04:04, Yao, Jiewen <jiewen.yao@intel.com> wrote:
>> Hi Ard
>> I agree with you on this enhancement.
>>
>> I prefer to adding the description as comment in the code, so that people
>> can get clear picture when he/she reads the code.
>>
>> //
>> // Instead of assuming that a PE/COFF section of type EFI_IMAGE_SCN_CNT_CODE
>> // can always be mapped read-only, classify a section as a code section only
>> // if it has the executable attribute set and the writable attribute
>> cleared.
>> //
>> // This adheres more closely to the PE/COFF spec, and avoids issues with
>> // Linux OS loaders that consists of a single read/write/execute section.
>> //
>> if ((Section[Index].Characteristics & (EFI_IMAGE_SCN_MEM_WRITE |
>> EFI_IMAGE_SCN_MEM_EXECUTE)) == EFI_IMAGE_SCN_MEM_EXECUTE) {
>>
>> With comment update, reviewed-by: Jiewen.yao@intel.com
>>
>
> Thanks Jiewen
>
> Pushed as a2ed40c02bf2
Is it possible that (recent?) Linux EFI stubs (aarch64) don't pass the
above check? I got a report from a colleague:
!!!!!!!! ProtectUefiImageCommon - CodeSegmentCount is 0 !!!!!!!!
EFI stub: Booting Linux Kernel...
EFI stub: Using DTB from configuration table
EFI stub: Exiting boot services and installing virtual address map...
I tried to reproduce it with "4.5.0-15.el7.aarch64", and with
"4.8.7-300.fc25.aarch64", and I'm not seeing the message with either.
I asked him about the exact kernel version (no answer yet, his workday
hasn't started yet).
Any idea how I could validate the section headers of a (decompressed)
kernel image? I tried "aarch64-linux-gnu-objdump --section-headers", but
it doesn't recognize the image.
Thanks,
Laszlo
>>> -----Original Message-----
>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>>> Sent: Saturday, February 25, 2017 1:51 AM
>>> To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>
>>> Cc: Gao, Liming <liming.gao@intel.com>; Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org>
>>> Subject: [PATCH] MdeModulePkg/DxeCore: base code protection on permission
>>> attributes
>>
>>
>>>
>>> Instead of assuming that a PE/COFF section of type EFI_IMAGE_SCN_CNT_CODE
>>> can always be mapped read-only, classify a section as a code section only
>>> if it has the executable attribute set and the writable attribute cleared.
>>>
>>> This adheres more closely to the PE/COFF spec, and avoids issues with
>>> Linux OS loaders that consists of a single read/write/execute section.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>>> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>>> index 1142dcc5a83d..3e037607a6be 100644
>>> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>>> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>>> @@ -533,7 +533,7 @@ ProtectUefiImageCommon (
>>> Name[7]
>>> ));
>>>
>>> - if ((Section[Index].Characteristics & EFI_IMAGE_SCN_CNT_CODE) != 0) {
>>> + if ((Section[Index].Characteristics & (EFI_IMAGE_SCN_MEM_WRITE |
>>> EFI_IMAGE_SCN_MEM_EXECUTE)) == EFI_IMAGE_SCN_MEM_EXECUTE) {
>>> DEBUG ((DEBUG_VERBOSE, " VirtualSize - 0x%08x\n",
>>> Section[Index].Misc.VirtualSize));
>>> DEBUG ((DEBUG_VERBOSE, " VirtualAddress - 0x%08x\n",
>>> Section[Index].VirtualAddress));
>>> DEBUG ((DEBUG_VERBOSE, " SizeOfRawData - 0x%08x\n",
>>> Section[Index].SizeOfRawData));
>>> --
>>> 2.7.4
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 17 March 2017 at 12:07, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/26/17 15:00, Ard Biesheuvel wrote:
>> On 25 February 2017 at 04:04, Yao, Jiewen <jiewen.yao@intel.com> wrote:
>>> Hi Ard
>>> I agree with you on this enhancement.
>>>
>>> I prefer to adding the description as comment in the code, so that people
>>> can get clear picture when he/she reads the code.
>>>
>>> //
>>> // Instead of assuming that a PE/COFF section of type EFI_IMAGE_SCN_CNT_CODE
>>> // can always be mapped read-only, classify a section as a code section only
>>> // if it has the executable attribute set and the writable attribute
>>> cleared.
>>> //
>>> // This adheres more closely to the PE/COFF spec, and avoids issues with
>>> // Linux OS loaders that consists of a single read/write/execute section.
>>> //
>>> if ((Section[Index].Characteristics & (EFI_IMAGE_SCN_MEM_WRITE |
>>> EFI_IMAGE_SCN_MEM_EXECUTE)) == EFI_IMAGE_SCN_MEM_EXECUTE) {
>>>
>>> With comment update, reviewed-by: Jiewen.yao@intel.com
>>>
>>
>> Thanks Jiewen
>>
>> Pushed as a2ed40c02bf2
>
> Is it possible that (recent?) Linux EFI stubs (aarch64) don't pass the
> above check? I got a report from a colleague:
>
> !!!!!!!! ProtectUefiImageCommon - CodeSegmentCount is 0 !!!!!!!!
> EFI stub: Booting Linux Kernel...
> EFI stub: Using DTB from configuration table
> EFI stub: Exiting boot services and installing virtual address map...
>
> I tried to reproduce it with "4.5.0-15.el7.aarch64", and with
> "4.8.7-300.fc25.aarch64", and I'm not seeing the message with either.
>
> I asked him about the exact kernel version (no answer yet, his workday
> hasn't started yet).
>
Well, the warning may be a bit loud, but this is expected behavior. I
expect to be able to queue the linux/arm64 changes that split the
PE/COFF sections and align them to 4 KB for v4.12, and so current
kernels all consists of a single rwx section, which does not qualify
as a code section.
> Any idea how I could validate the section headers of a (decompressed)
> kernel image? I tried "aarch64-linux-gnu-objdump --section-headers", but
> it doesn't recognize the image.
>
It's a PE/COFF image not an ELF image.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 03/17/17 13:11, Ard Biesheuvel wrote:
> On 17 March 2017 at 12:07, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 02/26/17 15:00, Ard Biesheuvel wrote:
>>> On 25 February 2017 at 04:04, Yao, Jiewen <jiewen.yao@intel.com> wrote:
>>>> Hi Ard
>>>> I agree with you on this enhancement.
>>>>
>>>> I prefer to adding the description as comment in the code, so that people
>>>> can get clear picture when he/she reads the code.
>>>>
>>>> //
>>>> // Instead of assuming that a PE/COFF section of type EFI_IMAGE_SCN_CNT_CODE
>>>> // can always be mapped read-only, classify a section as a code section only
>>>> // if it has the executable attribute set and the writable attribute
>>>> cleared.
>>>> //
>>>> // This adheres more closely to the PE/COFF spec, and avoids issues with
>>>> // Linux OS loaders that consists of a single read/write/execute section.
>>>> //
>>>> if ((Section[Index].Characteristics & (EFI_IMAGE_SCN_MEM_WRITE |
>>>> EFI_IMAGE_SCN_MEM_EXECUTE)) == EFI_IMAGE_SCN_MEM_EXECUTE) {
>>>>
>>>> With comment update, reviewed-by: Jiewen.yao@intel.com
>>>>
>>>
>>> Thanks Jiewen
>>>
>>> Pushed as a2ed40c02bf2
>>
>> Is it possible that (recent?) Linux EFI stubs (aarch64) don't pass the
>> above check? I got a report from a colleague:
>>
>> !!!!!!!! ProtectUefiImageCommon - CodeSegmentCount is 0 !!!!!!!!
>> EFI stub: Booting Linux Kernel...
>> EFI stub: Using DTB from configuration table
>> EFI stub: Exiting boot services and installing virtual address map...
>>
>> I tried to reproduce it with "4.5.0-15.el7.aarch64", and with
>> "4.8.7-300.fc25.aarch64", and I'm not seeing the message with either.
>>
>> I asked him about the exact kernel version (no answer yet, his workday
>> hasn't started yet).
>>
>
> Well, the warning may be a bit loud, but this is expected behavior. I
> expect to be able to queue the linux/arm64 changes that split the
> PE/COFF sections and align them to 4 KB for v4.12, and so current
> kernels all consists of a single rwx section, which does not qualify
> as a code section.
Okay, thanks.
In that case, does it make sense to downgrade the messages from DEBUG_ERROR to DEBUG_WARN?
if (ImageRecord->CodeSegmentCount == 0) {
DEBUG ((DEBUG_ERROR, "!!!!!!!! ProtectUefiImageCommon - CodeSegmentCount is 0 !!!!!!!!\n"));
PdbPointer = PeCoffLoaderGetPdbPointer ((VOID*) (UINTN) ImageAddress);
if (PdbPointer != NULL) {
DEBUG ((DEBUG_ERROR, "!!!!!!!! Image - %a !!!!!!!!\n", PdbPointer));
}
goto Finish;
}
Thanks
Laszlo
>> Any idea how I could validate the section headers of a (decompressed)
>> kernel image? I tried "aarch64-linux-gnu-objdump --section-headers", but
>> it doesn't recognize the image.
>>
>
> It's a PE/COFF image not an ELF image.
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 17 March 2017 at 12:35, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/17/17 13:11, Ard Biesheuvel wrote:
>> On 17 March 2017 at 12:07, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 02/26/17 15:00, Ard Biesheuvel wrote:
>>>> On 25 February 2017 at 04:04, Yao, Jiewen <jiewen.yao@intel.com> wrote:
>>>>> Hi Ard
>>>>> I agree with you on this enhancement.
>>>>>
>>>>> I prefer to adding the description as comment in the code, so that people
>>>>> can get clear picture when he/she reads the code.
>>>>>
>>>>> //
>>>>> // Instead of assuming that a PE/COFF section of type EFI_IMAGE_SCN_CNT_CODE
>>>>> // can always be mapped read-only, classify a section as a code section only
>>>>> // if it has the executable attribute set and the writable attribute
>>>>> cleared.
>>>>> //
>>>>> // This adheres more closely to the PE/COFF spec, and avoids issues with
>>>>> // Linux OS loaders that consists of a single read/write/execute section.
>>>>> //
>>>>> if ((Section[Index].Characteristics & (EFI_IMAGE_SCN_MEM_WRITE |
>>>>> EFI_IMAGE_SCN_MEM_EXECUTE)) == EFI_IMAGE_SCN_MEM_EXECUTE) {
>>>>>
>>>>> With comment update, reviewed-by: Jiewen.yao@intel.com
>>>>>
>>>>
>>>> Thanks Jiewen
>>>>
>>>> Pushed as a2ed40c02bf2
>>>
>>> Is it possible that (recent?) Linux EFI stubs (aarch64) don't pass the
>>> above check? I got a report from a colleague:
>>>
>>> !!!!!!!! ProtectUefiImageCommon - CodeSegmentCount is 0 !!!!!!!!
>>> EFI stub: Booting Linux Kernel...
>>> EFI stub: Using DTB from configuration table
>>> EFI stub: Exiting boot services and installing virtual address map...
>>>
>>> I tried to reproduce it with "4.5.0-15.el7.aarch64", and with
>>> "4.8.7-300.fc25.aarch64", and I'm not seeing the message with either.
>>>
>>> I asked him about the exact kernel version (no answer yet, his workday
>>> hasn't started yet).
>>>
>>
>> Well, the warning may be a bit loud, but this is expected behavior. I
>> expect to be able to queue the linux/arm64 changes that split the
>> PE/COFF sections and align them to 4 KB for v4.12, and so current
>> kernels all consists of a single rwx section, which does not qualify
>> as a code section.
>
> Okay, thanks.
>
> In that case, does it make sense to downgrade the messages from DEBUG_ERROR to DEBUG_WARN?
>
> if (ImageRecord->CodeSegmentCount == 0) {
> DEBUG ((DEBUG_ERROR, "!!!!!!!! ProtectUefiImageCommon - CodeSegmentCount is 0 !!!!!!!!\n"));
> PdbPointer = PeCoffLoaderGetPdbPointer ((VOID*) (UINTN) ImageAddress);
> if (PdbPointer != NULL) {
> DEBUG ((DEBUG_ERROR, "!!!!!!!! Image - %a !!!!!!!!\n", PdbPointer));
> }
> goto Finish;
> }
>
Yes, that sounds reasonable.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.