Re: [PATCH] Documentation: dt: chosen property for kaslr-seed

[Rob Herring]

On Mon, Jul 17, 2017 at 3:22 PM, Ard Biesheuvel
<ard.biesheuvel@xxxxxxxxxx> wrote:
> On 17 July 2017 at 20:54, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>> On Mon, Jul 17, 2017 at 12:32 PM, Rob Herring <robh@xxxxxxxxxx> wrote:
>>> On Fri, Jul 14, 2017 at 05:38:36PM -0700, Kees Cook wrote:
>>>> Document then /chosen/kaslr-seed property (and its interaction with the
>>>
>>> s/then/the/
>>>
>>>> EFI_RNG_PROTOCOL API).
>>>
>>> "dt-bindings: chosen: ..." for the subject.
>>
>> I'll send a v2 with these fixed and the Acks added.
>>
>>>> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
>>>> ---
>>>>  Documentation/devicetree/bindings/chosen.txt | 22 ++++++++++++++++++++--
>>>>  1 file changed, 20 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
>>>> index dee3f5d9df26..0cdb43b268e5 100644
>>>> --- a/Documentation/devicetree/bindings/chosen.txt
>>>> +++ b/Documentation/devicetree/bindings/chosen.txt
>>>> @@ -5,9 +5,27 @@ The chosen node does not represent a real device, but serves as a place
>>>>  for passing data between firmware and the operating system, like boot
>>>>  arguments. Data in the chosen node does not represent the hardware.
>>>>
>>>> +The following properties are recognized:
>>>>
>>>> -stdout-path property
>>>> ---------------------
>>>> +
>>>> +kaslr-seed
>>>> +-----------
>>>
>>> Is there some reason we would not feed this to other things needing
>>> entropy and therefore should have a more generic name?
>>
>> I'll let Ard answer this better than me, but IIRC, he wanted a narrow use.
>>
>
> The reason is that the seed is not used to feed a DRBG (because it is
> way to early for that when we lay out the VA space), but different
> slices of the u64 are used for randomizing different parts of the
> address space, i.e., the top 16 bits are used to randomize the linear
> region, bits 48 - 21 for the kernel itself, and the bits below that
> for the module region. (The virtual kernel offset is randomized
> further by the physical placement modulo 2 MB, but this is done by the
> stub, which calls EFI_RNG_PROTOCOL itself so it does not use
> /chosen/kaslr-seed)
>
> This means any use of this seed beyond its intended purpose may leak
> information.

Is having this value in /proc/device-tree/chosen/kaslr-seed a leak?
Should the kernel remove it?

> For UEFI systems, we do pass a random seed via a UEFI configuration
> table, but this is deliberately kept separate (and uses a UEFI
> specific interface, which seemed more appropriate)

So if we wanted a seed for non-UEFI systems, we should define a
separate property?

>>>> +This property is used when booting with CONFIG_RANDOMIZE_BASE to seed
>>>> +the entropy used to randomize the kernel image base address location. It
>>>> +is parsed as a u64 value, e.g.
>>>
>>> Why limit the size to 64-bit and why does it matter how the data is
>>> interpretted?
>>
>> IIRC, u64 is sufficient. (And note I'm just documenting what exists...)
>>
>
> See above. Seeding a DRBG typically requires more than that, but for
> KASLR it is sufficient.
>
>>>> +
>>>> +/ {
>>>> +     chosen {
>>>> +             kaslr-seed = <0xfeedbeef 0xc0def00d>;
>>>> +     };
>>>> +};
>>>> +
>>>> +Note that when booting through EFI when EFI_RNG_PROTOCOL is supported,
>>>> +this value will be overwritten by the EFI stub.
>>>
>>> Isn't this always true? The bootloader will overwrite. Just in the EFI
>>> case, the EFI stub is part of the bootloader from a functional (and not
>>> packaging) standpoint.
>>
>> I thought it best to call this out so that no one could get confused
>> if they wanted to understand how to use kaslr-seed with an EFI system.
>> (i.e. just implement EFI_RNG_PROTOCOL instead.)
>>
>
> Well, I guess the point being made is that setting this property from
> UEFI (or u-boot in EFI mode) is pointless is EFI_RNG_PROTOCOL is
> implemented as well.

Okay.

Rob