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

Mon Aug 7 21:14:58 UTC 2017

> From: Alexander Graf <agraf at 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.glpk at gmx.de> wrote:
> >> On 08/05/2017 08:43 PM, Rob Clark wrote:
> >>> 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.
> >>>
> >>> 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.

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.

> Which system broke for Mark? Banana pi mostly has 32bit SBCs, so I 
> assume it's one of those? In that case, we would need to either
> 
>    1) hack up the openbsd loader to also get compiled with 
> -mno-unaligned-access
> 
> or
> 
>    2) rework the 32bit arm mmu code to enable the MMU plus hardware 
> unaligned fixups on all systems. (that's a *lot* of work)

The system that broke was Cortex-A7 based, so ARMv7.

*However*, the OpenBSD bootloader does not perform any unaligned
access.  *All* cases of unaligned access that I encountered while
testing/debugging Rob's code were in U-Boot itself.  This was mostly a
consequence from a diff in the series that added __packed to the
device path node structs and then passed members of those structs to
functions that accessed the contents of tose (now byte aligned
structs) through (uint16_t *) or (uint32_t *) pointers.

There was initially some confusion because I used the pc reported by
U-Boot to look up the faulting instruction whereas I should have been
using the reloc_pc.

I think Rob is worried about grub/shim/fallback.efi doing unaligned
access of device path nodes on armv7.  But I'm not sure he has any
evidence beyond looking at code.

> To me personally, UEFI in 32bit ARM land is a nice to have standardized 
> platform, but not something where we really need to be fully standards 
> compliant in every aspect. I think "making things work" is good enough 
> there.

Right.  For us UEFI us just a convenient way to re-use the U-Boot
device driver code to load a kernel from a UFS/FFS filesystem.

> For AArch64 things are different. There we should strive for full UEFI 
> compliance as it's "the future" :).

Even there just making things work would be good enough for me ;).
Our AArach64 bootloader is almost identical to the AArch32 one and
works on a real UEFI implementation as well (SoftIron Overdrive 1000).

Cheers,

Mark