[U-Boot] [PATCH v0 21/20] efi_loader: hack for archs that cannot do unaligned accesses
Heinrich Schuchardt
xypron.glpk at gmx.de
Sat Aug 5 20:05:19 UTC 2017
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?
>
> BR,
> -R
>
>
>> 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
>>
Please, go for the simplest code.
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.
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));
>>>>>>
>>>>>
>>>>
>>>
>
More information about the U-Boot
mailing list