Error cases in syscall_asymm_operate() not trapped

[etienne-lms]

syscall_asymm_operate() can detect error conditions that are not reported, see #6135 (comment) and #6135 (comment).
First tests by @seonghp reports xtest failure when addressed (#6135 (comment)).
To be investigated.

[etienne-lms]

An obvious patch to fix the reported issue is the following:

--- a/core/tee/tee_svc_cryp.c
+++ b/core/tee/tee_svc_cryp.c
@@ -4245,25 +4245,25 @@
 			/*
 			 * If the optional TEE_ATTR_RSA_OAEP_MGF_HASH is
 			 * provided for algorithm
 			 * TEE_ALG_RSAES_PKCS1_OAEP_MGF1_x it must match
 			 * the internal hash x since we don't support using
 			 * a different hash for MGF1 yet.
 			 */
 			if (cs->algo != TEE_ALG_RSAES_PKCS1_V1_5 &&
 			    params[n].attributeID ==
 			    TEE_ATTR_RSA_OAEP_MGF_HASH) {
 				uint32_t hash = 0;
 
 				if (params[n].content.ref.length !=
 				    sizeof(hash)) {
 					res = TEE_ERROR_BAD_PARAMETERS;
- 					break;
+					goto out;
 				}
 				memcpy(&hash, params[n].content.ref.buffer,
 				       sizeof(hash));
 				if (hash !=
 				    TEE_INTERNAL_HASH_TO_ALGO(cs->algo)) {
 					res = TEE_ERROR_NOT_SUPPORTED;
- 					break;
+					goto out;
 				}
 			}

However, as reported by @seonghp such change leads to xtest regression_4006 failure. I happends that params[n].content.ref.buffer does not have the expected content, actually the buffer content may differ upon test execution, as if memory was not properly initialized despite regression_4000.c properly sets the buffer (line 4132 of tag 3.21.0):

			if (tv->algo == TEE_ALG_RSAES_PKCS1_OAEP_MGF1_SHA1) {
				algo_params[0].attributeID =
					TEE_ATTR_RSA_OAEP_MGF_HASH;
				algo_params[0].content.ref.length =
					sizeof(uint32_t);
				algo_params[0].content.ref.buffer =
					&(uint32_t){TEE_ALG_SHA1};            <----- HERE
				num_algo_params = 1;
			}

Note that the issue disappears when referred byffer is accessed before the TA is invoked, e.g.:

 			if (tv->algo == TEE_ALG_RSAES_PKCS1_OAEP_MGF1_SHA1) {
 				algo_params[0].attributeID =
 					TEE_ATTR_RSA_OAEP_MGF_HASH;
 				algo_params[0].content.ref.length =
 					sizeof(uint32_t);
 				algo_params[0].content.ref.buffer =
 					&(uint32_t){TEE_ALG_SHA1};
 				num_algo_params = 1;
+				fprintf(stderr, "TEE_ATTR_RSA_OAEP_MGF_HASH: hash 0x%x\n",
+					*(uint32_t *)algo_params[0].content.ref.buffer);
 			}

That said, changing regression_4000.c to explicitly use the stack as buffer reference fixes the issue:

 	(...)
+	uint32_t sha1_algo_id = TEE_ALG_SHA1;
 	(...)
 
 			if (tv->algo == TEE_ALG_RSAES_PKCS1_OAEP_MGF1_SHA1) {
 				algo_params[0].attributeID =
 					TEE_ATTR_RSA_OAEP_MGF_HASH;
 				algo_params[0].content.ref.length =
-					sizeof(uint32_t);
+					sizeof(sha1_algo_id);
 				algo_params[0].content.ref.buffer = 
-					&(uint32_t){TEE_ALG_SHA1};
+					&sha1_algo_id;
 				num_algo_params = 1;
 			}

I'll propose this fix in optee_test.

[etienne-lms]

Created #6144 and OP-TEE/optee_test#678 to address the issue.