On 07/23/15 12:18, Ard Biesheuvel wrote:

(cc'ing Laszlo)

On 23 July 2015 at 11:40, Haojian Zhuang haojian.zhuang@linaro.org wrote:

...

Hi all,

There's efi_low_alloc() in $KERNEL/drivers/firmware/efi/libstub/efi-stub-helper.c.

First of all, there are few Linux kernel folk on the UEFI list that you are cc'ing.

...

I have on question on the piece of code from efi_low_alloc().

/*

• Don't allocate at 0x0. It will confuse code that
• checks pointers against NULL. Skip the first 8
• bytes so we start at a nice even number.

*/ if (start == 0x0) start += 8;

Often, memory allocation functions return NULL if the allocation failed, and there is no way to distinguish failure from a successful allocation at 0x0. Also, the C standard does not allow a valid pointer to be 0x0, since 0x0 is reserved for the NULL pointer. This means that certain compiler optimizations may result in broken code if you [legally] dereference a pointer whose value happens to be 0x0.

The C standard does not require a null pointer to have all-bits-zero representation. However, the C standard *does* say that 0x0 is a null pointer constant:

6.3.2.3 Pointers

3 An integer constant expression with the value 0, or such an expression cast to type void *, is called a null pointer constant. [footnote 55] If a null pointer constant is converted to a pointer type, the resulting pointer, called a null pointer, is guaranteed to compare unequal to a pointer to any object or function.

4 Conversion of a null pointer to another pointer type yields a null pointer of that type. Any two null pointers shall compare equal.

[footnote 55] The macro NULL is defined in <stddef.h> (and other headers) as a null pointer constant; see 7.17.

7.17 Common definitions <stddef.h>

3 The macros are

NULL

which expands to an implementation-defined null pointer constant; [...]

Looking at the efi_low_alloc() function, it does not seem to create null pointers, in the terms of the above standard excerpt, even without the "start += 8" trick. (Because no null pointer constant is to be seen anywhere.)

*However*, on these platforms that we care about, an all-bits-zero content does happen to match the representation of a genuine null pointer.

Therefore, if some code were to: - call efi_low_alloc(), - convert the "addr" output parameter (of type UL) to a variable with pointer type (*), - then later compare that pointer against NULL (probably to see if the allocation failed earlier), then that check could evaluate to true (with incorrect implications).

(*) 6.3.2.3 Pointers

5 An integer may be converted to any pointer type. Except as previously specified, the result is implementation-defined, might not be correctly aligned, might not point to an entity of the referenced type, and might be a trap representation. [footnote 56]

[footnote 56] The mapping functions for converting a pointer to an integer or an integer to a pointer are intended to be consistent with the addressing structure of the execution environment.

Meaning, when the "addr" output parameter (of type UL, with value 0) is converted to a variable with pointer type, the C standard doesn't require, but also doesn't prohibit the system from storing a bit pattern that will later compare equal to NULL.

To summarize:

struct foo *p; unsigned u;

p = 0; /* "p" is now *by definition* a null pointer, regardless of * internal representation, because "0" is a null pointer * constant. */

p = NULL; /* ditto */

p = 1 - 3 + 2; /* ditto -- (1 - 3 + 2) is an integer constant * expression with value 0. */

However,

u = 0; p = (struct foo *)u; /* It is implementation-defined what "p" is at * this point. On the platforms that we care * about, "p" is a null pointer now, and the * (p == NULL) expression will evaluate to 1. */

Now, after the last example, if the optimizer can statically deduce that (p == NULL), on a specific implementation, then (due to null pointers never pointing to *objects*, see 6.3.2.3p3 above) any

p->field

expression might not even get compiled into the program, due to it being undefined behavior.

In one sentence: in C, avoid storing an object at an address whose representation in a pointer variable happens to match the representation of a null pointer.

Thanks Laszlo

...

In handle_kernel_image() of $KERNEL/arch/arm64/kernel/efi-stub.c, it's used to allocate memory to relocate kernel to (dram_base + TEXT_OFFSET). Because of the above code, kernel has to be relocated into (dram_base

• TEXT_OFFSET + SIZE_2M).

With the above code, I'll get the "Ignoring memory" message from linux kernel. If I remove it, I won't get the message any more.

Yes, this sucks. We are losing 2 megabytes of memory here, simply because the linear mapping starts at the base of the kernel image, and Linux currently has no way to use memory below the linear mapping. (see below)

...

[ 0.000000] efi: Getting EFI parameters from FDT: [ 0.000000] efi: System Table: 0x000000003da73f18 [ 0.000000] efi: MemMap Address: 0x0000000036d07618 [ 0.000000] efi: MemMap Size: 0x000004e0 [ 0.000000] efi: MemMap Desc. Size: 0x00000030 [ 0.000000] efi: MemMap Desc. Version: 0x00000001 [ 0.000000] EFI v2.40 by Linaro HiKey EFI Jun 28 2015 21:57:01 [ 0.000000] efi: [ 0.000000] Processing EFI memory map: [ 0.000000] 0x000000000000-0x000000000fff [Conventional Memory| | | | | |WB|WT|WC|UC] [ 0.000000] Ignoring memory block 0x0 - 0x1000 [ 0.000000] [ 0.000000] 0x000000001000-0x000000001fff [Loader Data | | | | | |WB|WT|WC|UC]

^^ this is an allocation that is done /after/ the allocation of the kernel image

...

[ 0.000000] Ignoring memory block 0x1000 - 0x2000 [ 0.000000] [ 0.000000] 0x000000002000-0x0000001fffff [Conventional Memory| | | | | |WB|WT|WC|UC] [ 0.000000] Ignoring memory range 0x2000 - 0x200000 [ 0.000000] [ 0.000000] 0x000000200000-0x000000e6bfff [Loader Data | | | | | |WB|WT|WC|UC]

^^^ this is the kernel mapping

...

So let's review the code again, we'll always check the return status value of efi_low_alloc() in kernel. Why do we need to avoid the 0x0 address? I think we could remove the piece of code.

Yes, perhaps it is a matter of redefining the return value of efi_low_alloc() so we can defer the failure handling to the caller.