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

[Laszlo Ersek <lersek@redhat.com>]

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.
>