libutils: fault_mitigation: fix panic on hash mismatch

[vsatoes]

When running a test with CFG_FAULT_MITIGATION=y and with a corrupted message, hash verification fails and panic TEE core:

F/TC:? 0 trace_syscall:149 syscall #40 (syscall_asymm_verify) E/TC:2 0 Panic at lib/libutils/ext/fault_mitigation.c:87 <___ftmn_callee_done_check> E/TC:2 0 TEE load address @ 0x43200000
E/TC:2 0 Call stack:
E/TC:2 0 0x4320a9f0 print_kernel_stack at optee-os/core/arch/arm/kernel/unwind_arm64.c:91 E/TC:2 0 0x432203fc __do_panic at optee-os/core/kernel/panic.c:26 (discriminator 32) E/TC:2 0 0x4327d324 ___ftmn_callee_done_check at optee-os/lib/libutils/ext/fault_mitigation.c:87 E/TC:2 0 0x43263aac __ftmn_callee_done_check at optee-os/lib/libutils/ext/include/fault_mitigation.h:349 E/TC:2 0 0x43258408 sw_crypto_acipher_rsassa_verify at optee-os/core/lib/libtomcrypt/rsa.c:669 E/TC:2 0 0x43247ecc syscall_asymm_verify at optee-os/core/tee/tee_svc_cryp.c:4420 E/TC:2 0 0x43206d18 scall_do_call at optee-os/core/arch/arm/kernel/arch_scall_a64.S:140 E/TC:2 0 0x43206798 thread_scall_handler at optee-os/core/arch/arm/kernel/thread.c:1115 E/TC:2 0 0x432043e8 el0_svc at optee-os/core/arch/arm/kernel/thread_a64.S:850

Function ftmn_set_check_res_memcmp() is currently used on the verification of RSA hash. When CFG_FAULT_MITIGATION flag is enabled, ftmn.check.res is set with the return value of the hash comparison. This can be 0, when hash matches, or another number, positive or negativen, when hash does not match.

For fault mitigation purposes, the value stored on ftmn.check.res is later compared with the result of the signature comparison, which can assume only two values : 1==valid or 0==invalid.

With that, ftmn_set_check_res_memcmp() should set ftmn.check.res either with 0, when hash matches, or with 1, when hash does not match. Fix ___ftmn_set_check_res_memcmp() to have this behavior. Note that, the fault mitigation check is done between ftmn.check.res and !*stat.

[jenswi-linaro]

I'm trying to cross-reference this but I can't make sense of the line number in the commit message. Which commit are you getting this stack trace from?

[vsatoes]

Hi @jenswi-linaro ,
Sorry about that, the stack was printed when I was running optee v3.21 (e8abbcf: Update CHANGELOG for 3.21.0)

[jenswi-linaro]

Got it, thanks.

I'm not so keen on changing the value saved by ftmn_set_check_res_memcmp(), while it may make sense in this context it might be utterly confusing in other cases.

How about (untested):

a/core/lib/libtomcrypt/src/pk/rsa/rsa_verify_hash.c
+++ b/core/lib/libtomcrypt/src/pk/rsa/rsa_verify_hash.c
@@ -180,6 +180,16 @@ int rsa_verify_hash_ex(const unsigned char *sig,            unsigned long  sigle
       inc1 = 1;
     }
 
+    if (!*stat) {
+      /*
+       * The call to ftmn_set_check_res_memcmp() above might have failed,
+       * since memcmp() may set any non-zero result force it 1 to match the
+       * check with FTMN_CALLEE_DONE_CHECK() below.
+       */
+      FTMN_SET_CHECK_RES_NOT_ZERO(&ftmn, FTMN_INCR1, 1);
+      inc1++;
+   }
+
 #ifdef LTC_CLEAN_STACK
     zeromem(out, outlen);
 #endif

[ydonghyuk]

Can anyone tell me what's going on with that panic?

I found the same panic,

With the patch provided by @jenswi-linaro , I only checked that vts passed.

[vsatoes]

Thanks for the suggestion @jenswi-linaro.

I agree this is a better way to handle the case where memcmp() sets a non-zero value.
I have also applied it and VTS test passed.

I updated the commit with your suggestion and the commit message based on that.

Thanks again

[jenswi-linaro]

Please remove the "[UPSTREAM] " part and replace "rtc" with "ltc" in the commit title.

[jenswi-linaro]

Looks good.

[jforissier]

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>