[U-Boot] [PATCH v0 21/20] efi_loader: hack for archs that cannot do unaligned accesses
Rob Clark
robdclark at gmail.com
Sat Aug 5 18:43:15 UTC 2017
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
>
>> 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));
>>>>>
>>>>
>>>
>>
More information about the U-Boot
mailing list