FAPI TEST: Rework the execution of FAPI integration tests #2716

JuergenReppSIT · 2023-11-28T11:50:46Z

The of usage of make check on big endian platforms is now possible.
The tests executed by fint-log-compiler.sh were integrated into main-fapi.c
A CA script for initializing the CA before make check was added.
The EK, the EK certificate and the EK fingerprint are now created in main-fapi.c

Signed-off-by: Juergen Repp juergen_repp@web.de

Due to tcti_libtpms->response_len and tcti_libtpms->response_buffer_len are size_t. We cannot convert the (size_t *) to (uint32_t *) on big-endian platforms. Thus we create temp uint32_t variables. Make the call and then assign it back to size_t variables. This commit partially fix the test failure on big-endian platforms. Signed-off-by: Ying-Chun Liu (PaulLiu) <paulliu@debian.org>

src/tss2-tcti/tcti-libtpms.c

joholl

I'm not an OpenSSL expert, but that looks quite good. Thanks for tackling this!

We might be able to delete some (most?) of test/helper/*.c now, right?

joholl · 2023-12-01T11:11:44Z

src/tss2-tcti/tcti-spi-helper.c

@@ -313,7 +318,7 @@ static uint32_t spi_tpm_helper_read_sts_reg(TSS2_TCTI_SPI_HELPER_CONTEXT* ctx)
 {
    uint32_t status = 0;
    spi_tpm_helper_read_reg(ctx, TCTI_SPI_HELPER_TPM_STS_REG, &status, sizeof(status));
-    return status;
+    return LE_TO_HOST_32(status);


Would we not need this elsewhere, too? I think did_vid/status in these two calls need to be turned from little endian to host endianness:

tpm2-tss/src/tss2-tcti/tcti-spi-helper.c

Lines 718 to 721 in 182b027

uint32_t did_vid = 0;

for (int retries = 100; retries > 0; retries--) {

// In case of failed read div_vid is set to zero

spi_tpm_helper_read_reg(ctx, TCTI_SPI_HELPER_TPM_DID_VID_REG, &did_vid, sizeof(did_vid));

tpm2-tss/src/tss2-tcti/tcti-spi-helper.c

Lines 319 to 321 in 182b027

static void spi_tpm_helper_write_sts_reg(TSS2_TCTI_SPI_HELPER_CONTEXT* ctx, uint32_t status)

{

spi_tpm_helper_write_reg(ctx, TCTI_SPI_HELPER_TPM_STS_REG, &status, sizeof(status));

If I am not mistaken, we might also want to add unit tests to catch stuff like that.

joholl · 2023-12-01T11:15:23Z

src/tss2-tcti/tcti-spi-helper.c

@@ -2,6 +2,10 @@
 /*
 * Copyright 2020 Fraunhofer SIT. All rights reserved.
 */
+#ifdef HAVE_CONFIG_H


About your commit message:

Based on #2685 In the function spi_tpm_helper_read_sts_reg the result is converted back to the correct endianess.

I'm unsure about what this commit has to do with #2685, other than that it lgtm.

joholl · 2023-12-01T12:32:28Z

.ci/docker.run

@@ -24,6 +24,11 @@ if [ -z "$WITH_CRYPTO" ]; then
    export WITH_CRYPTO="ossl"
 fi

+little_endian=$(echo -n I | od -to2 | awk 'FNR==1{ print substr($2,6,1)}')
+if [ $little_endian -eq 0 ]; then
+    export CONFIGURE_OPTIONS="$CONFIGURE_OPTIONS --with-integrationtcti=libtpms"


At some point, we probably want to test against libtpms by default (regardless of endianness).

joholl · 2023-12-01T12:46:40Z

test/integration/main-fapi.c


    LOG_INFO("Test returned %i", ret);
    if (ret) goto error;

