| Message ID | 20221206224929.33015-1-marex@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Patrice Chotard |
| Headers | show |
| Series | [v2,1/4] ARM: stm32: Fix ECDSA authentication with Dcache enabled | expand |
Hi Marek, Sorry for the delay. I cross-check with ROM code team to understood this API limitation. 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 ) 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; } Sorry again for the first review, not complete... Regards Patrick Reference in TF-A code: arm-trusted-firmware/plat/st/common/stm32mp_crypto_lib.c uint32_t verify_signature(uint8_t *hash_in, uint8_t *pubkey_in, uint8_t *signature, uint32_t ecc_algo) { int ret; ret = mmap_add_dynamic_region(STM32MP_ROM_BASE, STM32MP_ROM_BASE, STM32MP_ROM_SIZE_2MB_ALIGNED, MT_CODE | MT_SECURE); .... ret = auth_ops.verify_signature(hash_in, pubkey_in, signature, ecc_algo); .... mmap_remove_dynamic_region(STM32MP_ROM_BASE, STM32MP_ROM_SIZE_2MB_ALIGNED); return ret; }
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 !
Hi, On 12/7/22 20:32, Marek Vasut wrote: > 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 ? used for SSP = Secure Secret Provisioning https://wiki.st.com/stm32mpu/wiki/Secure_Secret_Provisioning_(SSP) > 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 ? Yes, the secure boot feature availability is indicated in the security field of the chip part number, for STM32MP13 and STM32MP15. - SSP is not supported - the associated authentication feature for secure boot is deactivated in ROM code => the key is burned/locked in OTP on these chips and checked by ROM code before to authenticate the FSBL ... > This indeed works, tested and sent V3. > >> Sorry again for the first review, not complete... > > Thank you for checking ! Regards Patrick
On 12/12/22 10:40, Patrick DELAUNAY wrote: > Hi, Hello Patrick [...] >> Hmmm, what does the BootROM use CRYP for then ? > > > used for SSP = Secure Secret Provisioning > > https://wiki.st.com/stm32mpu/wiki/Secure_Secret_Provisioning_(SSP) Oh, only this part, I see. >> 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 ? > > > Yes, the secure boot feature availability is indicated in the security > field of the chip part number, for STM32MP13 and STM32MP15. > > - SSP is not supported > > - the associated authentication feature for secure boot is deactivated > in ROM code > > > => the key is burned/locked in OTP on these chips > > and checked by ROM code before to authenticate the FSBL Thank you for the clarification, this is really useful.
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 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(+)