PTA Remote Attestation #7006

kunisuzaki · 2024-08-26T09:21:19Z

As commented at isse #6921 "OP-TEE Remote Attestation with VERAISON Verification", I create a pull request.
I hope this pull request follows the mammer.

I will add the CA and TA for this PTA Remote Attestation, I will make a pull request at optee_examples .

jenswi-linaro

This pull request is quite large. At first glance, it looks like there's a bit of imported code. That should preferably go into separate commits to make the review easier.

I'll take care of the base64 functions in a separate commit.

core/pta/remote_attestation/base64.c

jenswi-linaro · 2024-08-26T10:20:31Z

Base64 stuff in #7007

kunisuzaki · 2024-08-27T22:13:30Z

Thank you for moving base64.c and base64.h from libutee to libutils. I appreciate the update.

Could you please advise on the next steps? Should we update our codebase solely to reflect this relocation, or are there other changes we should consider?

Thank you for your guidance.

jenswi-linaro · 2024-08-28T09:50:57Z

Please rebase on top of the latest to get the updated base64 code. While you do that please separate the imported code that you copied from somewhere into separate commits. For instance UsefulBuf.c and UsefulBuf.h obviously are copied from somewhere so they should go into a separate commit.

kunisuzaki · 2024-08-29T11:07:40Z

jenswi-linaro
Thank you for your suggestion.
We understand that we need to rebase and separate the commits. Please bear with us for a moment.

etienne-lms

Some coding style comments.
I think qcbor implementation should go to a dedicated library directory like core/lib/qcbor/

etienne-lms · 2024-09-02T07:40:13Z

core/pta/remote_attestation/cbor.c

@@ -0,0 +1,190 @@
+#include "cbor.h"


Missing license and copyright header files.
Ditto for some of the other new header and C source files.

etienne-lms · 2024-09-02T07:41:24Z

lib/libutee/include/pta_remote_attestation.h

+ *
+ * Return codes:
+ * TEE_SUCCESS
+ * TEE_ERROR_ACCESS_DENIED  - Caller is not a user space TA


Prefer Client instead of Caller

etienne-lms · 2024-09-02T07:45:27Z

core/pta/remote_attestation/qcbor/UsefulBuf.c

@@ -0,0 +1,426 @@
+/*==============================================================================


/* SPDX-License-Identifier: BSD-3-Clause */

kunisuzaki · 2024-09-30T07:14:37Z

We reorganized the structure of files and confirmed the binary is build correctly.
We also modified the license.

jenswi-linaro · 2024-09-30T09:28:17Z

We don't use merge commits in pull requests. Please cherry-pick or rebase the branch as needed to replace the merge commits. The commit messages need some work, see https://optee.readthedocs.io/en/latest/general/contribute.html#commit-messages.

Since you need to rewrite the history of your branch you need to use git push -f when pushing the updated branch.

kunisuzaki · 2024-10-02T01:58:53Z

We update the branch.

jenswi-linaro · 2024-10-07T07:20:53Z

This pull request adds almost 15000 lines of code. That's too much to review in one go, it must be split into smaller commits. While you have a few commits here, the first one is still almost 15000 lines of code. Sometimes we do large commits like "core: copy files from base repo https://github.com/iisec-suzaki/optee-ra", but that's when updating or importing upstream code that has already been reviewed in that project. In this case, I expect the TA to be added in steps, with different components in separate patches.

We have already suggested that UsefulBuf.c and UsefulBuf.h and qcbor implementation should go into separate commits.

kunisuzaki · 2024-10-10T05:41:41Z

We separated the commit for adding the QCBOR-related libraries and the commit for adding the files necessary for PTA remote attestation. Most of this PR is copied from the QCBOR repository.

jforissier

Hello @kunisuzaki,

Some comments on the QCBOR import.