-    size = asprintf(&remove_cmd, "rm -r -f %s", tmpdir);
+#if !defined(FAPI_NONTPM) && !defined(DLOPEN)
+    test_ctx->test_esys_ctx.esys_ctx = test_ctx->fapi_ctx->esys;


Is this necessary? We already have the following in test_fapi_setup():

(*test_ctx)->test_esys_ctx.esys_ctx = (*test_ctx)->fapi_ctx->esys;

yes that's necessary because in some FAPI integration tests a new esys context is created.

joholl · 2023-12-01T12:52:23Z

test/integration/main-fapi.c

+    }
+#endif
+
+    size = asprintf(&remove_cmd, "rm -r -f %s", test_ctx->tmpdir);


Should we not move the following into test_fapi_teardown()?

size = asprintf(&remove_cmd, "rm -r -f %s", test_ctx->tmpdir); if (size < 0) { LOG_ERROR("Out of memory"); ret = EXIT_ERROR; goto error; } if (system(remove_cmd) != 0) { LOG_ERROR("Directory %s can't be deleted.", test_ctx->tmpdir); ret = EXIT_ERROR; goto error; }

I did put only things without error check to test_fapi_teardown

joholl · 2023-12-01T13:10:00Z

test/integration/test-common.c

@@ -582,3 +580,14 @@ test_esys_teardown(TSS2_TEST_ESYS_CONTEXT *test_ctx)
        free(test_ctx);
    }
 }
+
+void
+test_fapi_esys_teardown(TSS2_TEST_ESYS_CONTEXT *test_ctx)


I'm a little confused by this. We already have test_esys_teardown() and test_fapi_teardown(). This new function test_fapi_esys_teardown() seems to be never called. Maybe you added this accidentally?

joholl · 2023-12-01T13:10:50Z

test/integration/main-fapi.c

    ret = test_fapi_setup(&test_ctx);
    if (ret != 0) {
        goto error;
    }

 #if !defined(FAPI_NONTPM) && !defined(DLOPEN)
-    ret = test_fapi_checks_pre(test_ctx);
+        ret = test_fapi_checks_pre(test_ctx);


Wrong indentation

Based on tpm2-software#2686 In the function spi_tpm_helper_read_sts_reg the result is converted back to the correct endianess. Addresses tpm2-software#2531 Co-authored-by: Lukas Märdian <slyon@ubuntu.com> Signed-off-by: Juergen Repp <juergen_repp@web.de>

Usage of swtpm caused problems for the the tests on s390x/ubuntu. Therfore the configure option with-integrationtcti to enable the usage of libtpms fot the integration tests was added. The macro AC_C_BIGENDIAN was used to define WORDS_BIGENDIAN which can be used to skip tests which use little endian test data. Signed-off-by: Juergen Repp <juergen_repp@web.de>

Several global variables in fapi-main.c are now declared locally. Signed-off-by: Juergen Repp <juergen_repp@web.de>

* The tests of the TPM state is integrated. * A global FAPI test state was introduced. * The test dlopen-fapi-get-random was executed without the check of the TPM state. Because the esys function used in the test program could not be loaded. Signed-off-by: Juergen Repp <juergen_repp@web.de>

JuergenReppSIT · 2023-12-04T10:47:09Z

@joholl I have fixed most of your remarks. We could also completely switch to libtpms as default. It also would be possible to remove the int log compile scripts with the disadvantage that make check could not be started with the tpm servers.
@AndreasFuchsTPM Should we do that?

AndreasFuchsTPM · 2023-12-04T15:26:30Z

@AndreasFuchsTPM Should we do that?

I would be in favor of removing all scripts surrounding the tests.

Maybe a quick idea: Let's keep the capability for this release and drop it on the next release ?

JuergenReppSIT · 2023-12-04T15:33:10Z

Maybe a quick idea: Let's keep the capability for this release and drop it on the next release ?

That sounds reasonable.

joholl · 2023-12-04T15:34:08Z

@JuergenReppSIT Cool!

