diff mbox series

[4/8] crypto: stm32/cryp - fix race condition

Message ID 20211029135454.4383-5-nicolas.toromanoff@foss.st.com (mailing list archive)
State Superseded
Delegated to: Herbert Xu
Headers show
Series STM32 CRYP driver: many fixes | expand

Commit Message

Nicolas Toromanoff Oct. 29, 2021, 1:54 p.m. UTC
Erase key before finalizing request.
Fixes: 9e054ec21ef8 ("crypto: stm32 - Support for STM32 CRYP crypto module")

Signed-off-by: Nicolas Toromanoff <nicolas.toromanoff@foss.st.com>
---
 drivers/crypto/stm32/stm32-cryp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Marek Vasut Oct. 29, 2021, 2:23 p.m. UTC | #1
On 10/29/21 3:54 PM, Nicolas Toromanoff wrote:
> Erase key before finalizing request.
> Fixes: 9e054ec21ef8 ("crypto: stm32 - Support for STM32 CRYP crypto module")

Can you be a bit more specific in your commit messages ? That applies to 
the entire patchset. It is absolutely impossible to tell what race is 
fixed here or why it is fixed by exactly this change. This applies to 
the entire series.

And while I am at it, does the CRYP finally pass at least the most basic 
kernel boot time crypto tests or does running those still overwrite 
kernel memory and/or completely crash or lock up the machine ?
Nicolas Toromanoff Oct. 29, 2021, 3:21 p.m. UTC | #2
On Fri, 29 Oct 2021, Marek Vasut wrote:

> On 10/29/21 3:54 PM, Nicolas Toromanoff wrote:
>> Erase key before finalizing request.
>> Fixes: 9e054ec21ef8 ("crypto: stm32 - Support for STM32 CRYP crypto 
>> module")
>
> Can you be a bit more specific in your commit messages ? That applies to the 
> entire patchset. It is absolutely impossible to tell what race is fixed here 
> or why it is fixed by exactly this change. This applies to the entire series.

I'll send a v2 with better commit messages.

for this specific patch:
We reset the saved key before the crypto_finalize_*() call. Otherwise a 
still pending crypto action could be ran with a wrong key = {0};

> And while I am at it, does the CRYP finally pass at least the most basic 
> kernel boot time crypto tests or does running those still overwrite kernel 
> memory and/or completely crash or lock up the machine ?

All extra tests (finally) pass.

With a kernel config :
   # CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not set
   CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y
   CONFIG_CRYPTO_DEV_STM32_CRYP=m

while(true) do ; modprobe stm32-cryp && modprobe -r stm32-cryp ; done

ran a whole day without a crash, nor a detected error.
Marek Vasut Oct. 29, 2021, 4:48 p.m. UTC | #3
On 10/29/21 5:21 PM, Nicolas Toromanoff wrote:
> On Fri, 29 Oct 2021, Marek Vasut wrote:
> 
>> On 10/29/21 3:54 PM, Nicolas Toromanoff wrote:
>>> Erase key before finalizing request.
>>> Fixes: 9e054ec21ef8 ("crypto: stm32 - Support for STM32 CRYP crypto 
>>> module")
>>
>> Can you be a bit more specific in your commit messages ? That applies 
>> to the entire patchset. It is absolutely impossible to tell what race 
>> is fixed here or why it is fixed by exactly this change. This applies 
>> to the entire series.
> 
> I'll send a v2 with better commit messages.
> 
> for this specific patch:
> We reset the saved key before the crypto_finalize_*() call. Otherwise a 
> still pending crypto action could be ran with a wrong key = {0};
> 
>> And while I am at it, does the CRYP finally pass at least the most 
>> basic kernel boot time crypto tests or does running those still 
>> overwrite kernel memory and/or completely crash or lock up the machine ?
> 
> All extra tests (finally) pass.
> 
> With a kernel config :
>    # CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not set
>    CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y
>    CONFIG_CRYPTO_DEV_STM32_CRYP=m

Can you also do a boot test with CRYP compiled into the kernel ?
I recall that is how the original bug was reported -- the machine 
crashed completely on boot even before reaching userspace, or the kernel 
crashed on memory corruption before reaching userspace.
diff mbox series

Patch

diff --git a/drivers/crypto/stm32/stm32-cryp.c b/drivers/crypto/stm32/stm32-cryp.c
index 6eeeca0d70ce..f97f9ee68d6f 100644
--- a/drivers/crypto/stm32/stm32-cryp.c
+++ b/drivers/crypto/stm32/stm32-cryp.c
@@ -666,6 +666,8 @@  static void stm32_cryp_finish_req(struct stm32_cryp *cryp, int err)
 		free_pages((unsigned long)buf_out, pages);
 	}
 
+	memset(cryp->ctx->key, 0, sizeof(cryp->ctx->key));
+
 	pm_runtime_mark_last_busy(cryp->dev);
 	pm_runtime_put_autosuspend(cryp->dev);
 
@@ -674,8 +676,6 @@  static void stm32_cryp_finish_req(struct stm32_cryp *cryp, int err)
 	else
 		crypto_finalize_skcipher_request(cryp->engine, cryp->req,
 						   err);
-
-	memset(cryp->ctx->key, 0, cryp->ctx->keylen);
 }
 
 static int stm32_cryp_cpu_start(struct stm32_cryp *cryp)