On Tue, Aug 8, 2017 at 2:52 AM, Alexander Graf <ag...@suse.de> wrote: > > >> Am 07.08.2017 um 23:18 schrieb Rob Clark <robdcl...@gmail.com>: >> >> On Mon, Aug 7, 2017 at 5:14 PM, Mark Kettenis <mark.kette...@xs4all.nl> >> wrote: >>>> From: Alexander Graf <ag...@suse.de> >>>> Date: Mon, 7 Aug 2017 21:19:37 +0100 >>>> >>>>> On 05.08.17 21:31, Rob Clark wrote: >>>>>> On Sat, Aug 5, 2017 at 4:05 PM, Heinrich Schuchardt <xypron.g...@gmx.de> >>>>>> wrote: >>>>>>> On 08/05/2017 08:43 PM, Rob Clark wrote: >>>>>>>> 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. >>>>>>> >>>>>>> so thinking about this a bit, I have two options in mind: >>>>>>> >>>>>>> + efi_dp_sanitize() + efi_dp_free() helpers that are no-ops on >>>>>>> archs that can do unaligned access, but efi_dp_sanitize() always >>>>>>> allocates and copies on BROKEN_UNALIGNED archs and efi_dp_free() >>>>>>> always free's on BROKEN_UNALIGNED archs, even if the dp passed >>>>>>> from efi payload doesn't require it. >>>>>>> >>>>>>> + efi_dp_sanitize() that is no-op on archs that can do unaligned >>>>>>> access but only allocates/copies when passed a device path that >>>>>>> would result in unaligned access, plus hook some mechanism to >>>>>>> auto-free on EFI_EXIT().. which is slightly tricky w/ nesting of >>>>>>> efi calls, but not impossible and at least avoids the problem >>>>>>> of missing calls to free the dup'd device-path.. which is the >>>>>>> sort of leak I might miss since I'm not using EFI_LOADER on any >>>>>>> BROKEN_UNALIGNED arch >>>>>>> >>>>>>> anyone who cares about armv7 + efi have a preference between always >>>>>>> doing an extra alloc+copy+free vs EFI_EXIT() magic? >>>>>>> >>>>>> >>>>>> Please, go for the simplest code. >>>>> >>>>> well, the source of my question was there are two definitions of >>>>> "simplest" here, ie "simplest to use" and "simplest helpers".. I'm >>>>> mostly worried about future patches that use the helpers introducing >>>>> leaks. But actually I think a reasonable middle ground is "simplest >>>>> helpers plus always make the helpers not no-ops for DEBUG builds".. >>>>> >>>>>> I cannot imagine that copying takes more than 10ms for starting grub on >>>>>> any architecture. So the user will not notice it anyway. >>>>>> Assume every architecture requiring alignment for which you do not have >>>>>> proof of the contrary. >>>>>> >>>>>> Even on ARM64 there are op codes that fail without alignment. >>>>> >>>>> iirc the restrictions where mostly about device memory, and atomics. >>>>> Which should not apply here. (And we have aarch64 servers in >>>>> production with grub, which doesn't take any protections about >>>>> unaligned device-path nodes, using non u-boot EFI implementations. So >>>>> maybe that counts as proof to the contrary ;-)) >>>> >>>> The UEFI spec mandates that unaligned access are fixed up properly >>>> (usually by hardware). We violate the spec on 32bit ARM, but do fulfill >>>> it on AArch64. >>> >>> Actually for AArch32 the spec says: >>> >>> Unaligned access should be enabled if supported; Alignment faults >>> are enabled otherwise. >> >> It is a bit strange to me that alignment faults are disabled by >> default, but presumably is disabled out of reset.. I don't totally >> understand the connection to having mmu disabled, unless all memory is >> treated like device memory without mmu enabled. (Which, also, >> wouldn't that be super-slow?) But anyway, I guess that is a bit >> beside the point. > > It is treated as device memory with mmu disabled, yes. > >> >>> So UEFI payload that wants to support pre-ARMv7 hardware should make >>> sure it doesn't do any unaligned access. >>> >>>> The reason things worked before really was just that both U-Boot and >>>> grub were compiled with -mno-unaligned-access, so they never issued >>>> unaligned accesses. >>> >>> That in itself is not enough to prevent unaligned access. If you >>> explicitly cast an unaligned pointer to say (uint32_t *) and then >>> dereference it, you'll create an unaligned access. >> >> This is problematic around file nodes in the device path. Adding the >> padding bytes to the end of each device-path struct would "solve" >> that, and if pre-aarch64 we are aiming at "good enough to work", I >> kinda think that this the approach we should go for. Other than >> file-path nodes, the rest of the issues in u-boot should be solved by >> addition of missing __packed on 'struct efi_device_path' (which I've >> added locally and will be in the next revision of the patchset). > > Let's ask Leif and Ard if that is actually correct. If I remember correctly, > edk2 some times leverages automatic padding from the compiler on structs. >
in edk2's DevicePath.h it does '#pragma pack(1)' before all the device path structs (aka equiv of adding packed attribute to each struct) and then '#pragma pack()' after (disabling the packing) Also, fwiw I couldn't find anywhere in edk2 that was enabling alignment faults, but probably they have the mmu enabled and configured 1:1 similar to aarch64 in u-boot. BR, -R _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot