diff mbox

[edk2] MdeModulePkg/DxeCore: base code protection on permission attributes

Message ID 1487958664-10707-1-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel Feb. 24, 2017, 5:51 p.m. UTC 
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(-)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Yao, Jiewen Feb. 25, 2017, 4:04 a.m. UTC | #1 
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
Ard Biesheuvel Feb. 26, 2017, 2 p.m. UTC | #2 
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
Laszlo Ersek March 17, 2017, 12:07 p.m. UTC | #3 
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
Ard Biesheuvel March 17, 2017, 12:11 p.m. UTC | #4 
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
Laszlo Ersek March 17, 2017, 12:35 p.m. UTC | #5 
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
Ard Biesheuvel March 17, 2017, 1:23 p.m. UTC | #6 
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
diff mbox

Patch

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));