Re: [PATCH] hw/arm/virt: Add DT property /secure-chosen/kaslr-seed

[Pavel Dovgalyuk <dovgaluk@ispras.ru>]

On 17.04.2020 13:18, Peter Maydell wrote:
> On Fri, 10 Apr 2020 at 18:02, Jerome Forissier <jerome@forissier.org> wrote:
>> This commit generates a random seed to be used by the secure OS for
>> ASLR when the machine is secure. The seed is a 64-bit random value
>> exported via the DT in /secure-chosen/kaslr-seed. This interface is
>> used by OP-TEE [1].
>>
>> [1] https://github.com/OP-TEE/optee_os/commit/ef262691fe0e
> The kernel devicetree documentation documents this as a generic
> property of /chosen -- should we be providing a /chosen/kaslr-seed
> too ?
>
>> Signed-off-by: Jerome Forissier <jerome@forissier.org>
>> ---
>> +static void create_secure_kaslr_seed(VirtMachineState *vms)
>> +{
>> +    Error *err = NULL;
>> +    uint64_t seed;
>> +
>> +    if (qcrypto_random_bytes(&seed, sizeof(seed), &err)) {
>> +        error_free(err);
>> +        return;
>> +    }
> Since this is exposed to the guest I'm wondering if we should
> use qemu_guest_getrandom() (which lets you make the randomness
> deterministic for the benefit of record-and-replay). But I'm
> not sure if that function is usable before the guest has even
> started running. Pavel, could you answer that?

Yes, usage of deterministic functions is possible before machine is running,

because replay_configure is executed before machine initialization.


>
>> +    qemu_fdt_setprop_u64(vms->fdt, "/secure-chosen", "kaslr-seed", seed);
>> +}
>> +
>>   static void machvirt_init(MachineState *machine)
>>   {
>>       VirtMachineState *vms = VIRT_MACHINE(machine);
>> @@ -1837,6 +1850,7 @@ static void machvirt_init(MachineState *machine)
>>       if (vms->secure) {
>>           create_secure_ram(vms, secure_sysmem);
>>           create_uart(vms, VIRT_SECURE_UART, secure_sysmem, serial_hd(1));
>> +        create_secure_kaslr_seed(vms);
> This is implicitly relying on create_uart() having created
> the "/secure-chosen" node. I think it would be better now
> that we have multiple things we might want to put there if we
> just pulled the "create /secure-chosen" code out to the
> create_fdt() function so we do it at the same place we
> create "/chosen". (You can do that as a simple patch of its own
> that comes before this one in the patchseries.)
>
>>       }
>>
>>       vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64);
> thanks
> -- PMM