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.

BR,
-R

> Best regards
>
> Heinrich
>
>>
>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>>>
>>>>  ifdef CONFIG_ARM64
>>>>  PLATFORM_ELFFLAGS += -B aarch64 -O elf64-littleaarch64
>>>> diff --git a/include/efi_api.h b/include/efi_api.h
>>>> index ef91e34c7b..ddd1e6100a 100644
>>>> --- a/include/efi_api.h
>>>> +++ b/include/efi_api.h
>>>> @@ -284,6 +284,31 @@ struct efi_loaded_image {
>>>>  #define DEVICE_PATH_TYPE_END                 0x7f
>>>>  #  define DEVICE_PATH_SUB_TYPE_END           0xff
>>>>
>>>> +/*
>>>> + * 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.
>>>> + */
>>>> +
>>>> +#ifdef BROKEN_UNALIGNED
>>>> +#  define EFI_DP_PAD(n)  u8 __pad[n]
>>>> +#else
>>>> +#  define EFI_DP_PAD(n)
>>>> +#endif
>>>> +
>>>>  struct efi_device_path {
>>>>       u8 type;
>>>>       u8 sub_type;
>>>> @@ -318,12 +343,14 @@ struct efi_device_path_usb {
>>>>       struct efi_device_path dp;
>>>>       u8 parent_port_number;
>>>>       u8 usb_interface;
>>>> +     EFI_DP_PAD(2);
>>>>  } __packed;
>>>>
>>>>  struct efi_device_path_mac_addr {
>>>>       struct efi_device_path dp;
>>>>       struct efi_mac_addr mac;
>>>>       u8 if_type;
>>>> +     EFI_DP_PAD(3);
>>>>  } __packed;
>>>>
>>>>  struct efi_device_path_usb_class {
>>>> @@ -333,11 +360,13 @@ struct efi_device_path_usb_class {
>>>>       u8 device_class;
>>>>       u8 device_subclass;
>>>>       u8 device_protocol;
>>>> +     EFI_DP_PAD(1);
>>>>  } __packed;
>>>>
>>>>  struct efi_device_path_sd_mmc_path {
>>>>       struct efi_device_path dp;
>>>>       u8 slot_number;
>>>> +     EFI_DP_PAD(3);
>>>>  } __packed;
>>>>
>>>>  #define DEVICE_PATH_TYPE_MEDIA_DEVICE                0x04
>>>> @@ -353,6 +382,7 @@ struct efi_device_path_hard_drive_path {
>>>>       u8 partition_signature[16];
>>>>       u8 partmap_type;
>>>>       u8 signature_type;
>>>> +     EFI_DP_PAD(1);
>>>>  } __packed;
>>>>
>>>>  struct efi_device_path_cdrom_path {
>>>> diff --git a/lib/efi_loader/efi_device_path.c 
>>>> b/lib/efi_loader/efi_device_path.c
>>>> index b5acf73f98..515a1f4737 100644
>>>> --- a/lib/efi_loader/efi_device_path.c
>>>> +++ b/lib/efi_loader/efi_device_path.c
>>>> @@ -402,6 +402,9 @@ struct efi_device_path *efi_dp_from_file(struct 
>>>> blk_desc *desc, int part,
>>>>
>>>>       // TODO efi_device_path_file_path should be variable length:
>>>>       fpsize = sizeof(struct efi_device_path) + 2 * (strlen(path) + 1);
>>>> +#ifdef BROKEN_UNALIGNED
>>>> +     fpsize = ALIGN(fpsize, 4);
>>>> +#endif
>>>>       dpsize += fpsize;
>>>>
>>>>       start = buf = calloc(1, dpsize + sizeof(END));
>>>>
>>>
>>
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to