I'd like to minimize the differences with upstream, so that updates are easier. Please use the same directory structure as upstream, under core/lib/qcbor/, that is:
core/lib/qcbor/src/*
core/lib/qcbor/inc/*
...and drop commit "core/lib/qcbor: adjust include paths for QCBOR library files".
Since you do not import all files, please mention in the commit description the commands used to copy from the upstream library into core/lib/qcbor. We probably want only the src and include directory as well as the LICENSE file. Please see b60fc42a for an example.
Finally, please use subject "core: import QCBOR library" and give the URL in the description.

jforissier · 2024-10-10T07:56:54Z

core/pta/remote_attestation/cbor.c

@@ -1,3 +1,8 @@
+// SPDX-License-Identifier: BSD-2-Clause
+/*
+ * Copyright (C) 2024, Insitite of Information Security (IISEC)


Institute (and in other places too)

core/lib/qcbor/sub.mk

mk/config.mk

kunisuzaki · 2024-10-15T01:56:25Z

Hello @jforissier
Thank you for your comments.
We follow your suggestions and update the branch.

jforissier · 2024-10-15T08:32:17Z

For "ci: build with CFG_VERAISON_ATTESTATION_PTA=y":

Reviewed-by: Jerome Forissier <[email protected]>

jenswi-linaro · 2024-10-15T13:25:51Z

For "core: import QCBOR library", I've reproduced the commit with an identical result.
Acked-by: Jens Wiklander <[email protected]>

jenswi-linaro

Comments for "core/lib/qcbor: add build configuration for QCBOR library"

jenswi-linaro · 2024-10-15T13:31:31Z

core/lib/qcbor/sub.mk

+cflags-y += -Wno-declaration-after-statement
+cflags-y += -Wno-redundant-decls
+cflags-y += -DQCBOR_DISABLE_FLOAT_HW_USE
+cflags-remove-y += -mgeneral-regs-only


Why is this line needed?

To ignore these error.

core/lib/qcbor/src/ieee754.c: In function 'IEEE754_HalfToDouble': core/lib/qcbor/src/ieee754.c:212:1: error: '-mgeneral-regs-only' is incompatible with the use of floating-point types 212 | IEEE754_HalfToDouble(uint16_t uHalfPrecision) | ^~~~~~~~~~~~~~~~~~~~ core/lib/qcbor/src/ieee754.c: In function 'IEEE754_AssembleDouble': core/lib/qcbor/src/ieee754.c:197:1: error: '-mgeneral-regs-only' is incompatible with the use of floating-point types 197 | IEEE754_AssembleDouble(uint64_t uDoubleSign, | ^~~~~~~~~~~~~~~~~~~~~~

This will not work as intended since floating-point is not enabled in OP-TEE core. There are exceptions with VFP in for example https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/crypto/sha256_armv8a_ce.c, but that's only to allow assembly code use SIMD and floating-point registers. If you don't need floating-point conversion, we should disable it.

jenswi-linaro · 2024-10-15T13:31:51Z

core/lib/qcbor/sub.mk

+global-incdirs-y += inc
+cflags-y += -Wno-declaration-after-statement
+cflags-y += -Wno-redundant-decls
+cflags-y += -DQCBOR_DISABLE_FLOAT_HW_USE


Please use cppflags-y instead.

jenswi-linaro · 2024-10-15T14:08:28Z

I wonder if it wouldn't be easier to review this if the following commits were squashed into one.

core/pta/remote_attestation: add license information
core/pta/remote_attestation: apply patch from #6195 for region validation
core/pta/remote_attestation: update for libmbedtls API changes in #6151
core/pta/remote_attestation: replace custom base64 with library from #7007
core/pta/remote_attestation: rename configuration option for remote attestation PTA
core/pta/remote_attestation: add build configuration for remote attestation PTA
core/pta/remote_attestation: clean up QCBOR integration
core/pta/remote_attestation: copy remote attestation files from optee-ra

The diff is smaller and it's code that's supposed to work without further modifications. It's not a big deal, but the code churn may distract the review a bit.

kunisuzaki · 2024-10-16T03:54:08Z

Hello @jenswi-linaro,
We follow your suggestions and update the branch.

etienne-lms

Many coding style comments and few more interesting comments.

For the remote attestation PTA, since it is specific to the Veraison scheme, I think think it should be named Veraison, as usedd in the config switch CFG_VERAISON_ATTESTATION_PTA
core/pta/veraison_attestation/
lib/libutee/include/pta_veraison_attestation.h

Coul you update scripts/checkpatch_inc.sh to exclude

 CHECKPATCH_IGNORE=$(echo \
                 core/include/gen-asm-defines.h \
                 (...)
                 core/arch/arm/dts \
                 ta/pkcs11/scripts/verify-helpers.sh \
+                core/lib/qcbor \
                 core/arch/riscv/include/encoding.h )

etienne-lms · 2024-10-16T13:32:36Z

core/lib/qcbor/sub.mk

+cflags-y += -Wno-redundant-decls
+cppflags-y += -DQCBOR_DISABLE_FLOAT_HW_USE
+cppflags-y += -DQCBOR_DISABLE_PREFERRED_FLOAT
+cppflags-y += -DUSEFULBUF_DISABLE_ALL_FLOAT


Could you change cflags-y to cflags-lib-y so that they apply to qcbor library only?

For the cppflags-lib-y, the directives seems needed in the header files which the PTA include so I guess they should be global.

cflags-y only affects the current sub.mk file, while cflags-lib-y affects all the sub.mk used to build a lib.

Do we need these cppflags-y to be used outside of the qcbor library?

cflags-y only affects the current sub.mk file, while cflags-lib-y affects all the sub.mk used to build a lib.

Thanks for the details. Indeed chaning from cflags-y to cflags-lib-y does not chagne the build. Indeed, this sub.mk is the root one for this lib (qcbor) so they are equivalent here.

Do we need these cppflags-y to be used outside of the qcbor library?

I think they do. The directives defined with-D are used in some header files (core/lib/qcbor/inc/qcbor/*.h) indirectly included from core/pta/remote_attestation/{cbor.c|remote_attestation.c}.

It looks like -DUSEFULBUF_DISABLE_ALL_FLOAT should be enough, judging from qcbor/qcbor_private.h.

We don't have a global-cppflags-y, so either we need to implement that or set add it to cppflags$(sm) on the line after ifeq ($(CFG_QCBOR),y) in core/core.mk.

I can create a PR to add global-cppflags-y support for sub.mk files if that's what we want.

+1 for global-cpp-flags-y

#7085
Feel free to cherry-pick the patch into this PR or rebase this PR on top of #7085

Finally, what should we change in this PR?

etienne-lms · 2024-10-16T13:34:49Z

core/pta/remote_attestation/cbor.h

+ */
+
+#ifndef PTA_REMOTE_ATTESTATION_TA_CBOR_H
+#define PTA_REMOTE_ATTESTATION_TA_CBOR_H


