[PATCH v3] arm64/efi: efistub: jump to 'stext' directly, not through the header

[Ard Biesheuvel]

On 10 October 2014 12:49, Mark Rutland <mark.rutland at arm.com> wrote:
> On Thu, Oct 09, 2014 at 08:03:52PM +0100, Ard Biesheuvel wrote:
>> On 9 October 2014 19:23, Mark Rutland <mark.rutland at arm.com> wrote:
>> > Hi Ard,
>> >
>> > On Wed, Oct 08, 2014 at 03:11:27PM +0100, Ard Biesheuvel wrote:
>> >> After the EFI stub has done its business, it jumps into the kernel by
>> >> branching to offset #0 of the loaded Image, which is where it expects
>> >> to find the header containing a 'branch to stext' instruction.
>> >>
>> >> However, the UEFI spec 2.1.1 states the following regarding PE/COFF
>> >> image loading:
>> >> "A UEFI image is loaded into memory through the LoadImage() Boot
>> >> Service. This service loads an image with a PE32+ format into memory.
>> >> This PE32+ loader is required to load all sections of the PE32+ image
>> >> into memory."
>> >>
>> >> In other words, it is /not/ required to load parts of the image that are
>> >> not covered by a PE/COFF section, so it may not have loaded the header
>> >> at the expected offset, as it is not covered by any PE/COFF section.
>> >
>> > What does this mean for handle_kernel_image? Given we might not have
>> > _text through to _stext mapped, do we not need to take that into
>> > account?
>> >
>>
>> Actually, handle_kernel_image() does not interpret the header, it just
>> copies it along with the rest of the image if it needs to be
>> relocated, so I don't see an issue there.
>
> Sorry, I wasn't clear enough with my concern. My concern was whether we
> had any guarantee _something_ was mapped for the address range covering
> efi_head to stext.
>
> So long as _something_ is mapped there, we're ok -- handle_kernel_image
> will just copy some garbage along with the usable portion of the kernel.
>

Indeed.

> But if the EFI loader is allowed to load stext at the precise start of
> RAM (or anywhere not in the idmap), in attempting the copy we'd try to
> access unmapped addresses.
>
> So if that's a possibility, we need to shrink the copy to cover stext
> to _edata rather than _text to edata.
>
> Does that make sense?
>

That cannot happen. The PE/COFF .text section's positive relative
virtual offset ensures that the memory image has room for the header,
it's just not guaranteed that anything gets copied there.

>> However, I do remember Mark
>> Salter mentioning that there is at least one other location that needs
>> to be fixed up if this concern is valid. Mark?
>>
>> > Also, have we seen problems on any systems yet?
>> >
>>
>> No, I am not aware of any occurrences of this exact issue, this is
>> just one of the things I spotted while working on this code.
>
> Ok. I was just curious as to how urgent this was.
>
>> But I think we mostly agree that branching through the header relies
>> on behavior of the PE/COFF loader that is not covered by the spec.
>
> Yes. We should not rely on unspecified behaviour.
>
> Mark.