On Sat, Aug 5, 2017 at 1:06 PM, Rob Clark <robdcl...@gmail.com> wrote: > On Sat, Aug 5, 2017 at 12:52 PM, Heinrich Schuchardt <xypron.g...@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.g...@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 <robdcl...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot