DRAFT: Enable MBEDTLS_FS_IO when this feature can be used

[andresag01]

Description

This patch is the first step towards integrating the mbed TLS NV Seed feature with mbed OS. The change enables the macro MBEDTLS_FS_IO when filesystem support is present in mbed OS. To use NV Seed, users will need to define the macros MBEDTLS_ENTROPY_NV_SEED and MBEDTLS_PLATFORM_STD_NV_SEED_FILE either through the user config file or the mbed_app.json.

NOTE: The entropy seed file will be stored unencrypted in the non-volatile storage.

@RonEld @sbutcher-arm @yanesca: Please review.

Status

IN DEVELOPMENT

Migrations

NO

Related PRs

List related PRs against other branches:

• Fix C declaration of dir functions and types #3949: The PR enables the dir access functions to be callable from C so that mbed TLS code can use them. This change must be merged after PR Fix C declaration of dir functions and types #3949.

Todos

• Tests

[0xc0170]

retest uvisor

[theotherjimmy]

@yanesca @sbutcher-arm @RonEld Bump. Could you review?

[0xc0170]

@andresag01 Shall we close this? is this still relevant and we keep it open?

[ciarmcom]

ARM Internal Ref: IOTSSL-1723

[RonEld]

I approve this, but is this still a draft?

[andresag01]

@RonEld: Thanks for the review! I have marked it as DRAFT because I would like the changes to be reviewed by at least two mbed TLS developers before removing the label.

[0xc0170]

bump, is this still active, and targeting master?

[andresag01]

@0xc0170: This is still active, we are currently waiting for other mbed TLS team members to take a look (CC @yanesca, @sbutcher-arm)

[adbridge]

@mazimkhan @hanno-arm I think this falls to you two now

[hanno-becker]

@andresag01 Where is DEVICE_ENTROPY_SOURCE defined? It doesn't occur in the mbed-OS repository (I only found DEVICE_TRNG in relation to entropy). Depending on its definition, it might need to be updated to incorporate the new potential source of entropy from the NV seed.

[andresag01]

This is simply an out-of-date comment from PR #2716. I think my original idea was to create a simple macro that would make it easy to tell if an entropy source is available. However, I think this idea was scraped at some point (due to some complications with config/implementation) and unfortunately the comment was not updated.

Note that I did not change this line in the PR other than to align the \ character. I will update the comment before removing the DRAFT label. However, could you please look at the actual code changes for the time being? Thanks!

[hanno-becker]

Where is it documented what this option entails? Does it ensure the presence of FS IO functions that Mbed TLS is using, with the same (standard) signatures?

[hanno-becker]

Will the use of the NV seed be configured by manually setting MBEDTLS_NV_SEED, or will there be an option DEVICE_NV_SEED like NV_SEED and a line in platform_mbed.h defining MBEDTLS_NV_SEED if DEVICE_NV_SEED is set?

[hanno-becker]

The documentation is outdated and should be adapted. Overall the change looks good, but I'd like to have more confidence that MBED_CONF_FILESYSTEM_PRESENT and MBEDTLS_FS_IO are compatible.

[andresag01]

@hanno-arm: I do not think that you have been reviewing this PR by taking into account all the background information. Please refer to the internal ticket IOTSSL-1283 that contains information on how this can be used for NV Seed and some security considerations.

[hanno-becker]

@andresag01 I'll look at that. Please add the reference to the PR description.

[andresag01]

@hanno-arm: Regarding the correspondence between MBED_CONF_FILESYSTEM_PRESENT and wha. I am not aware of any documentation from mbed OS that describes what MBED_CONF_FILESYSTEM_PRESENT does. I looked into the code in mbed_retarget.h which I believe is the place where some of the libc functions end up being implemented and observed:

#if MBED_CONF_FILESYSTEM_PRESENT
#include "filesystem/FileSystem.h"
#include "filesystem/File.h"
#include "filesystem/Dir.h"
#endif

If you continue looking into that file you will see that for example the file opening code has:

#if MBED_CONF_FILESYSTEM_PRESENT
        } else {
            FileSystem *fs = path.fileSystem();
            if (fs == NULL) {
                /* The filesystem instance managing the namespace under the mount point
                 * has not been found. Free file handle */
                errno = ENOENT;
                filehandles[fh_i] = NULL;
                return -1;
            }
            int posix_mode = openmode_to_posix(openmode);
            File *file = new ManagedFile;
            int err = file->open(fs, path.fileName(), posix_mode);
            if (err < 0) {
                errno = -err;
                delete file;
            } else {
                res = file;
            }
#endif

So I thought that MBED_CONF_FILESYSTEM_PRESENT is probably a superset of whats needed for MBEDTLS_FS_IO. But we could certainly ask for more information...

[adbridge]

@hanno-arm Could you please re-review ?

[cmonr]

@hanno-arm @andresag01, any progress?

[hanno-becker]

@adbridge @cmonr Apologies for not getting back to you sooner. I am busy with urgent tasks at the moment but plan to come back to this next week.

[cmonr]

@hanno-arm @RonEld @yanesca @sbutcher-arm Thoughts? This PR has been sitting for a while. I've also added @mazimkhan since this is a TLS-related PR.

[cmonr]

@hanno-arm @RonEld @yanesca @sbutcher-arm @mazimkhan
Second ping.

[simonbutcher]

@k-stachowiak is looking into this PR, and confirming it's current status.

Based on new requirements we know are coming, we may close this PR and start again, or alternatively build on @andresag01 's existing work.

[0xc0170]

Please reopen with an update if needed, I'll close this one now.