Prefer PTA_REMOTE_ATTESTATION_CBOR_H| (wihtout TA_) to better match the header file name.
Ditto in hash.h and sign.h.

etienne-lms · 2024-10-16T13:39:31Z

core/pta/remote_attestation/hash.h

+#ifndef PTA_REMOTE_ATTESTATION_TA_HASH_H
+#define PTA_REMOTE_ATTESTATION_TA_HASH_H
+
+#include <tee_api_types.h>


prefer with explicit inclusion of stddef.h (for size_t) and stdint.h (for uint8_t)

etienne-lms · 2024-10-16T13:40:23Z

core/pta/remote_attestation/remote_attestation.c

+/* clang-format on */
+
+static TEE_Result cmd_get_cbor_evidence(uint32_t param_types,
+                                        TEE_Param params[TEE_NUM_PARAMS]) {


Brace at a new line, for coding style:

static TEE_Result cmd_get_cbor_evidence(uint32_t param_types, TEE_Param params[TEE_NUM_PARAMS]) {

This comment applies to all functions in core/pta/remote_attestation/*.c files.

etienne-lms · 2024-10-16T13:42:36Z

core/pta/remote_attestation/remote_attestation.c

+    const int psa_security_lifecycle = LIFECYCLE;
+    const char measurement_type[] = MEASURMENT_TYPE;
+    const uint8_t signer_id[SIGNER_ID_LEN] = {SIGNER_ID};
+    const uint8_t psa_instance_id[INSTANCE_ID_LEN] = {INSTANCE_ID};


for coding style: add a space char after opening brace and before closing brace:
const uint8_t signer_id[SIGNER_ID_LEN] = { SIGNER_ID };

Also applies to lines 57, 59 and 61.

etienne-lms · 2024-10-16T15:06:26Z

core/pta/remote_attestation/hash.c

+        return TEE_ERROR_ACCESS_DENIED;
+
+    /* The output buffer must be large enough to hold the hash. */
+    if (out_sz < TEE_SHA256_HASH_SIZE)


Nitpicking: this could be removed by expecting a well sized buffer:
`TEE_Result get_hash_ta_memory(uint8_t out[TEE_SHA256_HASH_SIZE]);

etienne-lms · 2024-10-16T15:16:54Z

core/lib/qcbor/inc/qcbor/UsefulBuf.h

+   UBC.ptr = UB.ptr;
+   UBC.len = UB.len;
+
+   return UBC;


What is the scope of such a local variable that in defined in an inline function?
Stack allocated for UBC is released when caller function block is terminated?

I did not write it, we will have to read the library to know the details.

etienne-lms · 2024-10-16T15:26:17Z

core/lib/qcbor/inc/qcbor/UsefulBuf.h

@@ -0,0 +1,2506 @@
+/* =========================================================================


Would it be possible to add an SPDX-License-Identifier tag for each source and header files in core/lib/qcbor/ . Maybe this can be addressed in a specific commit so that commit "core: import QCBOR library" dumps the files as found in the referred source stated in the commit message, without any such local changes.

Should I handle the addition of licence headers in a separate commit from "core: import QCBOR library"?

Yes, preferably I think but that's not a strong opinion.

Yes, please. It makes it easier for the library to be updated in the future.

etienne-lms · 2024-10-16T15:41:30Z

core/pta/remote_attestation/sub.mk

@@ -0,0 +1,4 @@
+srcs-$(CFG_VERAISON_ATTESTATION_PTA) += remote_attestation.c


This sub.mk file needs cflags-y += -Wno-declaration-after-statement and cflags-y += -Wno-redundant-decls.

etienne-lms · 2024-10-16T15:50:13Z

core/pta/remote_attestation/cbor.c

+                        size_t signer_id_len, const uint8_t *psa_instance_id,
+                        size_t psa_instance_id_len, const uint8_t *psa_nonce,
+                        size_t psa_nonce_len, const uint8_t *measurement_value,
+                        size_t mv_len, UsefulBuf cbor_evidence_buffer) {


Coding style:

UsefulBufC encode_evidence_to_cbor(const char *eat_profile, const int psa_client_id, (...) size_t mv_len, UsefulBuf cbor_evidence_buffer) {

and use tabulation for indentation.

etienne-lms · 2024-10-20T10:19:51Z

core/pta/remote_attestation/hash.c

+
+	/*
+     * Make an array of region pointers so we can use qsort() to order it.
+     */


indentation

We are using clang-format in this repository to format my code, but it doesn't seem to be working properly.

OP-TEE OS is not using clang-format. Rather a Linux kernel like format. Could it be possible to adhere to that format, for OP-TEE OS specific source/header files?

Regarding libraries whose source/header files are copied into OP-TEE OS source tree (like qcbor), it's fine preserving the original library format/coding-style. Just ensure CHECKPATCH_IGNORE in scripts/checkpatch_inc.sh contains the lib base path so that checkpatch does not emit false positive reports.

etienne-lms · 2024-10-20T10:20:48Z

core/pta/remote_attestation/hash.c

+		if (is_region_valid(r))
+			nregions++;
+
+	regions = malloc(nregions * sizeof(*regions));


prefer calloc(nregions, sizeof(*regions))

etienne-lms · 2024-10-20T10:21:05Z

core/pta/remote_attestation/hash.c

+	/*
+     * Sort regions so that they are in a consistent order even when TA ASLR
+     * is enabled.
+     */


etienne-lms · 2024-10-20T10:22:54Z

core/pta/remote_attestation/remote_attestation.c

+#include "hash.h"
+#include <base64.h>
+#include <stdlib.h>
+#include <string.h>


To following OP-TEE OS coding style:

#include <base64.h> #include <kernel/pseudo_ta.h> #include <pta_remote_attestation.h> #include <stdlib.h> #include <string.h> #include "cbor.h" #include "hash.h"

etienne-lms · 2024-10-20T10:23:45Z

core/pta/remote_attestation/remote_attestation.c

+
+/* clang-format off */
+#define SIGNER_ID                                      \
+    0xac, 0xbb, 0x11, 0xc7, 0xe4, 0xda, 0x21, 0x72,    \


could you fix indentation for lines 30 to 39?

etienne-lms · 2024-10-21T06:59:37Z

core/pta/remote_attestation/remote_attestation.c

+	}
+
+	/* Sign the CBOR and generate a COSE evidence */
+	UsefulBuf_MAKE_STACK_UB(buffer_for_cose, *output_buffer_len);


Please have a look ay my comment #7006 (comment).

etienne-lms · 2024-10-21T07:00:39Z

core/pta/remote_attestation/cbor.c

+	QCBOREncode_AddBytes(&context, payload);
+	QCBOREncode_CloseArray(&context);
+
+	UsefulBufC tbs;


Add an initialization value for each local variable that misses one + add an empty line below the local last variable definition.
This comment applies also to other functions in this source file.

etienne-lms · 2024-10-21T07:16:01Z

core/lib/qcbor/inc/qcbor/UsefulBuf.h

@@ -0,0 +1,2506 @@
+/* =========================================================================


Yes, preferably I think but that's not a strong opinion.

etienne-lms · 2024-10-21T07:22:58Z

core/pta/remote_attestation/sign.c

+static struct ecc_public_key *pubkey = NULL;
+
+/*
+FIXME: Currently, keys are directly embedded within the code. From a security


I think all config switches should be defined (with an explicit inline description comment) and have a default value in mk/config/mk, next to CFG_VERAISON_ATTESTATION_PTA.

etienne-lms · 2024-10-21T07:36:38Z

core/pta/remote_attestation/cbor.c

+	/* Close top level array for COSE_Sign1 */
+	QCBOREncode_CloseArray(&cose_context);
+
+	UsefulBufC signed_cose;


Could you move this definition (and those at lines 119, 120, 128, 129, 137) at instruction block entry?

mmxsrup · 2024-10-30T01:34:33Z

@etienne-lms We follow your suggestions and update the branch.

jforissier

For commit "core: import QCBOR library":
Acked-by: Jerome Forissier <[email protected]>
For commit "core/lib/qcbor: add build configuration for QCBOR library":
Reviewed-by: Jerome Forissier <[email protected]>
For commit "core/lib/qcbor: add SPDX license identifiers to QCBOR files":
Acked-by: Jerome Forissier <[email protected]>
I would prefer you reorder this commit just after "core: import QCBOR library".
For commit "core/pta/remote_attestation: integrate PTA remote attestation":
Could you please rewrite the description to use the imperative mood? "Copy PTA... Add build configuration... Replace the custom based implementation... etc.".

Thanks!

mmxsrup · 2024-11-16T04:20:59Z

@jforissier I added a Reviewed-by/Acked-by and updated the commit message.

mmxsrup · 2024-11-18T04:14:25Z

@etienne-lms Thanks a lot for your review. How should I put your Reviewed-by/Acked-by?

jenswi-linaro

Acked-by: Jens Wiklander <[email protected]>

jenswi-linaro · 2024-11-18T08:13:00Z

core/pta/veraison_attestation/sub.mk

+srcs-$(CFG_VERAISON_ATTESTATION_PTA) += hash.c
+srcs-$(CFG_VERAISON_ATTESTATION_PTA) += sign.c
+
+cflags-y += -Wno-declaration-after-statement


It's a pity we need to disable these two warnings because of the QCBOR header files.
It's out of the scope of this PR to fix that, but in the longer term, we should look into how to temporarily disable warnings in a header file.

That should be cflags-$(CFG_VERAISON_ATTESTATION_PTA) += ... and same for the next line

jforissier

For " core/pta/remote_attestation: integrate PTA remote attestation":

"remote attestation PTA", not "PTA remote attestation".
Please see my comments below
With the above addressed, please add Acked-by: Jerome Forissier <[email protected]>

jforissier · 2024-11-18T08:25:00Z

core/pta/veraison_attestation/sub.mk

+srcs-$(CFG_VERAISON_ATTESTATION_PTA) += hash.c
+srcs-$(CFG_VERAISON_ATTESTATION_PTA) += sign.c
+
+cflags-y += -Wno-declaration-after-statement


That should be cflags-$(CFG_VERAISON_ATTESTATION_PTA) += ... and same for the next line

jforissier · 2024-11-18T08:26:12Z

core/pta/veraison_attestation/sub.mk

+
+ifeq ($(CFG_VERAISON_ATTESTATION_PTA_TEST_KEY),y)
+cppflags-y += -DCFG_VERAISON_ATTESTATION_PTA_TEST_KEY
+endif


More simply written as cppflags-$(CFG_VERAISON_ATTESTATION_PTA_TEST_KEY) += -DCFG_VERAISON_ATTESTATION_PTA_TEST_KEY

jforissier · 2024-11-18T08:42:06Z

One more thing about " core/pta/remote_attestation: integrate PTA remote attestation": the path is now incorrect. I think the subject should be "core/pta/veraison_attestation: integrate Veraison remote attestation PTA"

etienne-lms

Acked-by: Etienne Carriere <[email protected]> (with minor comment addressed) for commits

"core: import QCBOR library"
with s/Imports/Import/ in commit message
"core/lib/qcbor: add SPDX license identifiers to QCBOR files"
with s/This commit adds/Add/ in commit message,
with s/core/lib/qcbor/core: lib: qcbor:/ in header line;
"core/lib/qcbor: add build configuration for QCBOR library"
with s/This commit adds/Add/ and s/Updates/Update/ in commit message,
with s/core/lib/qcbor/core: lib: qcbor:/ in header line;
"ci: build with CFG_VERAISON_ATTESTATION_PTA=y".

Few comments for commit "core/pta/veraison_attestation: integrate Veraison remote attestation PTA".

etienne-lms · 2024-11-18T09:55:46Z

core/pta/veraison_attestation/cbor.c

+}
+
+static void encode_protected_header_wrapper(QCBOREncodeContext *context,
+					    void *args)


prefer use __unused here instead of line 135.
(and add #include <compiler.h>)

etienne-lms · 2024-11-18T10:05:13Z

core/pta/veraison_attestation/cbor.c

+					  ubc_cbor_evidence);
+	if (UsefulBuf_IsNULLC(tbs_payload)) {
+		DMSG("Failed to encode to-be-signed payload");
+		mempool_free(mempool_default, (void *)protected_header.ptr);


Nitpicking maybe: for consistency, it would be nice to add a release_encoded_buffer() helper function (that calls mempool_free()) next to build_encoded_buffer() definition so that it is simpler to find the matching mempool_alloc()/mempool_free() calls.

etienne-lms · 2024-11-18T10:17:45Z

core/pta/veraison_attestation/hash.h

+#define PTA_VERAISON_ATTESTATION_HASH_H
+
+#include <stddef.h>
+#include <stdint.h>


#include <tee_api.h>

etienne-lms · 2024-11-18T10:23:02Z

core/pta/veraison_attestation/sign.c

+
+static void free_keypair(void)
+{
+	assert(key && key->d && key->x && key->y);


The 3 last assertion will fail when called from generate_key() error path.
Prefer something like:

- assert(key && key->d && key->x && key->y); + if(!key) + return;

Ditto for free_pubkey().

etienne-lms · 2024-11-18T10:24:00Z

core/pta/veraison_attestation/sign.c

+	/* Allocate the key pair*/
+	res = generate_key();
+	if (res != TEE_SUCCESS)
+		goto out;


nitpicking: return res is simpler here

etienne-lms · 2024-11-18T10:25:31Z

core/pta/veraison_attestation/sub.mk

+cflags-$(CFG_VERAISON_ATTESTATION_PTA) += -Wno-declaration-after-statement
+cflags-$(CFG_VERAISON_ATTESTATION_PTA) += -Wno-redundant-decls
+
+cppflags-$(CFG_VERAISON_ATTESTATION_PTA_TEST_KEY) += -DCFG_VERAISON_ATTESTATION_PTA_TEST_KEY


The above line is not needed. CFG_VERAISON_ATTESTATION_PTA_TEST_KEY macro is already defined for C sources build directive when the config switch is enabled.

Good catch, I missed that

etienne-lms · 2024-11-18T10:30:18Z

core/pta/veraison_attestation/veraison_attestation.c

+	/* Encode measurement_value to base64 */
+	if (base64_enc(measurement_value, TEE_SHA256_HASH_SIZE,
+		       b64_measurement_value,
+		       &b64_measurement_value_len) != 1) {


base64_en() returns a bool value. Prefer test against a bool value here:
if (!base64_enc(...)) { error case ... }

etienne-lms · 2024-11-18T10:32:20Z

core/pta/veraison_attestation/veraison_attestation.c

+		DMSG("Failed to encode measurement_value to base64");
+		return TEE_ERROR_GENERIC;
+	}
+	b64_measurement_value[b64_measurement_value_len] = '\0';


Terminal nul byte is already set by base64_enc().

etienne-lms · 2024-11-18T10:38:05Z

core/pta/veraison_attestation/veraison_attestation.c

+free_ubc_cose_evidence:
+	mempool_free(mempool_default, (void *)ubc_cose_evidence.ptr);
+free_ubc_cbor_evidence:
+	mempool_free(mempool_default, (void *)ubc_cbor_evidence.ptr);


I think calling here some release_cose_evidence()/release_cbor_evidence() helper function (or a single release_encoded_buffer() helper function) would help mantainaince of the allocation scheme use in this PTA.

etienne-lms · 2024-11-18T10:41:04Z

lib/libutee/include/pta_veraison_attestation.h

+ * TEE_ERROR_ACCESS_DENIED   - Caller is not a user space TA
+ * TEE_ERROR_BAD_PARAMETERS  - Incorrect input param
+ * TEE_ERROR_SHORT_BUFFER    - Output buffer size less than required
+ * TEE_ERROR_NOT_IMPLEMENTED - Command not implemented


There are few other return codes this PTA can report.
Maybe replace Return codes with Main return codes at line 24?

etienne-lms · 2024-11-18T14:30:07Z

core/pta/veraison_attestation/sign.c

+
+static void free_pubkey(void)
+{
+	if(!key)


s/key/pubkey/

etienne-lms · 2024-11-18T14:30:46Z

core/pta/veraison_attestation/sign.c

+	key = calloc(1, sizeof(*key));
+	if (!key) {
+		return TEE_ERROR_OUT_OF_MEMORY;
+	}


nitpicking: remove braces as per OP-TEE OS coding style

etienne-lms

Acked-by: Etienne Carriere <[email protected]> for commit
"core/pta/veraison_attestation: integrate Veraison remote attestation PTA"
with commit header line fixed:
pta: veraison_attestation: integrate Veraison remote attestation PTA

I think it would be worth to explicitly mention in mk/config.mk that CFG_VERAISON_ATTESTATION_PTA is an experimental feature even if it's somewhat already the case since CFG_VERAISON_ATTESTATION_PTA_TESTKEY is mandated.

mmxsrup · 2024-11-20T11:57:31Z

I did apply some additional fixes. Is there anything else that needs to be done?

etienne-lms · 2024-11-20T13:25:34Z

@mmxsrup, it's not easy to find what you've modified in the latest version. It would have been easier for us if you had appended some fixup commits on top of the already reviewed series so we can check and ack' them.
That said, the series looks good to me.

mmxsrup · 2024-11-21T07:23:25Z

@etienne-lms Sorry, Thank you for your cooperation.
@jforissier @jenswi-linaro Is there anything else that needs to be done?

jforissier · 2024-11-21T12:41:38Z

@jenswi-linaro should your Ack be applied to all commits? Thanks.

jenswi-linaro · 2024-11-21T14:12:31Z

@jenswi-linaro should your Ack be applied to all commits? Thanks.

Yes, please.

Import QCBOR v1.4.1 from https://github.com/laurencelundblade/QCBOR Commit 4487f10e1bf258434fb8a39e4f59c29e31910ad0 (tag v1.4.1) Certain files will never be needed and are thus removed (reducing number of lines to almost 60%): rm -f CMakeLists.txt Makefile SECURITY.md rm -f .gitignore rm -f cmd_line_main.c example.c example.h ub-example.c ub-example.h rm -rf QCBOR.xcodeproj doc doxygen test rm -rf .git .github Signed-off-by: Yuichi Sugiyama <[email protected]> Acked-by: Jens Wiklander <[email protected]> Acked-by: Jerome Forissier <[email protected]> Acked-by: Etienne Carriere <[email protected]>

Add SPDX license identifiers to QCBOR files as per BSD-3-Clause licensing requirements, ensuring clear license information across both header and source files. Signed-off-by: Yuichi Sugiyama <[email protected]> Acked-by: Jerome Forissier <[email protected]> Acked-by: Etienne Carriere <[email protected]> Acked-by: Jens Wiklander <[email protected]>

Add the necessary build configuration for integrating the QCBOR library. Update to core.mk ensure that the library is included when CFG_QCBOR is enabled. A sub.mk file is also added to define the source files and global include directories for QCBOR. Signed-off-by: Yuichi Sugiyama <[email protected]> Reviewed-by: Jerome Forissier <[email protected]> Acked-by: Etienne Carriere <[email protected]> Acked-by: Jens Wiklander <[email protected]>

Copy remote attestation PTA functionality from the repository: https://github.com/iisec-suzaki/optee-ra (commit: 80ca8ef), and make the following adjustments for integration: - Add build configuration for remote attestation PTA by introducing the CFG_VERAISON_ATTESTATION_PTA option to align with the new naming convention. - Replace the custom base64 implementation with the base64 library added in PR OP-TEE#7007. - Update QCBOR integration by removing custom QCBOR files and using the standard library, adjusting paths as necessary. - Apply region validation improvements introduced in PR OP-TEE#6195. - Update API calls in sign.c to align with libmbedtls changes from PR OP-TEE#6151. - Calculate the required buffer size at runtime to minimize memory allocation. - Refactor code to improve readability and maintainability. - Add SPDX license identifier (BSD-2-Clause) and copyright notice. Signed-off-by: Yuichi Sugiyama <[email protected]> Reviewed-by: Thomas Fossati <[email protected]> Acked-by: Jerome Forissier <[email protected]> Acked-by: Etienne Carriere <[email protected]> Acked-by: Jens Wiklander <[email protected]>

Add a build configuration CFG_VERAISON_ATTESTATION_PTA=y. Signed-off-by: Yuichi Sugiyama <[email protected]> Reviewed-by: Jerome Forissier <[email protected]> Acked-by: Etienne Carriere <[email protected]> Acked-by: Jens Wiklander <[email protected]>

mmxsrup · 2024-11-21T14:30:03Z

@jenswi-linaro I added Acked-by to all commits.

jforissier · 2024-11-22T08:20:12Z

@mmxsrup thanks for your contribution.

jenswi-linaro reviewed Aug 26, 2024

View reviewed changes

core/pta/remote_attestation/base64.c Outdated Show resolved Hide resolved

etienne-lms reviewed Sep 2, 2024

View reviewed changes

kunisuzaki force-pushed the master branch from 5bb6b7f to 9da290b Compare October 2, 2024 00:35

kunisuzaki force-pushed the master branch from 9da290b to 3fbe614 Compare October 10, 2024 02:34

jforissier changed the title ~~PTA Remote Attestaion~~ PTA Remote Attestation Oct 10, 2024

jforissier reviewed Oct 10, 2024

View reviewed changes

mk/config.mk Show resolved Hide resolved

kunisuzaki force-pushed the master branch from 3fbe614 to 8a1b390 Compare October 15, 2024 01:49

jenswi-linaro reviewed Oct 15, 2024

View reviewed changes

kunisuzaki force-pushed the master branch from 8a1b390 to 7abdecc Compare October 16, 2024 03:25

kunisuzaki force-pushed the master branch from 7abdecc to 9de5733 Compare October 16, 2024 12:48

etienne-lms reviewed Oct 16, 2024

View reviewed changes

kunisuzaki force-pushed the master branch from 9de5733 to 6e84e28 Compare October 18, 2024 23:35

etienne-lms reviewed Oct 21, 2024

View reviewed changes

kunisuzaki force-pushed the master branch from 6e84e28 to 3d373b6 Compare October 29, 2024 11:53

jforissier reviewed Nov 15, 2024

View reviewed changes

mmxsrup force-pushed the master branch from 0d29e93 to faf03e7 Compare November 16, 2024 04:13

jenswi-linaro reviewed Nov 18, 2024

View reviewed changes

jforissier reviewed Nov 18, 2024

View reviewed changes

mmxsrup force-pushed the master branch from faf03e7 to a11af72 Compare November 18, 2024 10:03

etienne-lms reviewed Nov 18, 2024

View reviewed changes

mmxsrup force-pushed the master branch from a11af72 to 2ccc904 Compare November 18, 2024 14:18

etienne-lms reviewed Nov 18, 2024

View reviewed changes

mmxsrup force-pushed the master branch from 2ccc904 to c670d31 Compare November 18, 2024 14:51

etienne-lms reviewed Nov 19, 2024

View reviewed changes

mmxsrup force-pushed the master branch 2 times, most recently from 06738f4 to 4ced682 Compare November 20, 2024 11:52

thomas-fossati mentioned this pull request Nov 21, 2024

feature: evolving the OP-TEE Veraison TA veraison/services#281

Open

mmxsrup added 5 commits November 21, 2024 14:25

mmxsrup force-pushed the master branch from 4ced682 to 06cd14f Compare November 21, 2024 14:27

jforissier merged commit b505a58 into OP-TEE:master Nov 22, 2024
8 of 9 checks passed

		@@ -0,0 +1,426 @@
		/*==============================================================================

		@@ -0,0 +1,2506 @@
		/* =========================================================================

		@@ -0,0 +1,4 @@
		srcs-$(CFG_VERAISON_ATTESTATION_PTA) += remote_attestation.c

PTA Remote Attestation #7006

PTA Remote Attestation #7006

Conversation

kunisuzaki commented Aug 26, 2024

jenswi-linaro left a comment

Choose a reason for hiding this comment

jenswi-linaro commented Aug 26, 2024

kunisuzaki commented Aug 27, 2024

jenswi-linaro commented Aug 28, 2024

kunisuzaki commented Aug 29, 2024

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunisuzaki commented Sep 30, 2024

jenswi-linaro commented Sep 30, 2024

kunisuzaki commented Oct 2, 2024

jenswi-linaro commented Oct 7, 2024

kunisuzaki commented Oct 10, 2024

jforissier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunisuzaki commented Oct 15, 2024

jforissier commented Oct 15, 2024

jenswi-linaro commented Oct 15, 2024

jenswi-linaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Oct 15, 2024

kunisuzaki commented Oct 16, 2024

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmxsrup Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmxsrup Oct 30, 2024 •

edited

Loading