[U-Boot] [PATCH v0 21/20] efi_loader: hack for archs that cannot do unaligned accesses

[Rob Clark]

On Sat, Aug 5, 2017 at 1:06 PM, Rob Clark <robdclark at gmail.com> wrote:
> On Sat, Aug 5, 2017 at 12:52 PM, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>> On 08/05/2017 06:16 PM, Rob Clark wrote:
>>> On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>>> On 08/05/2017 05:58 PM, Rob Clark wrote:
>>>>> Some arch's have trouble with unaligned accesses.  Technically
>>>>> EFI device-path structs should be byte aligned, and the next node
>>>>> in the path starts immediately after the previous.  Meaning that
>>>>> a pointer to an 'struct efi_device_path' is not necessarily word
>>>>> aligned.  See section 10.3.1 in v2.7 of UEFI spec.
>>>>>
>>>>> This causes problems not just for u-boot, but also most/all EFI
>>>>> payloads loaded by u-boot on these archs.  Fortunately the common
>>>>> practice for traversing a device path is to rely on the length
>>>>> field in the header, rather than the specified length of the
>>>>> particular device path type+subtype.  So the EFI_DP_PAD() macro
>>>>> will add the specified number of bytes to the tail of device path
>>>>> structs to pad them to word alignment.
>>>>>
>>>>> Technically this is non-compliant, BROKEN_UNALIGNED should *only*
>>>>> be defined on archs that cannot do unaligned accesses.
>>>>>
>>>>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>>>>> ---
>>>>> I'm not sure if there are other arch's that need -DBROKEN_UNALIGNED
>>>>>
>>>>> Mark, this is untested but I think it should solve your crash on the
>>>>> Banana Pi.  Could you give it a try when you get a chance?
>>>>>
>>>>>  arch/arm/config.mk               |  2 +-
>>>>>  include/efi_api.h                | 30 ++++++++++++++++++++++++++++++
>>>>>  lib/efi_loader/efi_device_path.c |  3 +++
>>>>>  3 files changed, 34 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/arm/config.mk b/arch/arm/config.mk
>>>>> index 1a77779db4..067dc93a9d 100644
>>>>> --- a/arch/arm/config.mk
>>>>> +++ b/arch/arm/config.mk
>>>>> @@ -28,7 +28,7 @@ LLVMS_RELFLAGS              := $(call cc-option,-mllvm,) \
>>>>>                       $(call cc-option,-arm-use-movt=0,)
>>>>>  PLATFORM_RELFLAGS    += $(LLVM_RELFLAGS)
>>>>>
>>>>> -PLATFORM_CPPFLAGS += -D__ARM__
>>>>> +PLATFORM_CPPFLAGS += -D__ARM__ -DBROKEN_UNALIGNED
>>>>
>>>> NAK
>>>>
>>>> We have more then ARM. And other architectures also create exceptions
>>>> for unaligned access.
>>>>
>>>> I hate platform specific code. It should not be used outside /arch.
>>>>
>>>> To play it save you should not use _packed at all!
>>>> Use memcpy to transfer between aligned and unaligned memory.
>>>
>>> except for reasons I explained in the thread on the patch that added
>>> the __packed in the first place.  Sorry, this is ugly but we have to
>>> do it.
>>>
>>> BR,
>>> -R
>>
>> According to the UEFI standard the nodes in paths are not to be assumed
>> to be aligned.
>>
>> So even if you use padding bytes in paths that you pass to the EFI
>> application you should not expect that the EFI application does the
>> same. Expect the EFI application either to remove them or send new nodes
>> without padding.
>>
>> To the idea of padding bytes and __packed does not make sense.
>
> Ok, to be fair, you are right about device-paths passed too u-boot.
> On BROKEN_UNALIGNED archs, we should sanitise with a copy to an
> aligned device-path in *addition* to what I proposed.  I can make a
> patch to add a helper to do this a bit later.
>
> But the padding bytes + __packed does make total sense because it
> avoids breaking efi payloads that already exist in the field.  The
> crash that Mark saw was not in u-boot, but in openbsd's bootaa64.efi.
> (It is possibly that we just get lucky here in u-boot since I add the
> /End node after the mac address by something the compiler turns into a
> memcpy.)
>
> My proposal simply preserves the bug that we already have on
> BROKEN_UNALIGNED archs (but makes the improvement that it fixes the
> bug on aarch64 and any other arch that can do unaligned access), to
> keep existing efi payloads working.
>

Ok, so I took a closer look at the assembly generated, and I realized
that with __packed structs, gcc seems to be generating ldrb+orr's for
*all* the fields, in other words it isn't assuming alignment of the
device-path pointer.  The only potential problem right now is that we
are missing __packed on 'struct efi_device_path' itself, so
dereferencing the length field could cause unaligned access.  Adding
the missing __packed annotation turns the generated code into a pair
of ldrb's plus an orr.  I'll add the missing __packed on
efi_device_path to my patchset.

(I'm basing this on the asm generated for vexpress_ca15_tc2 build.)

That is the good news.. the bad news is this probably still ends up
being a problem in a few places w/ utf16 strings (ie.
ascii2unicode()'ing into a filepath node, or converting filenames
passed from efi payload from utf16..).. I'll have to think a bit about
how best to handle that.  But at least this is all stuff that never
worked before in the first place, so I guess we don't have to solve it
immediately.

BR,
-R