| Message ID | 1542372182-26682-2-git-send-email-sumit.garg@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] libtomcrypt: Import SHA512/256 approved hash algorithm | expand |
Hi Daniel, Please help to review below patch. I have added approved hash algo (SHA512/256) for conditioning and changes health test to check bit-flips instead as per our discussion. On Fri, 16 Nov 2018 at 18:13, Sumit Garg <sumit.garg@linaro.org> wrote: > > This platform provides 7 on-chip thermal sensors accessible from secure > world only. So, using randomness from these sensors we have created a True > RNG as a pseudo TA. > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > --- > core/arch/arm/include/arm64.h | 4 + > core/arch/arm/plat-synquacer/main.c | 38 +++- > core/arch/arm/plat-synquacer/platform_config.h | 3 + > core/arch/arm/plat-synquacer/rng_pta.c | 270 +++++++++++++++++++++++++ > core/arch/arm/plat-synquacer/rng_pta.h | 37 ++++ > core/arch/arm/plat-synquacer/rng_pta_client.h | 22 ++ > core/arch/arm/plat-synquacer/sub.mk | 2 + > core/arch/arm/plat-synquacer/timer_fiq.c | 43 ++++ > 8 files changed, 414 insertions(+), 5 deletions(-) > create mode 100644 core/arch/arm/plat-synquacer/rng_pta.c > create mode 100644 core/arch/arm/plat-synquacer/rng_pta.h > create mode 100644 core/arch/arm/plat-synquacer/rng_pta_client.h > create mode 100644 core/arch/arm/plat-synquacer/timer_fiq.c > > diff --git a/core/arch/arm/include/arm64.h b/core/arch/arm/include/arm64.h > index 2c1fd8c..0cf14c0 100644 > --- a/core/arch/arm/include/arm64.h > +++ b/core/arch/arm/include/arm64.h > @@ -305,6 +305,10 @@ DEFINE_REG_READ_FUNC_(cntfrq, uint32_t, cntfrq_el0) > DEFINE_REG_READ_FUNC_(cntpct, uint64_t, cntpct_el0) > DEFINE_REG_READ_FUNC_(cntkctl, uint32_t, cntkctl_el1) > DEFINE_REG_WRITE_FUNC_(cntkctl, uint32_t, cntkctl_el1) > +DEFINE_REG_READ_FUNC_(cntps_ctl, uint32_t, cntps_ctl_el1) > +DEFINE_REG_WRITE_FUNC_(cntps_ctl, uint32_t, cntps_ctl_el1) > +DEFINE_REG_READ_FUNC_(cntps_cval, uint32_t, cntps_cval_el1) > +DEFINE_REG_WRITE_FUNC_(cntps_cval, uint32_t, cntps_cval_el1) > > DEFINE_REG_READ_FUNC_(pmccntr, uint64_t, pmccntr_el0) > > diff --git a/core/arch/arm/plat-synquacer/main.c b/core/arch/arm/plat-synquacer/main.c > index c3aac4c..7c3a6bc 100644 > --- a/core/arch/arm/plat-synquacer/main.c > +++ b/core/arch/arm/plat-synquacer/main.c > @@ -18,6 +18,7 @@ > #include <sm/optee_smc.h> > #include <tee/entry_fast.h> > #include <tee/entry_std.h> > +#include <rng_pta.h> > > static void main_fiq(void); > > @@ -38,6 +39,7 @@ static struct pl011_data console_data; > > register_phys_mem(MEM_AREA_IO_NSEC, CONSOLE_UART_BASE, CORE_MMU_DEVICE_SIZE); > register_phys_mem(MEM_AREA_IO_SEC, GIC_BASE, CORE_MMU_DEVICE_SIZE); > +register_phys_mem(MEM_AREA_IO_SEC, THERMAL_SENSOR_BASE, CORE_MMU_DEVICE_SIZE); > > const struct thread_handlers *generic_boot_get_handlers(void) > { > @@ -46,7 +48,7 @@ const struct thread_handlers *generic_boot_get_handlers(void) > > static void main_fiq(void) > { > - panic(); > + gic_it_handle(&gic_data); > } > > void console_init(void) > @@ -66,12 +68,38 @@ void main_init_gic(void) > if (!gicd_base) > panic(); > > - /* Initialize GIC */ > - gic_init(&gic_data, 0, gicd_base); > + /* On ARMv8-A, GIC configuration is initialized in TF-A */ > + gic_init_base_addr(&gic_data, 0, gicd_base); > + > itr_init(&gic_data.chip); > } > > -void main_secondary_init_gic(void) > +static enum itr_return timer_itr_cb(struct itr_handler *h __unused) > +{ > + /* Reset timer for next FIQ */ > + generic_timer_handler(); > + > + /* Collect entropy on each timer FIQ */ > + rng_collect_entropy(); > + > + return ITRR_HANDLED; > +} > + > +static struct itr_handler timer_itr = { > + .it = IT_SEC_TIMER, > + .flags = ITRF_TRIGGER_LEVEL, > + .handler = timer_itr_cb, > +}; > + > +static TEE_Result init_timer_itr(void) > { > - gic_cpu_init(&gic_data); > + itr_add(&timer_itr); > + itr_enable(IT_SEC_TIMER); > + > + /* Enable timer FIQ to fetch entropy required during boot */ > + generic_timer_start(); > + timer_fiq_running = true; Here I did enabled timer FIQs during OP-TEE init to reduce waiting time for edk2 to fetch initial seed. > + > + return TEE_SUCCESS; > } > +driver_init(init_timer_itr); > diff --git a/core/arch/arm/plat-synquacer/platform_config.h b/core/arch/arm/plat-synquacer/platform_config.h > index 4d6d545..8a91ddb 100644 > --- a/core/arch/arm/plat-synquacer/platform_config.h > +++ b/core/arch/arm/plat-synquacer/platform_config.h > @@ -19,6 +19,9 @@ > #define CONSOLE_UART_CLK_IN_HZ 62500000 > #define CONSOLE_BAUDRATE 115200 > > +#define THERMAL_SENSOR_BASE 0x54190000 > +#define IT_SEC_TIMER 29 > + > #define DRAM0_BASE 0x80000000 > > /* Platform specific defines */ > diff --git a/core/arch/arm/plat-synquacer/rng_pta.c b/core/arch/arm/plat-synquacer/rng_pta.c > new file mode 100644 > index 0000000..a760b54 > --- /dev/null > +++ b/core/arch/arm/plat-synquacer/rng_pta.c > @@ -0,0 +1,270 @@ > +// SPDX-License-Identifier: BSD-2-Clause > +/* > + * Copyright (C) 2018, Linaro Limited > + */ > + > +#include <crypto/crypto.h> > +#include <kernel/delay.h> > +#include <kernel/pseudo_ta.h> > +#include <kernel/spinlock.h> > +#include <mm/core_memprot.h> > +#include <io.h> > +#include <string.h> > +#include <rng_pta.h> > +#include <rng_pta_client.h> > + > +static uint8_t entropy_pool[ENTROPY_POOL_SIZE] = {0}; > +static uint32_t entropy_size; > + > +/* Current sensor data */ > +static uint8_t sensor_data[NUM_OF_SENSORS][SENSOR_DATA_SIZE] = {0}; > +static uint8_t s; > + > +/* Sensor data that passed health test */ > +static uint8_t sensor_data_pass[NUM_OF_SENSORS][SENSOR_DATA_SIZE] = {0}; > +static uint8_t num_sensors_pass; > + > +static uint8_t rest_data_pass[NUM_OF_SENSORS][SENSOR_DATA_SIZE] = {0}; > + > +static uint32_t health_test_fail_cnt; > + > +static unsigned int entropy_lock = SPINLOCK_UNLOCK; > + > +static void pool_add_entropy(uint8_t *entropy, uint32_t size) > +{ > + uint32_t copy_size; > + > + if (entropy_size >= ENTROPY_POOL_SIZE) > + return; > + > + if ((ENTROPY_POOL_SIZE - entropy_size) >= size) > + copy_size = size; > + else > + copy_size = ENTROPY_POOL_SIZE - entropy_size; > + > + memcpy((entropy_pool + entropy_size), entropy, copy_size); > + > + entropy_size += copy_size; > +} > + > +static void pool_get_entropy(uint8_t *buf, uint32_t size) > +{ > + uint32_t off; > + > + if (size > entropy_size) > + return; > + > + off = entropy_size - size; > + > + memcpy(buf, &entropy_pool[off], size); > + entropy_size -= size; > +} > + > +static bool health_test(uint8_t sensor_id) > +{ > + bool result = true; > + uint8_t bit_flip_1_0 = 0, bit_flip_0_1 = 0; > + uint8_t i; > + > + /* > + * Heatlh test to check if count of bit flips 1-0 or 0-1 lies in 12.5% > + * to 37.5% of 128 bytes raw data from particular sensor reading. In > + * ideal scenario either of bit flips should be around 25%. > + */ Results from this health tests are as follows: Only 18 failures observed out of 101850 times run on 128 bytes raw data from particular sensor reading. So it shows approximately 99.98% success ratio. Please share your views regarding 12.5% and 37.5% ratios. We could even take more conservative approach if you think so. > + for (i = 0; i < (SENSOR_DATA_SIZE - 1); i++) { > + if ((sensor_data[sensor_id][i] ^ > + sensor_data[sensor_id][i + 1]) & 0x1) { > + bit_flip_1_0 += sensor_data[sensor_id][i] & 0x1; > + bit_flip_0_1 += sensor_data[sensor_id][i + 1] & 0x1; > + } > + } > + > + if (bit_flip_1_0 > bit_flip_0_1) { > + if (bit_flip_0_1 < MIN_BIT_FLIP_COUNT) > + result = false; > + if (bit_flip_1_0 > MAX_BIT_FLIP_COUNT) > + result = false; > + } else { > + if (bit_flip_1_0 < MIN_BIT_FLIP_COUNT) > + result = false; > + if (bit_flip_0_1 > MAX_BIT_FLIP_COUNT) > + result = false; > + } > + > + return result; > +} > + > +static void pool_check_add_entropy(void) > +{ > + uint32_t i; > + uint8_t entropy_sha512_256[TEE_SHA256_HASH_SIZE]; > + uint8_t rest_sensors_pass = 0; > + TEE_Result res; > + > + for (i = 0; i < NUM_OF_SENSORS; i++) { > + /* Check if particular sensor data passes health test */ > + if (health_test(i) == true) { > + if (num_sensors_pass < NUM_OF_SENSORS) { > + memcpy(sensor_data_pass[num_sensors_pass], > + sensor_data[i], SENSOR_DATA_SIZE); > + num_sensors_pass++; > + } else { > + memcpy(rest_data_pass[rest_sensors_pass], > + sensor_data[i], SENSOR_DATA_SIZE); > + rest_sensors_pass++; > + } > + } else { > + health_test_fail_cnt++; > + /* > + * Report health test failures if it exceeds certain > + * threshold. > + */ > + if (health_test_fail_cnt >= THRESHOLD_REPORT_FAILURE) { > + IMSG("Health test failed %d times\n", > + health_test_fail_cnt); > + health_test_fail_cnt = 0; I choose to report failures of health test when it exceeds particular threshold (currently its 10) via IMSG uart prints. Please share you views regarding this approach. -Sumit > + } > + } > + } > + > + /* Check if sensor_data_pass is full */ > + if (num_sensors_pass == NUM_OF_SENSORS) { > + /* > + * Use vetted conditioner SHA512/256 as per > + * NIST.SP.800-90B to condition raw data from entropy > + * source. > + * Here we have assumed that entropy source provides > + * 4 bits per 7 sensor readings per sample. > + * Also as per NIST.SP.800-90B, to get full entropy > + * from vetted conditioner, we need to supply double of > + * input entropy. So with full entropy (8 bits per byte) > + * we will get yield as one byte of output data for > + * every 28 sensor readings. > + * For 32 bytes of SHA512/256 output data, we need to > + * supply 896 bytes of raw input data. > + */ > + res = hash_sha512_256_compute(entropy_sha512_256, > + (uint8_t *)sensor_data_pass, > + CONDITIONER_PAYLOAD); > + if (res == TEE_SUCCESS) > + pool_add_entropy(entropy_sha512_256, > + TEE_SHA256_HASH_SIZE); > + } > + > + if (rest_sensors_pass) > + memcpy((uint8_t *)sensor_data_pass, (uint8_t *)rest_data_pass, > + (rest_sensors_pass * SENSOR_DATA_SIZE)); > + > + num_sensors_pass = rest_sensors_pass; > +} > + > +void rng_collect_entropy(void) > +{ > + uint8_t i; > + void *vaddr; > + uint32_t exceptions = thread_mask_exceptions(THREAD_EXCP_ALL); > + > + cpu_spin_lock(&entropy_lock); > + > + for (i = 0; i < NUM_OF_SENSORS; i++) { > + vaddr = phys_to_virt_io(THERMAL_SENSOR_BASE0 + > + (THERMAL_SENSOR_OFFSET * i) + > + TEMP_DATA_REG_OFFSET); > + sensor_data[i][s] = (uint8_t)read32((vaddr_t)vaddr); > + } > + > + s++; > + > + if (s >= SENSOR_DATA_SIZE) { > + pool_check_add_entropy(); > + s = 0; > + } > + > + if (entropy_size >= ENTROPY_POOL_SIZE) { > + generic_timer_stop(); > + timer_fiq_running = false; > + } > + > + cpu_spin_unlock(&entropy_lock); > + thread_set_exceptions(exceptions); > +} > + > +static TEE_Result rng_get_entropy(uint32_t types, > + TEE_Param params[TEE_NUM_PARAMS]) > +{ > + uint8_t *e = NULL; > + uint32_t pool_size = 0, rq_size = 0; > + uint32_t exceptions = thread_mask_exceptions(THREAD_EXCP_ALL); > + > + cpu_spin_lock(&entropy_lock); > + > + if (types != TEE_PARAM_TYPES(TEE_PARAM_TYPE_MEMREF_INOUT, > + TEE_PARAM_TYPE_NONE, > + TEE_PARAM_TYPE_NONE, > + TEE_PARAM_TYPE_NONE)) { > + EMSG("bad parameters types: 0x%" PRIx32, types); > + return TEE_ERROR_BAD_PARAMETERS; > + } > + > + rq_size = params[0].memref.size; > + > + if ((rq_size == 0) || (rq_size > ENTROPY_POOL_SIZE)) > + return TEE_ERROR_NOT_SUPPORTED; > + > + e = (uint8_t *)params[0].memref.buffer; > + if (!e) > + return TEE_ERROR_BAD_PARAMETERS; > + > + pool_size = entropy_size; > + > + if (pool_size < rq_size) { > + params[0].memref.size = pool_size; > + pool_get_entropy(e, pool_size); > + } else { > + params[0].memref.size = rq_size; > + pool_get_entropy(e, rq_size); > + } > + > + if (timer_fiq_running == false) { > + /* Enable timer FIQ to fetch entropy */ > + generic_timer_start(); > + timer_fiq_running = true; > + } > + > + cpu_spin_unlock(&entropy_lock); > + thread_set_exceptions(exceptions); > + > + return TEE_SUCCESS; > +} > + > +/* > + * Trusted Application Entry Points > + */ > +static TEE_Result open_session(uint32_t param_types __unused, > + TEE_Param params[TEE_NUM_PARAMS] __unused, > + void **session_context __unused) > +{ > + DMSG("open entry point for pseudo-TA \"%s\"", PTA_NAME); > + return TEE_SUCCESS; > +} > + > +static TEE_Result invoke_command(void *pSessionContext __unused, > + uint32_t nCommandID, uint32_t nParamTypes, > + TEE_Param pParams[TEE_NUM_PARAMS]) > +{ > + FMSG("command entry point for pseudo-TA \"%s\"", PTA_NAME); > + > + switch (nCommandID) { > + case PTA_CMD_GET_ENTROPY: > + return rng_get_entropy(nParamTypes, pParams); > + default: > + break; > + } > + > + return TEE_ERROR_NOT_IMPLEMENTED; > +} > + > +pseudo_ta_register(.uuid = PTA_RNG_UUID, .name = PTA_NAME, > + .flags = PTA_DEFAULT_FLAGS, > + .open_session_entry_point = open_session, > + .invoke_command_entry_point = invoke_command); > diff --git a/core/arch/arm/plat-synquacer/rng_pta.h b/core/arch/arm/plat-synquacer/rng_pta.h > new file mode 100644 > index 0000000..e238c57 > --- /dev/null > +++ b/core/arch/arm/plat-synquacer/rng_pta.h > @@ -0,0 +1,37 @@ > +/* SPDX-License-Identifier: BSD-2-Clause */ > +/* > + * Copyright (C) 2018, Linaro Limited > + */ > + > +#ifndef __RNG_PTA_H > +#define __RNG_PTA_H > + > +#define PTA_NAME "rng.pta" > + > +#define THERMAL_SENSOR_BASE0 0x54190800 > +#define THERMAL_SENSOR_OFFSET 0x80 > +#define NUM_OF_SENSORS 7 > + > +#define TEMP_DATA_REG_OFFSET 0x34 > + > +#define ENTROPY_POOL_SIZE 4096 > + > +#define SENSOR_DATA_SIZE 128 > +#define CONDITIONER_PAYLOAD (SENSOR_DATA_SIZE * NUM_OF_SENSORS) > + > +#define LSB_MASK 0x1 > + > +#define MAX_BIT_FLIP_COUNT 48 > +#define MIN_BIT_FLIP_COUNT 16 > + > +#define THRESHOLD_REPORT_FAILURE 10 > + > +extern bool timer_fiq_running; > + > +void rng_collect_entropy(void); > + > +void generic_timer_start(void); > +void generic_timer_stop(void); > +void generic_timer_handler(void); > + > +#endif /* __RNG_PTA_H */ > diff --git a/core/arch/arm/plat-synquacer/rng_pta_client.h b/core/arch/arm/plat-synquacer/rng_pta_client.h > new file mode 100644 > index 0000000..ddd398c > --- /dev/null > +++ b/core/arch/arm/plat-synquacer/rng_pta_client.h > @@ -0,0 +1,22 @@ > +/* SPDX-License-Identifier: BSD-2-Clause */ > +/* > + * Copyright (C) 2018, Linaro Limited > + */ > + > +#ifndef __RNG_PTA_CLIENT_H > +#define __RNG_PTA_CLIENT_H > + > +#define PTA_RNG_UUID { 0xab7a617c, 0xb8e7, 0x4d8f, \ > + { 0x83, 0x01, 0xd0, 0x9b, 0x61, 0x03, 0x6b, 0x64 } } > + > +/* > + * PTA_CMD_GET_ENTROPY - Get Entropy from RNG using Thermal Sensor > + * > + * param[0] (inout memref) - Entropy buffer memory reference > + * param[1] unused > + * param[2] unused > + * param[3] unused > + */ > +#define PTA_CMD_GET_ENTROPY 0x0 > + > +#endif /* __RNG_PTA_CLIENT_H */ > diff --git a/core/arch/arm/plat-synquacer/sub.mk b/core/arch/arm/plat-synquacer/sub.mk > index 8ddc2fd..013e57d 100644 > --- a/core/arch/arm/plat-synquacer/sub.mk > +++ b/core/arch/arm/plat-synquacer/sub.mk > @@ -1,2 +1,4 @@ > global-incdirs-y += . > srcs-y += main.c > +srcs-y += rng_pta.c > +srcs-y += timer_fiq.c > diff --git a/core/arch/arm/plat-synquacer/timer_fiq.c b/core/arch/arm/plat-synquacer/timer_fiq.c > new file mode 100644 > index 0000000..e25a676 > --- /dev/null > +++ b/core/arch/arm/plat-synquacer/timer_fiq.c > @@ -0,0 +1,43 @@ > +// SPDX-License-Identifier: BSD-2-Clause > +/* > + * Copyright (c) 2018, Linaro Limited > + */ > + > +#include <arm.h> > +#include <console.h> > +#include <drivers/gic.h> > +#include <io.h> > +#include <kernel/panic.h> > +#include <kernel/misc.h> > +#include <rng_pta.h> > + > +bool timer_fiq_running = false; > + > +void generic_timer_start(void) > +{ > + uint64_t cval; > + uint32_t ctl = 1; > + > + /* The timer will fire every 2 ms */ > + cval = read_cntpct() + (read_cntfrq() / 500); > + write_cntps_cval(cval); > + > + /* Enable the secure physical timer */ > + write_cntps_ctl(ctl); > +} > + > +void generic_timer_stop(void) > +{ > + /* Disable the timer */ > + write_cntps_ctl(0); > +} > + > +void generic_timer_handler(void) > +{ > + /* Ensure that the timer did assert the interrupt */ > + assert((read_cntps_ctl() >> 2)); > + > + /* Disable the timer and reprogram it */ > + write_cntps_ctl(0); > + generic_timer_start(); > +} > -- > 2.7.4 >
On Fri, Nov 16, 2018 at 06:13:02PM +0530, Sumit Garg wrote: > This platform provides 7 on-chip thermal sensors accessible from secure > world only. So, using randomness from these sensors we have created a True > RNG as a pseudo TA. > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > --- > core/arch/arm/include/arm64.h | 4 + > core/arch/arm/plat-synquacer/main.c | 38 +++- > core/arch/arm/plat-synquacer/platform_config.h | 3 + > core/arch/arm/plat-synquacer/rng_pta.c | 270 +++++++++++++++++++++++++ > core/arch/arm/plat-synquacer/rng_pta.h | 37 ++++ > core/arch/arm/plat-synquacer/rng_pta_client.h | 22 ++ > core/arch/arm/plat-synquacer/sub.mk | 2 + > core/arch/arm/plat-synquacer/timer_fiq.c | 43 ++++ Feels like this could be split apart into a timer FIQ patch and a seperate RNG PTA patch (later patch would have to add call to rng_collect_entropy() but other than this the code appears to seperate cleanly. > diff --git a/core/arch/arm/plat-synquacer/rng_pta.c b/core/arch/arm/plat-synquacer/rng_pta.c > new file mode 100644 > index 0000000..a760b54 > --- /dev/null > +++ b/core/arch/arm/plat-synquacer/rng_pta.c > @@ -0,0 +1,270 @@ > +// SPDX-License-Identifier: BSD-2-Clause > +/* > + * Copyright (C) 2018, Linaro Limited > + */ Shouldn't the theory of operation, limitations, testing, entropy estimates etc. go here? (and if not here then where?) > + > +#include <crypto/crypto.h> > +#include <kernel/delay.h> > +#include <kernel/pseudo_ta.h> > +#include <kernel/spinlock.h> > +#include <mm/core_memprot.h> > +#include <io.h> > +#include <string.h> > +#include <rng_pta.h> > +#include <rng_pta_client.h> > + > +static uint8_t entropy_pool[ENTROPY_POOL_SIZE] = {0}; > +static uint32_t entropy_size; > + > +/* Current sensor data */ > +static uint8_t sensor_data[NUM_OF_SENSORS][SENSOR_DATA_SIZE] = {0}; > +static uint8_t s; A global variable called `s`? Are you kidding? ;-) > + > +/* Sensor data that passed health test */ > +static uint8_t sensor_data_pass[NUM_OF_SENSORS][SENSOR_DATA_SIZE] = {0}; > +static uint8_t num_sensors_pass; > + > +static uint8_t rest_data_pass[NUM_OF_SENSORS][SENSOR_DATA_SIZE] = {0}; > + > +static uint32_t health_test_fail_cnt; Failure count seems a bit useless if we don't know how many times it passed! > + > +static unsigned int entropy_lock = SPINLOCK_UNLOCK; > + > +static void pool_add_entropy(uint8_t *entropy, uint32_t size) > +{ > + uint32_t copy_size; > + > + if (entropy_size >= ENTROPY_POOL_SIZE) > + return; > + > + if ((ENTROPY_POOL_SIZE - entropy_size) >= size) > + copy_size = size; > + else > + copy_size = ENTROPY_POOL_SIZE - entropy_size; > + > + memcpy((entropy_pool + entropy_size), entropy, copy_size); > + > + entropy_size += copy_size; > +} > + > +static void pool_get_entropy(uint8_t *buf, uint32_t size) > +{ > + uint32_t off; > + > + if (size > entropy_size) > + return; > + > + off = entropy_size - size; > + > + memcpy(buf, &entropy_pool[off], size); > + entropy_size -= size; > +} > + > +static bool health_test(uint8_t sensor_id) > +{ > + bool result = true; > + uint8_t bit_flip_1_0 = 0, bit_flip_0_1 = 0; > + uint8_t i; > + > + /* > + * Heatlh test to check if count of bit flips 1-0 or 0-1 lies in 12.5% > + * to 37.5% of 128 bytes raw data from particular sensor reading. In > + * ideal scenario either of bit flips should be around 25%. > + */ This sort of comment should probably be placed next to MIN/MAX_BIT_FLIP_COUNT rather then in the code (since that's where the 12.5% is actually realized). Likewise the upper limit sounds very wrong to me. For a full entropy source Pbitflip is 0.5 so any value lower than this extremely suspect. I'm actually slightly surprised that the code works (it means that the previous sensor value strongly predicts the next). > + for (i = 0; i < (SENSOR_DATA_SIZE - 1); i++) { > + if ((sensor_data[sensor_id][i] ^ > + sensor_data[sensor_id][i + 1]) & 0x1) { > + bit_flip_1_0 += sensor_data[sensor_id][i] & 0x1; > + bit_flip_0_1 += sensor_data[sensor_id][i + 1] & 0x1; > + } > + } > + > + if (bit_flip_1_0 > bit_flip_0_1) { > + if (bit_flip_0_1 < MIN_BIT_FLIP_COUNT) > + result = false; > + if (bit_flip_1_0 > MAX_BIT_FLIP_COUNT) > + result = false; > + } else { > + if (bit_flip_1_0 < MIN_BIT_FLIP_COUNT) > + result = false; > + if (bit_flip_0_1 > MAX_BIT_FLIP_COUNT) > + result = false; > + } > + > + return result; > +} > + > +static void pool_check_add_entropy(void) > +{ > + uint32_t i; > + uint8_t entropy_sha512_256[TEE_SHA256_HASH_SIZE]; > + uint8_t rest_sensors_pass = 0; > + TEE_Result res; > + > + for (i = 0; i < NUM_OF_SENSORS; i++) { > + /* Check if particular sensor data passes health test */ > + if (health_test(i) == true) { > + if (num_sensors_pass < NUM_OF_SENSORS) { > + memcpy(sensor_data_pass[num_sensors_pass], > + sensor_data[i], SENSOR_DATA_SIZE); > + num_sensors_pass++; > + } else { > + memcpy(rest_data_pass[rest_sensors_pass], > + sensor_data[i], SENSOR_DATA_SIZE); > + rest_sensors_pass++; > + } > + } else { > + health_test_fail_cnt++; > + /* > + * Report health test failures if it exceeds certain > + * threshold. > + */ > + if (health_test_fail_cnt >= THRESHOLD_REPORT_FAILURE) { > + IMSG("Health test failed %d times\n", > + health_test_fail_cnt); > + health_test_fail_cnt = 0; How will this result in bug reports (will anyone "normal" be observing this UART?). > + } > + } > + } > + > + /* Check if sensor_data_pass is full */ > + if (num_sensors_pass == NUM_OF_SENSORS) { > + /* > + * Use vetted conditioner SHA512/256 as per > + * NIST.SP.800-90B to condition raw data from entropy > + * source. > + * Here we have assumed that entropy source provides > + * 4 bits per 7 sensor readings per sample. > + * Also as per NIST.SP.800-90B, to get full entropy > + * from vetted conditioner, we need to supply double of > + * input entropy. So with full entropy (8 bits per byte) > + * we will get yield as one byte of output data for > + * every 28 sensor readings. > + * For 32 bytes of SHA512/256 output data, we need to > + * supply 896 bytes of raw input data. > + */ Again most of these figures are implemented in the macros. That is where the workings of any calculations we make should be. > + res = hash_sha512_256_compute(entropy_sha512_256, > + (uint8_t *)sensor_data_pass, > + CONDITIONER_PAYLOAD); > + if (res == TEE_SUCCESS) > + pool_add_entropy(entropy_sha512_256, > + TEE_SHA256_HASH_SIZE); Is there a timer we can use to know how measure how long the hash takes takes (it is running at interrupt level)? > + } > + > + if (rest_sensors_pass) > + memcpy((uint8_t *)sensor_data_pass, (uint8_t *)rest_data_pass, > + (rest_sensors_pass * SENSOR_DATA_SIZE)); > + > + num_sensors_pass = rest_sensors_pass; > +} > + > +void rng_collect_entropy(void) > +{ > + uint8_t i; > + void *vaddr; > + uint32_t exceptions = thread_mask_exceptions(THREAD_EXCP_ALL); > + > + cpu_spin_lock(&entropy_lock); > + > + for (i = 0; i < NUM_OF_SENSORS; i++) { > + vaddr = phys_to_virt_io(THERMAL_SENSOR_BASE0 + > + (THERMAL_SENSOR_OFFSET * i) + > + TEMP_DATA_REG_OFFSET); > + sensor_data[i][s] = (uint8_t)read32((vaddr_t)vaddr); > + } > + > + s++; > + > + if (s >= SENSOR_DATA_SIZE) { > + pool_check_add_entropy(); > + s = 0; > + } > + > + if (entropy_size >= ENTROPY_POOL_SIZE) { > + generic_timer_stop(); > + timer_fiq_running = false; > + } > + > + cpu_spin_unlock(&entropy_lock); > + thread_set_exceptions(exceptions); > +} > + > +static TEE_Result rng_get_entropy(uint32_t types, > + TEE_Param params[TEE_NUM_PARAMS]) > +{ > + uint8_t *e = NULL; > + uint32_t pool_size = 0, rq_size = 0; > + uint32_t exceptions = thread_mask_exceptions(THREAD_EXCP_ALL); > + > + cpu_spin_lock(&entropy_lock); > + > + if (types != TEE_PARAM_TYPES(TEE_PARAM_TYPE_MEMREF_INOUT, > + TEE_PARAM_TYPE_NONE, > + TEE_PARAM_TYPE_NONE, > + TEE_PARAM_TYPE_NONE)) { > + EMSG("bad parameters types: 0x%" PRIx32, types); > + return TEE_ERROR_BAD_PARAMETERS; Spin lock still held, interrupts still masked. > + } > + > + rq_size = params[0].memref.size; > + > + if ((rq_size == 0) || (rq_size > ENTROPY_POOL_SIZE)) > + return TEE_ERROR_NOT_SUPPORTED; Spin lock still held, interrupts still masked. > + > + e = (uint8_t *)params[0].memref.buffer; > + if (!e) > + return TEE_ERROR_BAD_PARAMETERS; Spin lock still held, interrupts still masked. Arguably we should even have taken the lock (or masked interrupts) until *after* we validate the input. > + > + pool_size = entropy_size; > + > + if (pool_size < rq_size) { > + params[0].memref.size = pool_size; > + pool_get_entropy(e, pool_size); Shouldn't we be checking for continual health check failure here and providing a custom return code. > + } else { > + params[0].memref.size = rq_size; > + pool_get_entropy(e, rq_size); > + } > + > + if (timer_fiq_running == false) { > + /* Enable timer FIQ to fetch entropy */ > + generic_timer_start(); > + timer_fiq_running = true; > + } > + > + cpu_spin_unlock(&entropy_lock); > + thread_set_exceptions(exceptions); > + > + return TEE_SUCCESS; > +} > + > +/* > + * Trusted Application Entry Points > + */ > +static TEE_Result open_session(uint32_t param_types __unused, > + TEE_Param params[TEE_NUM_PARAMS] __unused, > + void **session_context __unused) > +{ > + DMSG("open entry point for pseudo-TA \"%s\"", PTA_NAME); > + return TEE_SUCCESS; > +} > + > +static TEE_Result invoke_command(void *pSessionContext __unused, > + uint32_t nCommandID, uint32_t nParamTypes, > + TEE_Param pParams[TEE_NUM_PARAMS]) > +{ > + FMSG("command entry point for pseudo-TA \"%s\"", PTA_NAME); Is it normal for invoke_command to be a different trace level to open_session? > + > + switch (nCommandID) { > + case PTA_CMD_GET_ENTROPY: > + return rng_get_entropy(nParamTypes, pParams); > + default: > + break; > + } > + > + return TEE_ERROR_NOT_IMPLEMENTED; > +} > + > +pseudo_ta_register(.uuid = PTA_RNG_UUID, .name = PTA_NAME, > + .flags = PTA_DEFAULT_FLAGS, > + .open_session_entry_point = open_session, > + .invoke_command_entry_point = invoke_command); > diff --git a/core/arch/arm/plat-synquacer/rng_pta.h b/core/arch/arm/plat-synquacer/rng_pta.h > new file mode 100644 > index 0000000..e238c57 > --- /dev/null > +++ b/core/arch/arm/plat-synquacer/rng_pta.h > @@ -0,0 +1,37 @@ > +/* SPDX-License-Identifier: BSD-2-Clause */ > +/* > + * Copyright (C) 2018, Linaro Limited > + */ > + > +#ifndef __RNG_PTA_H > +#define __RNG_PTA_H > + > +#define PTA_NAME "rng.pta" > + > +#define THERMAL_SENSOR_BASE0 0x54190800 > +#define THERMAL_SENSOR_OFFSET 0x80 > +#define NUM_OF_SENSORS 7 > + > +#define TEMP_DATA_REG_OFFSET 0x34 > + > +#define ENTROPY_POOL_SIZE 4096 > + > +#define SENSOR_DATA_SIZE 128 > +#define CONDITIONER_PAYLOAD (SENSOR_DATA_SIZE * NUM_OF_SENSORS) > + > +#define LSB_MASK 0x1 This is not used anywhere. Should be removed (to be honest it should be removed anyway... its rather too close to something like #define FIVE 5 for my taste).. > + > +#define MAX_BIT_FLIP_COUNT 48 > +#define MIN_BIT_FLIP_COUNT 16 Needs comment explaining how these values are arrived at. > + > +#define THRESHOLD_REPORT_FAILURE 10 Are any of these used in other files? If not they can go in the rng_pta.c file. > + > +extern bool timer_fiq_running; > + > +void rng_collect_entropy(void); > + > +void generic_timer_start(void); > +void generic_timer_stop(void); > +void generic_timer_handler(void); Why are the prototypes for the generic timer in rng_pta.h? > + > +#endif /* __RNG_PTA_H */ > diff --git a/core/arch/arm/plat-synquacer/rng_pta_client.h b/core/arch/arm/plat-synquacer/rng_pta_client.h > new file mode 100644 > index 0000000..ddd398c > --- /dev/null > +++ b/core/arch/arm/plat-synquacer/rng_pta_client.h > @@ -0,0 +1,22 @@ > +/* SPDX-License-Identifier: BSD-2-Clause */ > +/* > + * Copyright (C) 2018, Linaro Limited > + */ > + > +#ifndef __RNG_PTA_CLIENT_H > +#define __RNG_PTA_CLIENT_H > + > +#define PTA_RNG_UUID { 0xab7a617c, 0xb8e7, 0x4d8f, \ > + { 0x83, 0x01, 0xd0, 0x9b, 0x61, 0x03, 0x6b, 0x64 } } > + > +/* > + * PTA_CMD_GET_ENTROPY - Get Entropy from RNG using Thermal Sensor > + * > + * param[0] (inout memref) - Entropy buffer memory reference > + * param[1] unused > + * param[2] unused > + * param[3] unused Knowing what the different possible return codes mean would be nice. > + */ > +#define PTA_CMD_GET_ENTROPY 0x0 > + > +#endif /* __RNG_PTA_CLIENT_H */ > diff --git a/core/arch/arm/plat-synquacer/sub.mk b/core/arch/arm/plat-synquacer/sub.mk > index 8ddc2fd..013e57d 100644 > --- a/core/arch/arm/plat-synquacer/sub.mk > +++ b/core/arch/arm/plat-synquacer/sub.mk > @@ -1,2 +1,4 @@ > global-incdirs-y += . > srcs-y += main.c > +srcs-y += rng_pta.c > +srcs-y += timer_fiq.c > diff --git a/core/arch/arm/plat-synquacer/timer_fiq.c b/core/arch/arm/plat-synquacer/timer_fiq.c > new file mode 100644 > index 0000000..e25a676 > --- /dev/null > +++ b/core/arch/arm/plat-synquacer/timer_fiq.c > @@ -0,0 +1,43 @@ > +// SPDX-License-Identifier: BSD-2-Clause > +/* > + * Copyright (c) 2018, Linaro Limited > + */ > + > +#include <arm.h> > +#include <console.h> > +#include <drivers/gic.h> > +#include <io.h> > +#include <kernel/panic.h> > +#include <kernel/misc.h> > +#include <rng_pta.h> > + > +bool timer_fiq_running = false; > + > +void generic_timer_start(void) > +{ > + uint64_t cval; > + uint32_t ctl = 1; > + > + /* The timer will fire every 2 ms */ > + cval = read_cntpct() + (read_cntfrq() / 500); > + write_cntps_cval(cval); > + > + /* Enable the secure physical timer */ > + write_cntps_ctl(ctl); > +} > + > +void generic_timer_stop(void) > +{ > + /* Disable the timer */ > + write_cntps_ctl(0); > +} > + > +void generic_timer_handler(void) > +{ > + /* Ensure that the timer did assert the interrupt */ > + assert((read_cntps_ctl() >> 2)); > + > + /* Disable the timer and reprogram it */ > + write_cntps_ctl(0); > + generic_timer_start(); > +} > -- > 2.7.4 >
On Fri, 16 Nov 2018 at 21:32, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Fri, Nov 16, 2018 at 06:13:02PM +0530, Sumit Garg wrote: > > This platform provides 7 on-chip thermal sensors accessible from secure > > world only. So, using randomness from these sensors we have created a True > > RNG as a pseudo TA. > > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > > --- > > core/arch/arm/include/arm64.h | 4 + > > core/arch/arm/plat-synquacer/main.c | 38 +++- > > core/arch/arm/plat-synquacer/platform_config.h | 3 + > > core/arch/arm/plat-synquacer/rng_pta.c | 270 +++++++++++++++++++++++++ > > core/arch/arm/plat-synquacer/rng_pta.h | 37 ++++ > > core/arch/arm/plat-synquacer/rng_pta_client.h | 22 ++ > > core/arch/arm/plat-synquacer/sub.mk | 2 + > > core/arch/arm/plat-synquacer/timer_fiq.c | 43 ++++ > > Feels like this could be split apart into a timer FIQ patch > and a seperate RNG PTA patch (later patch would have to add call to > rng_collect_entropy() but other than this the code appears to seperate > cleanly. > Agree, will split this patch into 2. > > > diff --git a/core/arch/arm/plat-synquacer/rng_pta.c b/core/arch/arm/plat-synquacer/rng_pta.c > > new file mode 100644 > > index 0000000..a760b54 > > --- /dev/null > > +++ b/core/arch/arm/plat-synquacer/rng_pta.c > > @@ -0,0 +1,270 @@ > > +// SPDX-License-Identifier: BSD-2-Clause > > +/* > > + * Copyright (C) 2018, Linaro Limited > > + */ > > Shouldn't the theory of operation, limitations, testing, entropy > estimates etc. go here? (and if not here then where?) > Agree, will add this info here. > > > + > > +#include <crypto/crypto.h> > > +#include <kernel/delay.h> > > +#include <kernel/pseudo_ta.h> > > +#include <kernel/spinlock.h> > > +#include <mm/core_memprot.h> > > +#include <io.h> > > +#include <string.h> > > +#include <rng_pta.h> > > +#include <rng_pta_client.h> > > + > > +static uint8_t entropy_pool[ENTROPY_POOL_SIZE] = {0}; > > +static uint32_t entropy_size; > > + > > +/* Current sensor data */ > > +static uint8_t sensor_data[NUM_OF_SENSORS][SENSOR_DATA_SIZE] = {0}; > > +static uint8_t s; > > A global variable called `s`? Are you kidding? ;-) > Haha. It was more of copy-paste effect from local variable index name. Will rename it to be more specific one. > > > + > > +/* Sensor data that passed health test */ > > +static uint8_t sensor_data_pass[NUM_OF_SENSORS][SENSOR_DATA_SIZE] = {0}; > > +static uint8_t num_sensors_pass; > > + > > +static uint8_t rest_data_pass[NUM_OF_SENSORS][SENSOR_DATA_SIZE] = {0}; > > + > > +static uint32_t health_test_fail_cnt; > > Failure count seems a bit useless if we don't know how many times it > passed! > Hmm, will add pass count too. And report failure in case failures exceeds a particular threshold probability. > > + > > +static unsigned int entropy_lock = SPINLOCK_UNLOCK; > > + > > +static void pool_add_entropy(uint8_t *entropy, uint32_t size) > > +{ > > + uint32_t copy_size; > > + > > + if (entropy_size >= ENTROPY_POOL_SIZE) > > + return; > > + > > + if ((ENTROPY_POOL_SIZE - entropy_size) >= size) > > + copy_size = size; > > + else > > + copy_size = ENTROPY_POOL_SIZE - entropy_size; > > + > > + memcpy((entropy_pool + entropy_size), entropy, copy_size); > > + > > + entropy_size += copy_size; > > +} > > + > > +static void pool_get_entropy(uint8_t *buf, uint32_t size) > > +{ > > + uint32_t off; > > + > > + if (size > entropy_size) > > + return; > > + > > + off = entropy_size - size; > > + > > + memcpy(buf, &entropy_pool[off], size); > > + entropy_size -= size; > > +} > > + > > +static bool health_test(uint8_t sensor_id) > > +{ > > + bool result = true; > > + uint8_t bit_flip_1_0 = 0, bit_flip_0_1 = 0; > > + uint8_t i; > > + > > + /* > > + * Heatlh test to check if count of bit flips 1-0 or 0-1 lies in 12.5% > > + * to 37.5% of 128 bytes raw data from particular sensor reading. In > > + * ideal scenario either of bit flips should be around 25%. > > + */ > > This sort of comment should probably be placed next to > MIN/MAX_BIT_FLIP_COUNT rather then in the code (since that's where > the 12.5% is actually realized). Ok. > > Likewise the upper limit sounds very wrong to me. For a full entropy > source Pbitflip is 0.5 so any value lower than this extremely suspect. > I'm actually slightly surprised that the code works (it means that the > previous sensor value strongly predicts the next). Isn't for a full entropy source two consecutive sample reading should have following probable values? Val Probability 11 -> 0.25 00 -> 0.25 01 -> 0.25 10 -> 0.25 So ideal probability of rising or falling edge should be 0.25. Earlier I set MAX and MIN limit as +/- 0.125 of this ideal value. But if I set MAX limit to 0.5, I do see drop in health test failures. Also isn't 0.5 would be "10101010......1010101..." continuous sequence? > > > > + for (i = 0; i < (SENSOR_DATA_SIZE - 1); i++) { > > + if ((sensor_data[sensor_id][i] ^ > > + sensor_data[sensor_id][i + 1]) & 0x1) { > > + bit_flip_1_0 += sensor_data[sensor_id][i] & 0x1; > > + bit_flip_0_1 += sensor_data[sensor_id][i + 1] & 0x1; > > + } > > + } > > + > > + if (bit_flip_1_0 > bit_flip_0_1) { > > + if (bit_flip_0_1 < MIN_BIT_FLIP_COUNT) > > + result = false; > > + if (bit_flip_1_0 > MAX_BIT_FLIP_COUNT) > > + result = false; > > + } else { > > + if (bit_flip_1_0 < MIN_BIT_FLIP_COUNT) > > + result = false; > > + if (bit_flip_0_1 > MAX_BIT_FLIP_COUNT) > > + result = false; > > + } > > + > > + return result; > > +} > > + > > +static void pool_check_add_entropy(void) > > +{ > > + uint32_t i; > > + uint8_t entropy_sha512_256[TEE_SHA256_HASH_SIZE]; > > + uint8_t rest_sensors_pass = 0; > > + TEE_Result res; > > + > > + for (i = 0; i < NUM_OF_SENSORS; i++) { > > + /* Check if particular sensor data passes health test */ > > + if (health_test(i) == true) { > > + if (num_sensors_pass < NUM_OF_SENSORS) { > > + memcpy(sensor_data_pass[num_sensors_pass], > > + sensor_data[i], SENSOR_DATA_SIZE); > > + num_sensors_pass++; > > + } else { > > + memcpy(rest_data_pass[rest_sensors_pass], > > + sensor_data[i], SENSOR_DATA_SIZE); > > + rest_sensors_pass++; > > + } > > + } else { > > + health_test_fail_cnt++; > > + /* > > + * Report health test failures if it exceeds certain > > + * threshold. > > + */ > > + if (health_test_fail_cnt >= THRESHOLD_REPORT_FAILURE) { > > + IMSG("Health test failed %d times\n", > > + health_test_fail_cnt); > > + health_test_fail_cnt = 0; > > How will this result in bug reports (will anyone "normal" be observing > this UART?). > In case of Developerbox, UART is shared among normal and secure world. There could be a daemon in normal world observing this kind of messages on UART. Having said that it seems that reporting error code to consuming application in case of health test failure to be more appropriate. > > > + } > > + } > > + } > > + > > + /* Check if sensor_data_pass is full */ > > + if (num_sensors_pass == NUM_OF_SENSORS) { > > + /* > > + * Use vetted conditioner SHA512/256 as per > > + * NIST.SP.800-90B to condition raw data from entropy > > + * source. > > + * Here we have assumed that entropy source provides > > + * 4 bits per 7 sensor readings per sample. > > + * Also as per NIST.SP.800-90B, to get full entropy > > + * from vetted conditioner, we need to supply double of > > + * input entropy. So with full entropy (8 bits per byte) > > + * we will get yield as one byte of output data for > > + * every 28 sensor readings. > > + * For 32 bytes of SHA512/256 output data, we need to > > + * supply 896 bytes of raw input data. > > + */ > > Again most of these figures are implemented in the macros. That is where > the workings of any calculations we make should be. > Ok. > > > + res = hash_sha512_256_compute(entropy_sha512_256, > > + (uint8_t *)sensor_data_pass, > > + CONDITIONER_PAYLOAD); > > + if (res == TEE_SUCCESS) > > + pool_add_entropy(entropy_sha512_256, > > + TEE_SHA256_HASH_SIZE); > > Is there a timer we can use to know how measure how long the hash takes > takes (it is running at interrupt level)? > Yes we could use generic timer to measure this. I will try to measure it. > > > + } > > + > > + if (rest_sensors_pass) > > + memcpy((uint8_t *)sensor_data_pass, (uint8_t *)rest_data_pass, > > + (rest_sensors_pass * SENSOR_DATA_SIZE)); > > + > > + num_sensors_pass = rest_sensors_pass; > > +} > > + > > +void rng_collect_entropy(void) > > +{ > > + uint8_t i; > > + void *vaddr; > > + uint32_t exceptions = thread_mask_exceptions(THREAD_EXCP_ALL); > > + > > + cpu_spin_lock(&entropy_lock); > > + > > + for (i = 0; i < NUM_OF_SENSORS; i++) { > > + vaddr = phys_to_virt_io(THERMAL_SENSOR_BASE0 + > > + (THERMAL_SENSOR_OFFSET * i) + > > + TEMP_DATA_REG_OFFSET); > > + sensor_data[i][s] = (uint8_t)read32((vaddr_t)vaddr); > > + } > > + > > + s++; > > + > > + if (s >= SENSOR_DATA_SIZE) { > > + pool_check_add_entropy(); > > + s = 0; > > + } > > + > > + if (entropy_size >= ENTROPY_POOL_SIZE) { > > + generic_timer_stop(); > > + timer_fiq_running = false; > > + } > > + > > + cpu_spin_unlock(&entropy_lock); > > + thread_set_exceptions(exceptions); > > +} > > + > > +static TEE_Result rng_get_entropy(uint32_t types, > > + TEE_Param params[TEE_NUM_PARAMS]) > > +{ > > + uint8_t *e = NULL; > > + uint32_t pool_size = 0, rq_size = 0; > > + uint32_t exceptions = thread_mask_exceptions(THREAD_EXCP_ALL); > > + > > + cpu_spin_lock(&entropy_lock); > > + > > + if (types != TEE_PARAM_TYPES(TEE_PARAM_TYPE_MEMREF_INOUT, > > + TEE_PARAM_TYPE_NONE, > > + TEE_PARAM_TYPE_NONE, > > + TEE_PARAM_TYPE_NONE)) { > > + EMSG("bad parameters types: 0x%" PRIx32, types); > > + return TEE_ERROR_BAD_PARAMETERS; > > Spin lock still held, interrupts still masked. > > > > + } > > + > > + rq_size = params[0].memref.size; > > + > > + if ((rq_size == 0) || (rq_size > ENTROPY_POOL_SIZE)) > > + return TEE_ERROR_NOT_SUPPORTED; > > Spin lock still held, interrupts still masked. > > > > + > > + e = (uint8_t *)params[0].memref.buffer; > > + if (!e) > > + return TEE_ERROR_BAD_PARAMETERS; > > Spin lock still held, interrupts still masked. > > Arguably we should even have taken the lock (or masked interrupts) until > *after* we validate the input. > Yes you are correct. As we don't touch global vars until validation of input. I will move these spin lock and interrupts masking after validation of input. > > > + > > + pool_size = entropy_size; > > + > > + if (pool_size < rq_size) { > > + params[0].memref.size = pool_size; > > + pool_get_entropy(e, pool_size); > > Shouldn't we be checking for continual health check failure here and > providing a custom return code. > Ok, I will add a custom error code here. > > > + } else { > > + params[0].memref.size = rq_size; > > + pool_get_entropy(e, rq_size); > > + } > > + > > + if (timer_fiq_running == false) { > > + /* Enable timer FIQ to fetch entropy */ > > + generic_timer_start(); > > + timer_fiq_running = true; > > + } > > + > > + cpu_spin_unlock(&entropy_lock); > > + thread_set_exceptions(exceptions); > > + > > + return TEE_SUCCESS; > > +} > > + > > +/* > > + * Trusted Application Entry Points > > + */ > > +static TEE_Result open_session(uint32_t param_types __unused, > > + TEE_Param params[TEE_NUM_PARAMS] __unused, > > + void **session_context __unused) > > +{ > > + DMSG("open entry point for pseudo-TA \"%s\"", PTA_NAME); > > + return TEE_SUCCESS; > > +} > > + > > +static TEE_Result invoke_command(void *pSessionContext __unused, > > + uint32_t nCommandID, uint32_t nParamTypes, > > + TEE_Param pParams[TEE_NUM_PARAMS]) > > +{ > > + FMSG("command entry point for pseudo-TA \"%s\"", PTA_NAME); > > Is it normal for invoke_command to be a different trace level to > open_session? > I don't think there in any normal for trace levels to be used inside Trusted Application. Here I have used FMSG (flow trace level = 4) instead of DMSG (debug trace level = 3) in invoke_command for ease of debugging as invoke_command is called much often as compared to open_session. > > > + > > + switch (nCommandID) { > > + case PTA_CMD_GET_ENTROPY: > > + return rng_get_entropy(nParamTypes, pParams); > > + default: > > + break; > > + } > > + > > + return TEE_ERROR_NOT_IMPLEMENTED; > > +} > > + > > +pseudo_ta_register(.uuid = PTA_RNG_UUID, .name = PTA_NAME, > > + .flags = PTA_DEFAULT_FLAGS, > > + .open_session_entry_point = open_session, > > + .invoke_command_entry_point = invoke_command); > > diff --git a/core/arch/arm/plat-synquacer/rng_pta.h b/core/arch/arm/plat-synquacer/rng_pta.h > > new file mode 100644 > > index 0000000..e238c57 > > --- /dev/null > > +++ b/core/arch/arm/plat-synquacer/rng_pta.h > > @@ -0,0 +1,37 @@ > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > +/* > > + * Copyright (C) 2018, Linaro Limited > > + */ > > + > > +#ifndef __RNG_PTA_H > > +#define __RNG_PTA_H > > + > > +#define PTA_NAME "rng.pta" > > + > > +#define THERMAL_SENSOR_BASE0 0x54190800 > > +#define THERMAL_SENSOR_OFFSET 0x80 > > +#define NUM_OF_SENSORS 7 > > + > > +#define TEMP_DATA_REG_OFFSET 0x34 > > + > > +#define ENTROPY_POOL_SIZE 4096 > > + > > +#define SENSOR_DATA_SIZE 128 > > +#define CONDITIONER_PAYLOAD (SENSOR_DATA_SIZE * NUM_OF_SENSORS) > > + > > +#define LSB_MASK 0x1 > > This is not used anywhere. Should be removed (to be honest it should be > removed anyway... its rather too close to something like #define FIVE 5 > for my taste).. > Ok. > > > + > > +#define MAX_BIT_FLIP_COUNT 48 > > +#define MIN_BIT_FLIP_COUNT 16 > > Needs comment explaining how these values are arrived at. > Ok. > > > + > > +#define THRESHOLD_REPORT_FAILURE 10 > > Are any of these used in other files? If not they can go in the rng_pta.c file. > No they aren't used anywhere else. Will move them to rng_pta.c file. > > > + > > +extern bool timer_fiq_running; > > + > > +void rng_collect_entropy(void); > > + > > +void generic_timer_start(void); > > +void generic_timer_stop(void); > > +void generic_timer_handler(void); > > Why are the prototypes for the generic timer in rng_pta.h? > Will create separate .h for generic timer. > > > + > > +#endif /* __RNG_PTA_H */ > > diff --git a/core/arch/arm/plat-synquacer/rng_pta_client.h b/core/arch/arm/plat-synquacer/rng_pta_client.h > > new file mode 100644 > > index 0000000..ddd398c > > --- /dev/null > > +++ b/core/arch/arm/plat-synquacer/rng_pta_client.h > > @@ -0,0 +1,22 @@ > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > +/* > > + * Copyright (C) 2018, Linaro Limited > > + */ > > + > > +#ifndef __RNG_PTA_CLIENT_H > > +#define __RNG_PTA_CLIENT_H > > + > > +#define PTA_RNG_UUID { 0xab7a617c, 0xb8e7, 0x4d8f, \ > > + { 0x83, 0x01, 0xd0, 0x9b, 0x61, 0x03, 0x6b, 0x64 } } > > + > > +/* > > + * PTA_CMD_GET_ENTROPY - Get Entropy from RNG using Thermal Sensor > > + * > > + * param[0] (inout memref) - Entropy buffer memory reference > > + * param[1] unused > > + * param[2] unused > > + * param[3] unused > > Knowing what the different possible return codes mean would be nice. > Ok will add return codes here. > > > + */ > > +#define PTA_CMD_GET_ENTROPY 0x0 > > + > > +#endif /* __RNG_PTA_CLIENT_H */ > > diff --git a/core/arch/arm/plat-synquacer/sub.mk b/core/arch/arm/plat-synquacer/sub.mk > > index 8ddc2fd..013e57d 100644 > > --- a/core/arch/arm/plat-synquacer/sub.mk > > +++ b/core/arch/arm/plat-synquacer/sub.mk > > @@ -1,2 +1,4 @@ > > global-incdirs-y += . > > srcs-y += main.c > > +srcs-y += rng_pta.c > > +srcs-y += timer_fiq.c > > diff --git a/core/arch/arm/plat-synquacer/timer_fiq.c b/core/arch/arm/plat-synquacer/timer_fiq.c > > new file mode 100644 > > index 0000000..e25a676 > > --- /dev/null > > +++ b/core/arch/arm/plat-synquacer/timer_fiq.c > > @@ -0,0 +1,43 @@ > > +// SPDX-License-Identifier: BSD-2-Clause > > +/* > > + * Copyright (c) 2018, Linaro Limited > > + */ > > + > > +#include <arm.h> > > +#include <console.h> > > +#include <drivers/gic.h> > > +#include <io.h> > > +#include <kernel/panic.h> > > +#include <kernel/misc.h> > > +#include <rng_pta.h> > > + > > +bool timer_fiq_running = false; > > + > > +void generic_timer_start(void) > > +{ > > + uint64_t cval; > > + uint32_t ctl = 1; > > + > > + /* The timer will fire every 2 ms */ > > + cval = read_cntpct() + (read_cntfrq() / 500); > > + write_cntps_cval(cval); > > + > > + /* Enable the secure physical timer */ > > + write_cntps_ctl(ctl); > > +} > > + > > +void generic_timer_stop(void) > > +{ > > + /* Disable the timer */ > > + write_cntps_ctl(0); > > +} > > + > > +void generic_timer_handler(void) > > +{ > > + /* Ensure that the timer did assert the interrupt */ > > + assert((read_cntps_ctl() >> 2)); > > + > > + /* Disable the timer and reprogram it */ > > + write_cntps_ctl(0); > > + generic_timer_start(); > > +} > > -- > > 2.7.4 > >
On Mon, 19 Nov 2018 at 13:39, Sumit Garg <sumit.garg@linaro.org> wrote: > > On Fri, 16 Nov 2018 at 21:32, Daniel Thompson > <daniel.thompson@linaro.org> wrote: > > > > On Fri, Nov 16, 2018 at 06:13:02PM +0530, Sumit Garg wrote: > > > This platform provides 7 on-chip thermal sensors accessible from secure > > > world only. So, using randomness from these sensors we have created a True > > > RNG as a pseudo TA. > > > > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > > > --- > > > core/arch/arm/include/arm64.h | 4 + > > > core/arch/arm/plat-synquacer/main.c | 38 +++- > > > core/arch/arm/plat-synquacer/platform_config.h | 3 + > > > core/arch/arm/plat-synquacer/rng_pta.c | 270 +++++++++++++++++++++++++ > > > core/arch/arm/plat-synquacer/rng_pta.h | 37 ++++ > > > core/arch/arm/plat-synquacer/rng_pta_client.h | 22 ++ > > > core/arch/arm/plat-synquacer/sub.mk | 2 + > > > core/arch/arm/plat-synquacer/timer_fiq.c | 43 ++++ > > > > Feels like this could be split apart into a timer FIQ patch > > and a seperate RNG PTA patch (later patch would have to add call to > > rng_collect_entropy() but other than this the code appears to seperate > > cleanly. > > > > Agree, will split this patch into 2. > > > > > > diff --git a/core/arch/arm/plat-synquacer/rng_pta.c b/core/arch/arm/plat-synquacer/rng_pta.c > > > new file mode 100644 > > > index 0000000..a760b54 > > > --- /dev/null > > > +++ b/core/arch/arm/plat-synquacer/rng_pta.c > > > @@ -0,0 +1,270 @@ > > > +// SPDX-License-Identifier: BSD-2-Clause > > > +/* > > > + * Copyright (C) 2018, Linaro Limited > > > + */ > > > > Shouldn't the theory of operation, limitations, testing, entropy > > estimates etc. go here? (and if not here then where?) > > > > Agree, will add this info here. > > > > > > + > > > +#include <crypto/crypto.h> > > > +#include <kernel/delay.h> > > > +#include <kernel/pseudo_ta.h> > > > +#include <kernel/spinlock.h> > > > +#include <mm/core_memprot.h> > > > +#include <io.h> > > > +#include <string.h> > > > +#include <rng_pta.h> > > > +#include <rng_pta_client.h> > > > + > > > +static uint8_t entropy_pool[ENTROPY_POOL_SIZE] = {0}; > > > +static uint32_t entropy_size; > > > + > > > +/* Current sensor data */ > > > +static uint8_t sensor_data[NUM_OF_SENSORS][SENSOR_DATA_SIZE] = {0}; > > > +static uint8_t s; > > > > A global variable called `s`? Are you kidding? ;-) > > > > Haha. It was more of copy-paste effect from local variable index name. > Will rename it to be more specific one. > > > > > > + > > > +/* Sensor data that passed health test */ > > > +static uint8_t sensor_data_pass[NUM_OF_SENSORS][SENSOR_DATA_SIZE] = {0}; > > > +static uint8_t num_sensors_pass; > > > + > > > +static uint8_t rest_data_pass[NUM_OF_SENSORS][SENSOR_DATA_SIZE] = {0}; > > > + > > > +static uint32_t health_test_fail_cnt; > > > > Failure count seems a bit useless if we don't know how many times it > > passed! > > > > Hmm, will add pass count too. And report failure in case failures > exceeds a particular threshold probability. > > > > + > > > +static unsigned int entropy_lock = SPINLOCK_UNLOCK; > > > + > > > +static void pool_add_entropy(uint8_t *entropy, uint32_t size) > > > +{ > > > + uint32_t copy_size; > > > + > > > + if (entropy_size >= ENTROPY_POOL_SIZE) > > > + return; > > > + > > > + if ((ENTROPY_POOL_SIZE - entropy_size) >= size) > > > + copy_size = size; > > > + else > > > + copy_size = ENTROPY_POOL_SIZE - entropy_size; > > > + > > > + memcpy((entropy_pool + entropy_size), entropy, copy_size); > > > + > > > + entropy_size += copy_size; > > > +} > > > + > > > +static void pool_get_entropy(uint8_t *buf, uint32_t size) > > > +{ > > > + uint32_t off; > > > + > > > + if (size > entropy_size) > > > + return; > > > + > > > + off = entropy_size - size; > > > + > > > + memcpy(buf, &entropy_pool[off], size); > > > + entropy_size -= size; > > > +} > > > + > > > +static bool health_test(uint8_t sensor_id) > > > +{ > > > + bool result = true; > > > + uint8_t bit_flip_1_0 = 0, bit_flip_0_1 = 0; > > > + uint8_t i; > > > + > > > + /* > > > + * Heatlh test to check if count of bit flips 1-0 or 0-1 lies in 12.5% > > > + * to 37.5% of 128 bytes raw data from particular sensor reading. In > > > + * ideal scenario either of bit flips should be around 25%. > > > + */ > > > > This sort of comment should probably be placed next to > > MIN/MAX_BIT_FLIP_COUNT rather then in the code (since that's where > > the 12.5% is actually realized). > > Ok. > > > > > Likewise the upper limit sounds very wrong to me. For a full entropy > > source Pbitflip is 0.5 so any value lower than this extremely suspect. > > I'm actually slightly surprised that the code works (it means that the > > previous sensor value strongly predicts the next). > > Isn't for a full entropy source two consecutive sample reading should > have following probable values? > > Val Probability > 11 -> 0.25 > 00 -> 0.25 > 01 -> 0.25 > 10 -> 0.25 > > So ideal probability of rising or falling edge should be 0.25. Earlier > I set MAX and MIN limit as +/- 0.125 of this ideal value. > > But if I set MAX limit to 0.5, I do see drop in health test failures. > > Also isn't 0.5 would be "10101010......1010101..." continuous sequence? > > > > > > > > + for (i = 0; i < (SENSOR_DATA_SIZE - 1); i++) { > > > + if ((sensor_data[sensor_id][i] ^ > > > + sensor_data[sensor_id][i + 1]) & 0x1) { > > > + bit_flip_1_0 += sensor_data[sensor_id][i] & 0x1; > > > + bit_flip_0_1 += sensor_data[sensor_id][i + 1] & 0x1; > > > + } > > > + } > > > + > > > + if (bit_flip_1_0 > bit_flip_0_1) { > > > + if (bit_flip_0_1 < MIN_BIT_FLIP_COUNT) > > > + result = false; > > > + if (bit_flip_1_0 > MAX_BIT_FLIP_COUNT) > > > + result = false; > > > + } else { > > > + if (bit_flip_1_0 < MIN_BIT_FLIP_COUNT) > > > + result = false; > > > + if (bit_flip_0_1 > MAX_BIT_FLIP_COUNT) > > > + result = false; > > > + } > > > + > > > + return result; > > > +} > > > + > > > +static void pool_check_add_entropy(void) > > > +{ > > > + uint32_t i; > > > + uint8_t entropy_sha512_256[TEE_SHA256_HASH_SIZE]; > > > + uint8_t rest_sensors_pass = 0; > > > + TEE_Result res; > > > + > > > + for (i = 0; i < NUM_OF_SENSORS; i++) { > > > + /* Check if particular sensor data passes health test */ > > > + if (health_test(i) == true) { > > > + if (num_sensors_pass < NUM_OF_SENSORS) { > > > + memcpy(sensor_data_pass[num_sensors_pass], > > > + sensor_data[i], SENSOR_DATA_SIZE); > > > + num_sensors_pass++; > > > + } else { > > > + memcpy(rest_data_pass[rest_sensors_pass], > > > + sensor_data[i], SENSOR_DATA_SIZE); > > > + rest_sensors_pass++; > > > + } > > > + } else { > > > + health_test_fail_cnt++; > > > + /* > > > + * Report health test failures if it exceeds certain > > > + * threshold. > > > + */ > > > + if (health_test_fail_cnt >= THRESHOLD_REPORT_FAILURE) { > > > + IMSG("Health test failed %d times\n", > > > + health_test_fail_cnt); > > > + health_test_fail_cnt = 0; > > > > How will this result in bug reports (will anyone "normal" be observing > > this UART?). > > > > In case of Developerbox, UART is shared among normal and secure world. > There could be a daemon in normal world observing this kind of > messages on UART. > > Having said that it seems that reporting error code to consuming > application in case of health test failure to be more appropriate. > > > > > > + } > > > + } > > > + } > > > + > > > + /* Check if sensor_data_pass is full */ > > > + if (num_sensors_pass == NUM_OF_SENSORS) { > > > + /* > > > + * Use vetted conditioner SHA512/256 as per > > > + * NIST.SP.800-90B to condition raw data from entropy > > > + * source. > > > + * Here we have assumed that entropy source provides > > > + * 4 bits per 7 sensor readings per sample. > > > + * Also as per NIST.SP.800-90B, to get full entropy > > > + * from vetted conditioner, we need to supply double of > > > + * input entropy. So with full entropy (8 bits per byte) > > > + * we will get yield as one byte of output data for > > > + * every 28 sensor readings. > > > + * For 32 bytes of SHA512/256 output data, we need to > > > + * supply 896 bytes of raw input data. > > > + */ > > > > Again most of these figures are implemented in the macros. That is where > > the workings of any calculations we make should be. > > > > Ok. > > > > > > + res = hash_sha512_256_compute(entropy_sha512_256, > > > + (uint8_t *)sensor_data_pass, > > > + CONDITIONER_PAYLOAD); > > > + if (res == TEE_SUCCESS) > > > + pool_add_entropy(entropy_sha512_256, > > > + TEE_SHA256_HASH_SIZE); > > > > Is there a timer we can use to know how measure how long the hash takes > > takes (it is running at interrupt level)? > > > > Yes we could use generic timer to measure this. I will try to measure it. > Here hash takes approx. 24 micro seconds. -Sumit > > > > > + } > > > + > > > + if (rest_sensors_pass) > > > + memcpy((uint8_t *)sensor_data_pass, (uint8_t *)rest_data_pass, > > > + (rest_sensors_pass * SENSOR_DATA_SIZE)); > > > + > > > + num_sensors_pass = rest_sensors_pass; > > > +} > > > + > > > +void rng_collect_entropy(void) > > > +{ > > > + uint8_t i; > > > + void *vaddr; > > > + uint32_t exceptions = thread_mask_exceptions(THREAD_EXCP_ALL); > > > + > > > + cpu_spin_lock(&entropy_lock); > > > + > > > + for (i = 0; i < NUM_OF_SENSORS; i++) { > > > + vaddr = phys_to_virt_io(THERMAL_SENSOR_BASE0 + > > > + (THERMAL_SENSOR_OFFSET * i) + > > > + TEMP_DATA_REG_OFFSET); > > > + sensor_data[i][s] = (uint8_t)read32((vaddr_t)vaddr); > > > + } > > > + > > > + s++; > > > + > > > + if (s >= SENSOR_DATA_SIZE) { > > > + pool_check_add_entropy(); > > > + s = 0; > > > + } > > > + > > > + if (entropy_size >= ENTROPY_POOL_SIZE) { > > > + generic_timer_stop(); > > > + timer_fiq_running = false; > > > + } > > > + > > > + cpu_spin_unlock(&entropy_lock); > > > + thread_set_exceptions(exceptions); > > > +} > > > + > > > +static TEE_Result rng_get_entropy(uint32_t types, > > > + TEE_Param params[TEE_NUM_PARAMS]) > > > +{ > > > + uint8_t *e = NULL; > > > + uint32_t pool_size = 0, rq_size = 0; > > > + uint32_t exceptions = thread_mask_exceptions(THREAD_EXCP_ALL); > > > + > > > + cpu_spin_lock(&entropy_lock); > > > + > > > + if (types != TEE_PARAM_TYPES(TEE_PARAM_TYPE_MEMREF_INOUT, > > > + TEE_PARAM_TYPE_NONE, > > > + TEE_PARAM_TYPE_NONE, > > > + TEE_PARAM_TYPE_NONE)) { > > > + EMSG("bad parameters types: 0x%" PRIx32, types); > > > + return TEE_ERROR_BAD_PARAMETERS; > > > > Spin lock still held, interrupts still masked. > > > > > > > + } > > > + > > > + rq_size = params[0].memref.size; > > > + > > > + if ((rq_size == 0) || (rq_size > ENTROPY_POOL_SIZE)) > > > + return TEE_ERROR_NOT_SUPPORTED; > > > > Spin lock still held, interrupts still masked. > > > > > > > + > > > + e = (uint8_t *)params[0].memref.buffer; > > > + if (!e) > > > + return TEE_ERROR_BAD_PARAMETERS; > > > > Spin lock still held, interrupts still masked. > > > > Arguably we should even have taken the lock (or masked interrupts) until > > *after* we validate the input. > > > > Yes you are correct. As we don't touch global vars until validation of > input. I will move these spin lock and interrupts masking after > validation of input. > > > > > > + > > > + pool_size = entropy_size; > > > + > > > + if (pool_size < rq_size) { > > > + params[0].memref.size = pool_size; > > > + pool_get_entropy(e, pool_size); > > > > Shouldn't we be checking for continual health check failure here and > > providing a custom return code. > > > > Ok, I will add a custom error code here. > > > > > > + } else { > > > + params[0].memref.size = rq_size; > > > + pool_get_entropy(e, rq_size); > > > + } > > > + > > > + if (timer_fiq_running == false) { > > > + /* Enable timer FIQ to fetch entropy */ > > > + generic_timer_start(); > > > + timer_fiq_running = true; > > > + } > > > + > > > + cpu_spin_unlock(&entropy_lock); > > > + thread_set_exceptions(exceptions); > > > + > > > + return TEE_SUCCESS; > > > +} > > > + > > > +/* > > > + * Trusted Application Entry Points > > > + */ > > > +static TEE_Result open_session(uint32_t param_types __unused, > > > + TEE_Param params[TEE_NUM_PARAMS] __unused, > > > + void **session_context __unused) > > > +{ > > > + DMSG("open entry point for pseudo-TA \"%s\"", PTA_NAME); > > > + return TEE_SUCCESS; > > > +} > > > + > > > +static TEE_Result invoke_command(void *pSessionContext __unused, > > > + uint32_t nCommandID, uint32_t nParamTypes, > > > + TEE_Param pParams[TEE_NUM_PARAMS]) > > > +{ > > > + FMSG("command entry point for pseudo-TA \"%s\"", PTA_NAME); > > > > Is it normal for invoke_command to be a different trace level to > > open_session? > > > > I don't think there in any normal for trace levels to be used inside > Trusted Application. > > Here I have used FMSG (flow trace level = 4) instead of DMSG (debug > trace level = 3) in invoke_command for ease of debugging as > invoke_command is called much often as compared to open_session. > > > > > > + > > > + switch (nCommandID) { > > > + case PTA_CMD_GET_ENTROPY: > > > + return rng_get_entropy(nParamTypes, pParams); > > > + default: > > > + break; > > > + } > > > + > > > + return TEE_ERROR_NOT_IMPLEMENTED; > > > +} > > > + > > > +pseudo_ta_register(.uuid = PTA_RNG_UUID, .name = PTA_NAME, > > > + .flags = PTA_DEFAULT_FLAGS, > > > + .open_session_entry_point = open_session, > > > + .invoke_command_entry_point = invoke_command); > > > diff --git a/core/arch/arm/plat-synquacer/rng_pta.h b/core/arch/arm/plat-synquacer/rng_pta.h > > > new file mode 100644 > > > index 0000000..e238c57 > > > --- /dev/null > > > +++ b/core/arch/arm/plat-synquacer/rng_pta.h > > > @@ -0,0 +1,37 @@ > > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > > +/* > > > + * Copyright (C) 2018, Linaro Limited > > > + */ > > > + > > > +#ifndef __RNG_PTA_H > > > +#define __RNG_PTA_H > > > + > > > +#define PTA_NAME "rng.pta" > > > + > > > +#define THERMAL_SENSOR_BASE0 0x54190800 > > > +#define THERMAL_SENSOR_OFFSET 0x80 > > > +#define NUM_OF_SENSORS 7 > > > + > > > +#define TEMP_DATA_REG_OFFSET 0x34 > > > + > > > +#define ENTROPY_POOL_SIZE 4096 > > > + > > > +#define SENSOR_DATA_SIZE 128 > > > +#define CONDITIONER_PAYLOAD (SENSOR_DATA_SIZE * NUM_OF_SENSORS) > > > + > > > +#define LSB_MASK 0x1 > > > > This is not used anywhere. Should be removed (to be honest it should be > > removed anyway... its rather too close to something like #define FIVE 5 > > for my taste).. > > > > Ok. > > > > > > + > > > +#define MAX_BIT_FLIP_COUNT 48 > > > +#define MIN_BIT_FLIP_COUNT 16 > > > > Needs comment explaining how these values are arrived at. > > > > Ok. > > > > > > + > > > +#define THRESHOLD_REPORT_FAILURE 10 > > > > Are any of these used in other files? If not they can go in the rng_pta.c file. > > > > No they aren't used anywhere else. Will move them to rng_pta.c file. > > > > > > + > > > +extern bool timer_fiq_running; > > > + > > > +void rng_collect_entropy(void); > > > + > > > +void generic_timer_start(void); > > > +void generic_timer_stop(void); > > > +void generic_timer_handler(void); > > > > Why are the prototypes for the generic timer in rng_pta.h? > > > > Will create separate .h for generic timer. > > > > > > + > > > +#endif /* __RNG_PTA_H */ > > > diff --git a/core/arch/arm/plat-synquacer/rng_pta_client.h b/core/arch/arm/plat-synquacer/rng_pta_client.h > > > new file mode 100644 > > > index 0000000..ddd398c > > > --- /dev/null > > > +++ b/core/arch/arm/plat-synquacer/rng_pta_client.h > > > @@ -0,0 +1,22 @@ > > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > > +/* > > > + * Copyright (C) 2018, Linaro Limited > > > + */ > > > + > > > +#ifndef __RNG_PTA_CLIENT_H > > > +#define __RNG_PTA_CLIENT_H > > > + > > > +#define PTA_RNG_UUID { 0xab7a617c, 0xb8e7, 0x4d8f, \ > > > + { 0x83, 0x01, 0xd0, 0x9b, 0x61, 0x03, 0x6b, 0x64 } } > > > + > > > +/* > > > + * PTA_CMD_GET_ENTROPY - Get Entropy from RNG using Thermal Sensor > > > + * > > > + * param[0] (inout memref) - Entropy buffer memory reference > > > + * param[1] unused > > > + * param[2] unused > > > + * param[3] unused > > > > Knowing what the different possible return codes mean would be nice. > > > > Ok will add return codes here. > > > > > > + */ > > > +#define PTA_CMD_GET_ENTROPY 0x0 > > > + > > > +#endif /* __RNG_PTA_CLIENT_H */ > > > diff --git a/core/arch/arm/plat-synquacer/sub.mk b/core/arch/arm/plat-synquacer/sub.mk > > > index 8ddc2fd..013e57d 100644 > > > --- a/core/arch/arm/plat-synquacer/sub.mk > > > +++ b/core/arch/arm/plat-synquacer/sub.mk > > > @@ -1,2 +1,4 @@ > > > global-incdirs-y += . > > > srcs-y += main.c > > > +srcs-y += rng_pta.c > > > +srcs-y += timer_fiq.c > > > diff --git a/core/arch/arm/plat-synquacer/timer_fiq.c b/core/arch/arm/plat-synquacer/timer_fiq.c > > > new file mode 100644 > > > index 0000000..e25a676 > > > --- /dev/null > > > +++ b/core/arch/arm/plat-synquacer/timer_fiq.c > > > @@ -0,0 +1,43 @@ > > > +// SPDX-License-Identifier: BSD-2-Clause > > > +/* > > > + * Copyright (c) 2018, Linaro Limited > > > + */ > > > + > > > +#include <arm.h> > > > +#include <console.h> > > > +#include <drivers/gic.h> > > > +#include <io.h> > > > +#include <kernel/panic.h> > > > +#include <kernel/misc.h> > > > +#include <rng_pta.h> > > > + > > > +bool timer_fiq_running = false; > > > + > > > +void generic_timer_start(void) > > > +{ > > > + uint64_t cval; > > > + uint32_t ctl = 1; > > > + > > > + /* The timer will fire every 2 ms */ > > > + cval = read_cntpct() + (read_cntfrq() / 500); > > > + write_cntps_cval(cval); > > > + > > > + /* Enable the secure physical timer */ > > > + write_cntps_ctl(ctl); > > > +} > > > + > > > +void generic_timer_stop(void) > > > +{ > > > + /* Disable the timer */ > > > + write_cntps_ctl(0); > > > +} > > > + > > > +void generic_timer_handler(void) > > > +{ > > > + /* Ensure that the timer did assert the interrupt */ > > > + assert((read_cntps_ctl() >> 2)); > > > + > > > + /* Disable the timer and reprogram it */ > > > + write_cntps_ctl(0); > > > + generic_timer_start(); > > > +} > > > -- > > > 2.7.4 > > >
On Mon, Nov 19, 2018 at 01:39:36PM +0530, Sumit Garg wrote: > Isn't for a full entropy source two consecutive sample reading should > have following probable values? > > Val Probability > 11 -> 0.25 > 00 -> 0.25 > 01 -> 0.25 > 10 -> 0.25 > > So ideal probability of rising or falling edge should be 0.25. Ah... I see. The probability of a rising or falling edge is different to the probability of a bitflip. However I think its OK to do a health check based on the probability of edges but let's make sure we name things that way (might be better to rename the variables too, _rising instead of _0_1). > But if I set MAX limit to 0.5, I do see drop in health test failures. > > Also isn't 0.5 would be "10101010......1010101..." continuous sequence? Yes. It is impossible for the probability of an specific edge to ever be more 0.5 . > > > + if (health_test_fail_cnt >= THRESHOLD_REPORT_FAILURE) { > > > + IMSG("Health test failed %d times\n", > > > + health_test_fail_cnt); > > > + health_test_fail_cnt = 0; > > > > How will this result in bug reports (will anyone "normal" be observing > > this UART?). > > > > In case of Developerbox, UART is shared among normal and secure world. > There could be a daemon in normal world observing this kind of > messages on UART. > > Having said that it seems that reporting error code to consuming > application in case of health test failure to be more appropriate. Agree. The normal use-case for Developerbox is PC-like (nothing attached to *any* UART). > > > +static TEE_Result open_session(uint32_t param_types __unused, > > > + TEE_Param params[TEE_NUM_PARAMS] __unused, > > > + void **session_context __unused) > > > +{ > > > + DMSG("open entry point for pseudo-TA \"%s\"", PTA_NAME); > > > + return TEE_SUCCESS; > > > +} > > > + > > > +static TEE_Result invoke_command(void *pSessionContext __unused, > > > + uint32_t nCommandID, uint32_t nParamTypes, > > > + TEE_Param pParams[TEE_NUM_PARAMS]) > > > +{ > > > + FMSG("command entry point for pseudo-TA \"%s\"", PTA_NAME); > > > > Is it normal for invoke_command to be a different trace level to > > open_session? > > > > I don't think there in any normal for trace levels to be used inside > Trusted Application. > > Here I have used FMSG (flow trace level = 4) instead of DMSG (debug > trace level = 3) in invoke_command for ease of debugging as > invoke_command is called much often as compared to open_session. Well I can live with it... but since open_session() doesn't actually do anything perhaps just removing open_session() altogether is a good plan. Daniel.
On Mon, 19 Nov 2018 at 19:07, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Mon, Nov 19, 2018 at 01:39:36PM +0530, Sumit Garg wrote: > > Isn't for a full entropy source two consecutive sample reading should > > have following probable values? > > > > Val Probability > > 11 -> 0.25 > > 00 -> 0.25 > > 01 -> 0.25 > > 10 -> 0.25 > > > > So ideal probability of rising or falling edge should be 0.25. > > Ah... I see. > > The probability of a rising or falling edge is different to the > probability of a bitflip. However I think its OK to do a health check > based on the probability of edges but let's make sure we name things > that way (might be better to rename the variables too, _rising instead > of _0_1). > Sure will use "_rising" and "_falling" nomenclature. > > But if I set MAX limit to 0.5, I do see drop in health test failures. > > > > Also isn't 0.5 would be "10101010......1010101..." continuous sequence? > > Yes. It is impossible for the probability of an specific edge to ever > be more 0.5 . > > > > > > + if (health_test_fail_cnt >= THRESHOLD_REPORT_FAILURE) { > > > > + IMSG("Health test failed %d times\n", > > > > + health_test_fail_cnt); > > > > + health_test_fail_cnt = 0; > > > > > > How will this result in bug reports (will anyone "normal" be observing > > > this UART?). > > > > > > > In case of Developerbox, UART is shared among normal and secure world. > > There could be a daemon in normal world observing this kind of > > messages on UART. > > > > Having said that it seems that reporting error code to consuming > > application in case of health test failure to be more appropriate. > > Agree. The normal use-case for Developerbox is PC-like (nothing attached > to *any* UART). > > > > > > +static TEE_Result open_session(uint32_t param_types __unused, > > > > + TEE_Param params[TEE_NUM_PARAMS] __unused, > > > > + void **session_context __unused) > > > > +{ > > > > + DMSG("open entry point for pseudo-TA \"%s\"", PTA_NAME); > > > > + return TEE_SUCCESS; > > > > +} > > > > + > > > > +static TEE_Result invoke_command(void *pSessionContext __unused, > > > > + uint32_t nCommandID, uint32_t nParamTypes, > > > > + TEE_Param pParams[TEE_NUM_PARAMS]) > > > > +{ > > > > + FMSG("command entry point for pseudo-TA \"%s\"", PTA_NAME); > > > > > > Is it normal for invoke_command to be a different trace level to > > > open_session? > > > > > > > I don't think there in any normal for trace levels to be used inside > > Trusted Application. > > > > Here I have used FMSG (flow trace level = 4) instead of DMSG (debug > > trace level = 3) in invoke_command for ease of debugging as > > invoke_command is called much often as compared to open_session. > > Well I can live with it... but since open_session() doesn't actually do > anything perhaps just removing open_session() altogether is a good plan. > Actually OP-TEE OS requires trusted application to implement open_session() api. If it doesn't do anything then we need to provide a dummy function. -Sumit > > Daniel.
On Tue, Nov 20, 2018 at 10:03:14AM +0530, Sumit Garg wrote: > > > > > +static TEE_Result open_session(uint32_t param_types __unused, > > > > > + TEE_Param params[TEE_NUM_PARAMS] __unused, > > > > > + void **session_context __unused) > > > > > +{ > > > > > + DMSG("open entry point for pseudo-TA \"%s\"", PTA_NAME); > > > > > + return TEE_SUCCESS; > > > > > +} > > > > > + > > > > > +static TEE_Result invoke_command(void *pSessionContext __unused, > > > > > + uint32_t nCommandID, uint32_t nParamTypes, > > > > > + TEE_Param pParams[TEE_NUM_PARAMS]) > > > > > +{ > > > > > + FMSG("command entry point for pseudo-TA \"%s\"", PTA_NAME); > > > > > > > > Is it normal for invoke_command to be a different trace level to > > > > open_session? > > > > > > > > > > I don't think there in any normal for trace levels to be used inside > > > Trusted Application. > > > > > > Here I have used FMSG (flow trace level = 4) instead of DMSG (debug > > > trace level = 3) in invoke_command for ease of debugging as > > > invoke_command is called much often as compared to open_session. > > > > Well I can live with it... but since open_session() doesn't actually do > > anything perhaps just removing open_session() altogether is a good plan. > > > > Actually OP-TEE OS requires trusted application to implement > open_session() api. If it doesn't do anything then we need to provide > a dummy function. You sure about that? I checked the OP-TEE source code before my last reply and didn't observe with this. https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/pta/benchmark.c#L177 https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/kernel/pseudo_ta.c#L153 Daniel.
On Tue, 20 Nov 2018 at 16:30, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Tue, Nov 20, 2018 at 10:03:14AM +0530, Sumit Garg wrote: > > > > > > +static TEE_Result open_session(uint32_t param_types __unused, > > > > > > + TEE_Param params[TEE_NUM_PARAMS] __unused, > > > > > > + void **session_context __unused) > > > > > > +{ > > > > > > + DMSG("open entry point for pseudo-TA \"%s\"", PTA_NAME); > > > > > > + return TEE_SUCCESS; > > > > > > +} > > > > > > + > > > > > > +static TEE_Result invoke_command(void *pSessionContext __unused, > > > > > > + uint32_t nCommandID, uint32_t nParamTypes, > > > > > > + TEE_Param pParams[TEE_NUM_PARAMS]) > > > > > > +{ > > > > > > + FMSG("command entry point for pseudo-TA \"%s\"", PTA_NAME); > > > > > > > > > > Is it normal for invoke_command to be a different trace level to > > > > > open_session? > > > > > > > > > > > > > I don't think there in any normal for trace levels to be used inside > > > > Trusted Application. > > > > > > > > Here I have used FMSG (flow trace level = 4) instead of DMSG (debug > > > > trace level = 3) in invoke_command for ease of debugging as > > > > invoke_command is called much often as compared to open_session. > > > > > > Well I can live with it... but since open_session() doesn't actually do > > > anything perhaps just removing open_session() altogether is a good plan. > > > > > > > Actually OP-TEE OS requires trusted application to implement > > open_session() api. If it doesn't do anything then we need to provide > > a dummy function. > > You sure about that? > > I checked the OP-TEE source code before my last reply and didn't observe with this. > Ah.. I see. Thanks for pointing it out. Actually my perspective was set from user TA. I haven't checked it for pseudo TA. I assumed both to be similar. Ok then, I will remove this dummy open_session() api. Please do let me know if you have any further comment on v3 patch-set. -Sumit > https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/pta/benchmark.c#L177 > https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/kernel/pseudo_ta.c#L153 > > > Daniel.
diff --git a/core/arch/arm/include/arm64.h b/core/arch/arm/include/arm64.h index 2c1fd8c..0cf14c0 100644 --- a/core/arch/arm/include/arm64.h +++ b/core/arch/arm/include/arm64.h @@ -305,6 +305,10 @@ DEFINE_REG_READ_FUNC_(cntfrq, uint32_t, cntfrq_el0) DEFINE_REG_READ_FUNC_(cntpct, uint64_t, cntpct_el0) DEFINE_REG_READ_FUNC_(cntkctl, uint32_t, cntkctl_el1) DEFINE_REG_WRITE_FUNC_(cntkctl, uint32_t, cntkctl_el1) +DEFINE_REG_READ_FUNC_(cntps_ctl, uint32_t, cntps_ctl_el1) +DEFINE_REG_WRITE_FUNC_(cntps_ctl, uint32_t, cntps_ctl_el1) +DEFINE_REG_READ_FUNC_(cntps_cval, uint32_t, cntps_cval_el1) +DEFINE_REG_WRITE_FUNC_(cntps_cval, uint32_t, cntps_cval_el1) DEFINE_REG_READ_FUNC_(pmccntr, uint64_t, pmccntr_el0) diff --git a/core/arch/arm/plat-synquacer/main.c b/core/arch/arm/plat-synquacer/main.c index c3aac4c..7c3a6bc 100644 --- a/core/arch/arm/plat-synquacer/main.c +++ b/core/arch/arm/plat-synquacer/main.c @@ -18,6 +18,7 @@ #include <sm/optee_smc.h> #include <tee/entry_fast.h> #include <tee/entry_std.h> +#include <rng_pta.h> static void main_fiq(void); @@ -38,6 +39,7 @@ static struct pl011_data console_data; register_phys_mem(MEM_AREA_IO_NSEC, CONSOLE_UART_BASE, CORE_MMU_DEVICE_SIZE); register_phys_mem(MEM_AREA_IO_SEC, GIC_BASE, CORE_MMU_DEVICE_SIZE); +register_phys_mem(MEM_AREA_IO_SEC, THERMAL_SENSOR_BASE, CORE_MMU_DEVICE_SIZE); const struct thread_handlers *generic_boot_get_handlers(void) { @@ -46,7 +48,7 @@ const struct thread_handlers *generic_boot_get_handlers(void) static void main_fiq(void) { - panic(); + gic_it_handle(&gic_data); } void console_init(void) @@ -66,12 +68,38 @@ void main_init_gic(void) if (!gicd_base) panic(); - /* Initialize GIC */ - gic_init(&gic_data, 0, gicd_base); + /* On ARMv8-A, GIC configuration is initialized in TF-A */ + gic_init_base_addr(&gic_data, 0, gicd_base); + itr_init(&gic_data.chip); } -void main_secondary_init_gic(void) +static enum itr_return timer_itr_cb(struct itr_handler *h __unused) +{ + /* Reset timer for next FIQ */ + generic_timer_handler(); + + /* Collect entropy on each timer FIQ */ + rng_collect_entropy(); + + return ITRR_HANDLED; +} + +static struct itr_handler timer_itr = { + .it = IT_SEC_TIMER, + .flags = ITRF_TRIGGER_LEVEL, + .handler = timer_itr_cb, +}; + +static TEE_Result init_timer_itr(void) { - gic_cpu_init(&gic_data); + itr_add(&timer_itr); + itr_enable(IT_SEC_TIMER); + + /* Enable timer FIQ to fetch entropy required during boot */ + generic_timer_start(); + timer_fiq_running = true; + + return TEE_SUCCESS; } +driver_init(init_timer_itr); diff --git a/core/arch/arm/plat-synquacer/platform_config.h b/core/arch/arm/plat-synquacer/platform_config.h index 4d6d545..8a91ddb 100644 --- a/core/arch/arm/plat-synquacer/platform_config.h +++ b/core/arch/arm/plat-synquacer/platform_config.h @@ -19,6 +19,9 @@ #define CONSOLE_UART_CLK_IN_HZ 62500000 #define CONSOLE_BAUDRATE 115200 +#define THERMAL_SENSOR_BASE 0x54190000 +#define IT_SEC_TIMER 29 + #define DRAM0_BASE 0x80000000 /* Platform specific defines */ diff --git a/core/arch/arm/plat-synquacer/rng_pta.c b/core/arch/arm/plat-synquacer/rng_pta.c new file mode 100644 index 0000000..a760b54 --- /dev/null +++ b/core/arch/arm/plat-synquacer/rng_pta.c @@ -0,0 +1,270 @@ +// SPDX-License-Identifier: BSD-2-Clause +/* + * Copyright (C) 2018, Linaro Limited + */ + +#include <crypto/crypto.h> +#include <kernel/delay.h> +#include <kernel/pseudo_ta.h> +#include <kernel/spinlock.h> +#include <mm/core_memprot.h> +#include <io.h> +#include <string.h> +#include <rng_pta.h> +#include <rng_pta_client.h> + +static uint8_t entropy_pool[ENTROPY_POOL_SIZE] = {0}; +static uint32_t entropy_size; + +/* Current sensor data */ +static uint8_t sensor_data[NUM_OF_SENSORS][SENSOR_DATA_SIZE] = {0}; +static uint8_t s; + +/* Sensor data that passed health test */ +static uint8_t sensor_data_pass[NUM_OF_SENSORS][SENSOR_DATA_SIZE] = {0}; +static uint8_t num_sensors_pass; + +static uint8_t rest_data_pass[NUM_OF_SENSORS][SENSOR_DATA_SIZE] = {0}; + +static uint32_t health_test_fail_cnt; + +static unsigned int entropy_lock = SPINLOCK_UNLOCK; + +static void pool_add_entropy(uint8_t *entropy, uint32_t size) +{ + uint32_t copy_size; + + if (entropy_size >= ENTROPY_POOL_SIZE) + return; + + if ((ENTROPY_POOL_SIZE - entropy_size) >= size) + copy_size = size; + else + copy_size = ENTROPY_POOL_SIZE - entropy_size; + + memcpy((entropy_pool + entropy_size), entropy, copy_size); + + entropy_size += copy_size; +} + +static void pool_get_entropy(uint8_t *buf, uint32_t size) +{ + uint32_t off; + + if (size > entropy_size) + return; + + off = entropy_size - size; + + memcpy(buf, &entropy_pool[off], size); + entropy_size -= size; +} + +static bool health_test(uint8_t sensor_id) +{ + bool result = true; + uint8_t bit_flip_1_0 = 0, bit_flip_0_1 = 0; + uint8_t i; + + /* + * Heatlh test to check if count of bit flips 1-0 or 0-1 lies in 12.5% + * to 37.5% of 128 bytes raw data from particular sensor reading. In + * ideal scenario either of bit flips should be around 25%. + */ + for (i = 0; i < (SENSOR_DATA_SIZE - 1); i++) { + if ((sensor_data[sensor_id][i] ^ + sensor_data[sensor_id][i + 1]) & 0x1) { + bit_flip_1_0 += sensor_data[sensor_id][i] & 0x1; + bit_flip_0_1 += sensor_data[sensor_id][i + 1] & 0x1; + } + } + + if (bit_flip_1_0 > bit_flip_0_1) { + if (bit_flip_0_1 < MIN_BIT_FLIP_COUNT) + result = false; + if (bit_flip_1_0 > MAX_BIT_FLIP_COUNT) + result = false; + } else { + if (bit_flip_1_0 < MIN_BIT_FLIP_COUNT) + result = false; + if (bit_flip_0_1 > MAX_BIT_FLIP_COUNT) + result = false; + } + + return result; +} + +static void pool_check_add_entropy(void) +{ + uint32_t i; + uint8_t entropy_sha512_256[TEE_SHA256_HASH_SIZE]; + uint8_t rest_sensors_pass = 0; + TEE_Result res; + + for (i = 0; i < NUM_OF_SENSORS; i++) { + /* Check if particular sensor data passes health test */ + if (health_test(i) == true) { + if (num_sensors_pass < NUM_OF_SENSORS) { + memcpy(sensor_data_pass[num_sensors_pass], + sensor_data[i], SENSOR_DATA_SIZE); + num_sensors_pass++; + } else { + memcpy(rest_data_pass[rest_sensors_pass], + sensor_data[i], SENSOR_DATA_SIZE); + rest_sensors_pass++; + } + } else { + health_test_fail_cnt++; + /* + * Report health test failures if it exceeds certain + * threshold. + */ + if (health_test_fail_cnt >= THRESHOLD_REPORT_FAILURE) { + IMSG("Health test failed %d times\n", + health_test_fail_cnt); + health_test_fail_cnt = 0; + } + } + } + + /* Check if sensor_data_pass is full */ + if (num_sensors_pass == NUM_OF_SENSORS) { + /* + * Use vetted conditioner SHA512/256 as per + * NIST.SP.800-90B to condition raw data from entropy + * source. + * Here we have assumed that entropy source provides + * 4 bits per 7 sensor readings per sample. + * Also as per NIST.SP.800-90B, to get full entropy + * from vetted conditioner, we need to supply double of + * input entropy. So with full entropy (8 bits per byte) + * we will get yield as one byte of output data for + * every 28 sensor readings. + * For 32 bytes of SHA512/256 output data, we need to + * supply 896 bytes of raw input data. + */ + res = hash_sha512_256_compute(entropy_sha512_256, + (uint8_t *)sensor_data_pass, + CONDITIONER_PAYLOAD); + if (res == TEE_SUCCESS) + pool_add_entropy(entropy_sha512_256, + TEE_SHA256_HASH_SIZE); + } + + if (rest_sensors_pass) + memcpy((uint8_t *)sensor_data_pass, (uint8_t *)rest_data_pass, + (rest_sensors_pass * SENSOR_DATA_SIZE)); + + num_sensors_pass = rest_sensors_pass; +} + +void rng_collect_entropy(void) +{ + uint8_t i; + void *vaddr; + uint32_t exceptions = thread_mask_exceptions(THREAD_EXCP_ALL); + + cpu_spin_lock(&entropy_lock); + + for (i = 0; i < NUM_OF_SENSORS; i++) { + vaddr = phys_to_virt_io(THERMAL_SENSOR_BASE0 + + (THERMAL_SENSOR_OFFSET * i) + + TEMP_DATA_REG_OFFSET); + sensor_data[i][s] = (uint8_t)read32((vaddr_t)vaddr); + } + + s++; + + if (s >= SENSOR_DATA_SIZE) { + pool_check_add_entropy(); + s = 0; + } + + if (entropy_size >= ENTROPY_POOL_SIZE) { + generic_timer_stop(); + timer_fiq_running = false; + } + + cpu_spin_unlock(&entropy_lock); + thread_set_exceptions(exceptions); +} + +static TEE_Result rng_get_entropy(uint32_t types, + TEE_Param params[TEE_NUM_PARAMS]) +{ + uint8_t *e = NULL; + uint32_t pool_size = 0, rq_size = 0; + uint32_t exceptions = thread_mask_exceptions(THREAD_EXCP_ALL); + + cpu_spin_lock(&entropy_lock); + + if (types != TEE_PARAM_TYPES(TEE_PARAM_TYPE_MEMREF_INOUT, + TEE_PARAM_TYPE_NONE, + TEE_PARAM_TYPE_NONE, + TEE_PARAM_TYPE_NONE)) { + EMSG("bad parameters types: 0x%" PRIx32, types); + return TEE_ERROR_BAD_PARAMETERS; + } + + rq_size = params[0].memref.size; + + if ((rq_size == 0) || (rq_size > ENTROPY_POOL_SIZE)) + return TEE_ERROR_NOT_SUPPORTED; + + e = (uint8_t *)params[0].memref.buffer; + if (!e) + return TEE_ERROR_BAD_PARAMETERS; + + pool_size = entropy_size; + + if (pool_size < rq_size) { + params[0].memref.size = pool_size; + pool_get_entropy(e, pool_size); + } else { + params[0].memref.size = rq_size; + pool_get_entropy(e, rq_size); + } + + if (timer_fiq_running == false) { + /* Enable timer FIQ to fetch entropy */ + generic_timer_start(); + timer_fiq_running = true; + } + + cpu_spin_unlock(&entropy_lock); + thread_set_exceptions(exceptions); + + return TEE_SUCCESS; +} + +/* + * Trusted Application Entry Points + */ +static TEE_Result open_session(uint32_t param_types __unused, + TEE_Param params[TEE_NUM_PARAMS] __unused, + void **session_context __unused) +{ + DMSG("open entry point for pseudo-TA \"%s\"", PTA_NAME); + return TEE_SUCCESS; +} + +static TEE_Result invoke_command(void *pSessionContext __unused, + uint32_t nCommandID, uint32_t nParamTypes, + TEE_Param pParams[TEE_NUM_PARAMS]) +{ + FMSG("command entry point for pseudo-TA \"%s\"", PTA_NAME); + + switch (nCommandID) { + case PTA_CMD_GET_ENTROPY: + return rng_get_entropy(nParamTypes, pParams); + default: + break; + } + + return TEE_ERROR_NOT_IMPLEMENTED; +} + +pseudo_ta_register(.uuid = PTA_RNG_UUID, .name = PTA_NAME, + .flags = PTA_DEFAULT_FLAGS, + .open_session_entry_point = open_session, + .invoke_command_entry_point = invoke_command); diff --git a/core/arch/arm/plat-synquacer/rng_pta.h b/core/arch/arm/plat-synquacer/rng_pta.h new file mode 100644 index 0000000..e238c57 --- /dev/null +++ b/core/arch/arm/plat-synquacer/rng_pta.h @@ -0,0 +1,37 @@ +/* SPDX-License-Identifier: BSD-2-Clause */ +/* + * Copyright (C) 2018, Linaro Limited + */ + +#ifndef __RNG_PTA_H +#define __RNG_PTA_H + +#define PTA_NAME "rng.pta" + +#define THERMAL_SENSOR_BASE0 0x54190800 +#define THERMAL_SENSOR_OFFSET 0x80 +#define NUM_OF_SENSORS 7 + +#define TEMP_DATA_REG_OFFSET 0x34 + +#define ENTROPY_POOL_SIZE 4096 + +#define SENSOR_DATA_SIZE 128 +#define CONDITIONER_PAYLOAD (SENSOR_DATA_SIZE * NUM_OF_SENSORS) + +#define LSB_MASK 0x1 + +#define MAX_BIT_FLIP_COUNT 48 +#define MIN_BIT_FLIP_COUNT 16 + +#define THRESHOLD_REPORT_FAILURE 10 + +extern bool timer_fiq_running; + +void rng_collect_entropy(void); + +void generic_timer_start(void); +void generic_timer_stop(void); +void generic_timer_handler(void); + +#endif /* __RNG_PTA_H */ diff --git a/core/arch/arm/plat-synquacer/rng_pta_client.h b/core/arch/arm/plat-synquacer/rng_pta_client.h new file mode 100644 index 0000000..ddd398c --- /dev/null +++ b/core/arch/arm/plat-synquacer/rng_pta_client.h @@ -0,0 +1,22 @@ +/* SPDX-License-Identifier: BSD-2-Clause */ +/* + * Copyright (C) 2018, Linaro Limited + */ + +#ifndef __RNG_PTA_CLIENT_H +#define __RNG_PTA_CLIENT_H + +#define PTA_RNG_UUID { 0xab7a617c, 0xb8e7, 0x4d8f, \ + { 0x83, 0x01, 0xd0, 0x9b, 0x61, 0x03, 0x6b, 0x64 } } + +/* + * PTA_CMD_GET_ENTROPY - Get Entropy from RNG using Thermal Sensor + * + * param[0] (inout memref) - Entropy buffer memory reference + * param[1] unused + * param[2] unused + * param[3] unused + */ +#define PTA_CMD_GET_ENTROPY 0x0 + +#endif /* __RNG_PTA_CLIENT_H */ diff --git a/core/arch/arm/plat-synquacer/sub.mk b/core/arch/arm/plat-synquacer/sub.mk index 8ddc2fd..013e57d 100644 --- a/core/arch/arm/plat-synquacer/sub.mk +++ b/core/arch/arm/plat-synquacer/sub.mk @@ -1,2 +1,4 @@ global-incdirs-y += . srcs-y += main.c +srcs-y += rng_pta.c +srcs-y += timer_fiq.c diff --git a/core/arch/arm/plat-synquacer/timer_fiq.c b/core/arch/arm/plat-synquacer/timer_fiq.c new file mode 100644 index 0000000..e25a676 --- /dev/null +++ b/core/arch/arm/plat-synquacer/timer_fiq.c @@ -0,0 +1,43 @@ +// SPDX-License-Identifier: BSD-2-Clause +/* + * Copyright (c) 2018, Linaro Limited + */ + +#include <arm.h> +#include <console.h> +#include <drivers/gic.h> +#include <io.h> +#include <kernel/panic.h> +#include <kernel/misc.h> +#include <rng_pta.h> + +bool timer_fiq_running = false; + +void generic_timer_start(void) +{ + uint64_t cval; + uint32_t ctl = 1; + + /* The timer will fire every 2 ms */ + cval = read_cntpct() + (read_cntfrq() / 500); + write_cntps_cval(cval); + + /* Enable the secure physical timer */ + write_cntps_ctl(ctl); +} + +void generic_timer_stop(void) +{ + /* Disable the timer */ + write_cntps_ctl(0); +} + +void generic_timer_handler(void) +{ + /* Ensure that the timer did assert the interrupt */ + assert((read_cntps_ctl() >> 2)); + + /* Disable the timer and reprogram it */ + write_cntps_ctl(0); + generic_timer_start(); +}
This platform provides 7 on-chip thermal sensors accessible from secure world only. So, using randomness from these sensors we have created a True RNG as a pseudo TA. Signed-off-by: Sumit Garg <sumit.garg@linaro.org> --- core/arch/arm/include/arm64.h | 4 + core/arch/arm/plat-synquacer/main.c | 38 +++- core/arch/arm/plat-synquacer/platform_config.h | 3 + core/arch/arm/plat-synquacer/rng_pta.c | 270 +++++++++++++++++++++++++ core/arch/arm/plat-synquacer/rng_pta.h | 37 ++++ core/arch/arm/plat-synquacer/rng_pta_client.h | 22 ++ core/arch/arm/plat-synquacer/sub.mk | 2 + core/arch/arm/plat-synquacer/timer_fiq.c | 43 ++++ 8 files changed, 414 insertions(+), 5 deletions(-) create mode 100644 core/arch/arm/plat-synquacer/rng_pta.c create mode 100644 core/arch/arm/plat-synquacer/rng_pta.h create mode 100644 core/arch/arm/plat-synquacer/rng_pta_client.h create mode 100644 core/arch/arm/plat-synquacer/timer_fiq.c -- 2.7.4