About getting rid of mssim/swtpm: I'd prefer if we still supported testing against mssim/swtpm. This is because it might be more convenient in some contexts and it might have some upsides (testing against remote etc.).

Maybe related: There is a way to reset all three simulators (see below). Just spitballing: Maybe we want to have a future setup where the simulator is started once and then tested against?

@AndreasFuchsTPM Maybe we even want to standardize the TCTI reset functionality in the TCTI spec (would mean a v3 of the context). Of course that means only for non-productive TPMs and with TSS2_TCTI_RC_NOT_IMPLEMENTED for real ones.

tpm2-tss/test/tpmclient/tpmclient.int.c

Lines 176 to 205 in 182b027

    
           #ifdef TCTI_LIBTPMS 
        
               rval = Tss2_Tcti_Libtpms_Reset(tcti); 
        
               /* If TCTI is not libtpms, bad context is returned. */ 
        
               if (rval != TSS2_TCTI_RC_BAD_CONTEXT) { 
        
                   return rval; 
        
               } else { 
        
                   LOG_WARNING("TPM Reset failed: wrong TCTI type retrying with swtpm..."); 
        
               } 
        
           #endif /* TCTI_LIBTPMS */ 
        
           #ifdef TCTI_SWTPM 
        
               rval = Tss2_Tcti_Swtpm_Reset(tcti); 
        
               /* If TCTI is not swtpm, bad context is returned. */ 
        
               if (rval != TSS2_TCTI_RC_BAD_CONTEXT) { 
        
                   return rval; 
        
               } else { 
        
                   LOG_WARNING("TPM Reset failed: wrong TCTI type retrying with mssim..."); 
        
               } 
        
           #endif /* TCTI_SWTPM */ 
        
           #ifdef TCTI_MSSIM 
        
               rval = (TSS2_RC)tcti_platform_command( tcti, MS_SIM_POWER_OFF ); 
        
               if (rval == TSS2_RC_SUCCESS) { 
        
                   rval = (TSS2_RC)tcti_platform_command( tcti, MS_SIM_POWER_ON ); 
        
               } else { 
        
                   LOG_WARNING("TPM Reset failed: mssim returned 0x%x.", rval); 
        
               } 
        
           #endif /* TCTI_MSSIM */

JuergenReppSIT · 2023-12-04T15:56:54Z

@joholl Thank you for the review. I have not updated tcti-spi-helper. We should create a separate PR with the unit tests for the big endian problem. I just wanted to integrate the corresponding PR #2686 with the requested changes to test docker.run on the big endian platform.

AndreasFuchsTPM

So is this ready to merge ?
@JuergenReppSIT @joholl all agree ?

* A CA script for initializing the CA was added. * The EK, the EK certificate and the EK fingerprint is now created in main-fapi.c * The creation of the EK certificate was removed from fint-log-compiler.sh * Unneeded helper programs are removed. Signed-off-by: Juergen Repp <juergen_repp@web.de>

codecov · 2023-12-10T18:33:51Z

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (d0632da) 82.57% compared to head (a155675) 82.51%.
Report is 18 commits behind head on master.

Files	Patch %	Lines
src/tss2-tcti/tcti-libtpms.c	53.33%	7 Missing ⚠️
src/tss2-fapi/api/Fapi_Provision.c	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2716      +/-   ##
==========================================
- Coverage   82.57%   82.51%   -0.06%     
==========================================
  Files         368      369       +1     
  Lines       42986    43128     +142     
==========================================
+ Hits        35496    35589      +93     
- Misses       7490     7539      +49

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

joholl · 2023-12-22T10:03:37Z

Ready to merge from my perspective :)

JuergenReppSIT mentioned this pull request Nov 28, 2023

tcti-spi-helper: uint32_t is conversion to a void* buffer fails on big endian #2686

Closed

JuergenReppSIT changed the title ~~FAPI TEST: Rework of the execution of FAPI integration tests~~ FAPI TEST: Rework the execution of FAPI integration tests Nov 28, 2023

