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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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