Re: [PATCH v2 1/4] ARM: stm32: Fix ECDSA authentication with Dcache enabled

[Marek Vasut <marex@denx.de>]

On 12/7/22 11:08, Patrick DELAUNAY wrote:
> Hi Marek,

Hello Patrick,

> Sorry for the delay.

No worries.

> I cross-check with ROM code team to understood this API limitation.

Thank you!

> On 12/6/22 23:49, Marek Vasut wrote:
>> In case Dcache is enabled while the ECDSA authentication function is
>> called via BootROM ROM API, the CRYP DMA might pick stale version of
>> data from DRAM. Disable Dcache around the BootROM call to avoid this
>> issue.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
>> ---
>> V2: - Initialize reenable_dcache variable
>> ---
>>   arch/arm/mach-stm32mp/ecdsa_romapi.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/arch/arm/mach-stm32mp/ecdsa_romapi.c 
>> b/arch/arm/mach-stm32mp/ecdsa_romapi.c
>> index a2f63ff879f..082178ce83f 100644
>> --- a/arch/arm/mach-stm32mp/ecdsa_romapi.c
>> +++ b/arch/arm/mach-stm32mp/ecdsa_romapi.c
>> @@ -63,6 +63,7 @@ static int romapi_ecdsa_verify(struct udevice *dev,
>>                      const void *hash, size_t hash_len,
>>                      const void *signature, size_t sig_len)
>>   {
>> +    bool reenable_dcache = false;
>>       struct ecdsa_rom_api rom;
>>       uint8_t raw_key[64];
>>       uint32_t rom_ret;
>> @@ -81,8 +82,21 @@ static int romapi_ecdsa_verify(struct udevice *dev,
>>       memcpy(raw_key + 32, pubkey->y, 32);
>>       stm32mp_rom_get_ecdsa_functions(&rom);
>> +
>> +    /*
>> +     * Disable D-cache before calling into BootROM, else CRYP DMA
>> +     * may fail to pick up the correct data.
>> +     */
>> +    if (dcache_status()) {
>> +        dcache_disable();
>> +        reenable_dcache = true;
>> +    }
>> +
>>       rom_ret = rom.ecdsa_verify_signature(hash, raw_key, signature, 
>> algo);
>> +    if (reenable_dcache)
>> +        dcache_enable();
>> +
>>       return rom_ret == ROM_API_SUCCESS ? 0 : -EPERM;
>>   }
> 
> 
> In fact, the ecdsa_verify_signature() don't use the HW (no DMA and no 
> use of CRYP IP )

Hmmm, what does the BootROM use CRYP for then ?
It is necessary to have MP15xC/F for the authenticated boot to work, but 
it seems the only difference there is the presence of CRYP. Or is there 
some BootROM fuse too ?

> It is only a SW library, integrated in ROM code and exported to avoid 
> the need
> 
> to include the same library in FSBL = TF-A, with size limitation (SYSRAM).
> 
> 
> This library don't need to deactivate the data cache, the only impact of 
> this deactivation it
> 
> is to reduce the execution performance....
> 
> 
> After cross-check, I think the only problem today it the U-Boot MMU 
> configuration of STM32MP15x
> 
> plaform: by default only the DDR is marked executable in U-Boot, all the 
> other region are
> 
> defined as DEVICE memory/not executable (DCACHE_OFF in mmu_setup).
> 
> 
> Deactivate the data cache only avoids the exception which occurs on jump 
> to NotExecutable region
> 
> because in U-Boot "dcache OFF" imply  "MMU off"  (see cache_enable in 
> ./arch/arm/lib/cache-cp15.c)
> 
> and with MMU deactivated the check on executable MMU tag is also 
> deactivated.
> 
> 
> I think the next patch is enough:
> 
> 
> #define STM32MP_ROM_BASE        U(0x00000000)
> 
> 
> static int romapi_ecdsa_verify(struct udevice *dev,
>                      const void *hash, size_t hash_len,
>                      const void *signature, size_t sig_len)
>   {
>       struct ecdsa_rom_api rom;
>       uint8_t raw_key[64];
>       uint32_t rom_ret;
> @@ -81,8 +82,21 @@ static int romapi_ecdsa_verify(struct udevice *dev,
>       memcpy(raw_key + 32, pubkey->y, 32);
> 
>       stm32mp_rom_get_ecdsa_functions(&rom);
> +
> +    /* mark executable the exported ROM code function: */
> +    mmu_set_region_dcache_behaviour(STM32MP_ROM_BASE, MMU_SECTION_SIZE, 
> DCACHE_DEFAULT_OPTION);
> +
>       rom_ret = rom.ecdsa_verify_signature(hash, raw_key, signature, algo);
> 
>       return rom_ret == ROM_API_SUCCESS ? 0 : -EPERM;
>   }

This indeed works, tested and sent V3.

> Sorry again for the first review, not complete...

Thank you for checking !