JuergenReppSIT force-pushed the test-use-libtpms-on-big-endian branch from 1b61f57 to 75a6372 Compare November 29, 2023 09:01

AndreasFuchsTPM requested a review from joholl November 30, 2023 16:06

joholl reviewed Dec 1, 2023

View reviewed changes

src/tss2-tcti/tcti-libtpms.c Show resolved Hide resolved

joholl reviewed Dec 1, 2023

View reviewed changes

JuergenReppSIT and others added 4 commits December 4, 2023 11:08

FAPI-TEST: Remove unneeded global variables.

f781ea9

Several global variables in fapi-main.c are now declared locally. Signed-off-by: Juergen Repp <juergen_repp@web.de>

JuergenReppSIT force-pushed the test-use-libtpms-on-big-endian branch from 75a6372 to 1a8252b Compare December 4, 2023 10:40

JuergenReppSIT force-pushed the test-use-libtpms-on-big-endian branch 2 times, most recently from 1a8252b to 19c5612 Compare December 6, 2023 14:48

AndreasFuchsTPM previously approved these changes Dec 7, 2023

View reviewed changes

JuergenReppSIT dismissed AndreasFuchsTPM’s stale review via a155675 December 10, 2023 18:03

JuergenReppSIT force-pushed the test-use-libtpms-on-big-endian branch from 19c5612 to a155675 Compare December 10, 2023 18:03

AndreasFuchsTPM approved these changes Jan 10, 2024

View reviewed changes

AndreasFuchsTPM merged commit f1a6167 into tpm2-software:master Jan 10, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FAPI TEST: Rework the execution of FAPI integration tests #2716

FAPI TEST: Rework the execution of FAPI integration tests #2716

JuergenReppSIT commented Nov 28, 2023 •

edited

Loading

joholl left a comment

joholl Dec 1, 2023

joholl Dec 1, 2023

joholl Dec 1, 2023

joholl Dec 1, 2023

JuergenReppSIT Dec 4, 2023

joholl Dec 1, 2023

JuergenReppSIT Dec 4, 2023

joholl Dec 1, 2023

joholl Dec 1, 2023

JuergenReppSIT commented Dec 4, 2023

AndreasFuchsTPM commented Dec 4, 2023

JuergenReppSIT commented Dec 4, 2023

joholl commented Dec 4, 2023 •

edited

Loading

JuergenReppSIT commented Dec 4, 2023

AndreasFuchsTPM left a comment

codecov bot commented Dec 10, 2023

joholl commented Dec 22, 2023

	uint32_t did_vid = 0;
	for (int retries = 100; retries > 0; retries--) {
	// In case of failed read div_vid is set to zero
	spi_tpm_helper_read_reg(ctx, TCTI_SPI_HELPER_TPM_DID_VID_REG, &did_vid, sizeof(did_vid));

	static void spi_tpm_helper_write_sts_reg(TSS2_TCTI_SPI_HELPER_CONTEXT* ctx, uint32_t status)
	{
	spi_tpm_helper_write_reg(ctx, TCTI_SPI_HELPER_TPM_STS_REG, &status, sizeof(status));

FAPI TEST: Rework the execution of FAPI integration tests #2716

FAPI TEST: Rework the execution of FAPI integration tests #2716

Conversation

JuergenReppSIT commented Nov 28, 2023 • edited Loading

joholl 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JuergenReppSIT commented Dec 4, 2023

AndreasFuchsTPM commented Dec 4, 2023

JuergenReppSIT commented Dec 4, 2023

joholl commented Dec 4, 2023 • edited Loading

JuergenReppSIT commented Dec 4, 2023

AndreasFuchsTPM left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 10, 2023

Codecov Report

joholl commented Dec 22, 2023

JuergenReppSIT commented Nov 28, 2023 •

edited

Loading

joholl commented Dec 4, 2023 •

edited

Loading