[U-Boot] [PATCH v0 13/20] efi_loader: use proper device-paths for partitions

[Mark Kettenis]

> From: Rob Clark <robdclark at gmail.com>
> Date: Sat, 5 Aug 2017 11:36:25 -0400
> 
> On Sat, Aug 5, 2017 at 11:24 AM, Rob Clark <robdclark at gmail.com> wrote:
> > On Sat, Aug 5, 2017 at 11:10 AM, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >> On 08/05/2017 04:35 PM, Rob Clark wrote:
> >>> On Sat, Aug 5, 2017 at 10:28 AM, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
> >>>>> Date: Sat, 5 Aug 2017 16:01:51 +0200 (CEST)
> >>>>> From: Mark Kettenis <mark.kettenis at xs4all.nl>
> >>>>>
> >>>>> Unfortunately something in this patch series breaks things for me on a
> >>>>> Banana Pi:
> >>>>
> >>>> And according to git bisect:
> >>>>
> >>>> 4e3e748a50fc3f43e20c7ff407184596d7c9a589 is the first bad commit
> >>>> commit 4e3e748a50fc3f43e20c7ff407184596d7c9a589
> >>>> Author: Peter Jones <pjones at redhat.com>
> >>>> Date:   Wed Jun 21 16:39:02 2017 -0400
> >>>>
> >>>>     efi: add some more device path structures
> >>>>
> >>>>     Signed-off-by: Peter Jones <pjones at redhat.com>
> >>>>     Signed-off-by: Rob Clark <robdclark at gmail.com>
> >>>
> >>>
> >>> hmm, odd.. it is only adding some #define's and structs that are not
> >>> used until a later commit..
> >>>
> >>> although it does also make 'struct efi_device_path_mac_addr' __packed,
> >>> which it should have been before.  Is this an armv7?  I wonder if we
> >>> have some troubles with unaligned accesses on armv7 that we don't have
> >>> on aarch64 (or maybe compiler isn't turning access to device-path
> >>> nodes into byte accesses if it can't do unaligned accesses.  (The node
> >>> in the device-path structure are byte-packed.)
> >>
> >> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.faqs/ka15414.html
> >>
> >> <cite>On older processors, such as ARM9 family based processors, an
> >> unaligned load had to be synthesised in software.  Typically by doing a
> >> series of small accesses, and combining the results. ... Unaligned
> >> access support must be enabled by setting the SCTLR.A bit in the system
> >> control coprocessor</cite>
> >>
> >> Generally packing structures is not a good idea performance-wise. The
> >> sequence of fields should be carefully chosen to fill up to powers of
> >> two (2, 4 , 8).
> >
> > Yeah, it was clearly a dumb idea for UEFI to not make device-path
> > nodes word aligned.  But when implementing a standard, we don't have a
> > choice but to implement it properly, warts and all :-/
> >
> 
> btw, just for reference (if anyone is curious), see sect 10.3.1 in
> UEFI spec v2.7.  If you don't want to bother looking it up, here is
> the exact wording:
> 
>    A Device Path is a series of generic Device Path nodes. The first
>    Device Path node starts at byte offset zero of the Device Path.
>    The next Device Path node starts at the end of the previous Device
>    Path node. Therefore all nodes are byte-packed data structures that
>    may appear on any byte boundary. All code references to device path
>    notes must assume all fields are unaligned. Since every Device Path
>    node contains a length field in a known place, it is possible to
>    traverse Device Path nodes that are of an unknown type. There is
>    no limit to the number, type, or sequence of nodes in a Device Path.
> 
> So clearly what we were doing before was incorrect.. but cheating w/
> extra padding bytes on arch's that cannot handle unaligned accesses
> will avoid having to go fix grub, bootaa64/shim/fallback (and anything
> else that uses gnu-efi), and apparently openbsd's bootaa64.efi.  It is
> kinda weird to be using efi on these arch's in the first place, so I
> guess I don't feel as badly about the padding bytes hack on those
> arch's.  (But we should not use the hack on aarch64.)

Looking a bit more closely at the OpenBSD efiboot code, I'm pretty
sure we handle the parsing of device path nodes correctly.  We use an
incarnation of the Intel EFI header files which have:

typedef struct _EFI_DEVICE_PATH {
        UINT8                           Type;
        UINT8                           SubType;
        UINT8                           Length[2];
} EFI_DEVICE_PATH;

#define DevicePathNodeLength(a)     ( ((a)->Length[0]) | ((a)->Length[1] << 8) )

so this is all done using byte access.

Going back to the original crash report:

data abort
pc : [<7ef8d878>]          lr : [<7ef8d874>]
reloc pc : [<4a039878>]    lr : [<4a039874>]
sp : 7af29660  ip : 00000000     fp : 7af29774
r10: 7efec230  r9 : 7af33ee0     r8 : 00000000
r7 : 00000009  r6 : 7ef9e8b8     r5 : 7af296a0  r4 : 7efa4495
r3 : 7af296a0  r2 : 0000008c     r1 : 7af29658  r0 : 00000004
Flags: nzCV  IRQs off  FIQs off  Mode SVC_32

I think it is actually "reloc pc" instead of "pc" that we need to look
at:

$ addr2line -e u-boot 0x4a039874 
/home/kettenis/tmp/rclark/u-boot/include/efi_loader.h:272

which points at the guidstr() function.  That code certainly looks
suspicious.  It will defenitely trigger alignment faults if the guid
isn't 32-bit aligned.

The relevant instruction is a 16-bit load:

  4a039878:       e1d430b4        ldrh    r3, [r4, #4]

and with r4 = 7efa4495 that will defenitely trap.

Looking at the defenition efi_guid_t in u-boot:

  typedef struct {
          u8 b[16];
  } efi_guid_t;

there is no guarantee that GUIDs are properly aligned, so you'll need
to fix the guidstr function introduced in commit
b6d913c6101ba891eb2bcb08a4ee9fc8fb57367.

Things are already broken before that commit though, so there is
another problem.  I'll see if I can figure out what it is...