Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Write of coverage data file in REE FS #231

Closed
wants to merge 2 commits into from

Conversation

lenormandfranck
Copy link

Allow to write a coverage date file generated at runtime from optee core in plaintext in REE FS.

It stores the data files in a subfolder which can be configured by the user. The write of a data file has two arguments:

  • The place to write: Absolute path of the object file which is covered by the data file, it is used later when processing the coverage to have the note files from compilation along with the data files produced at runtime.
  • The buffer of data to write

Make the function mk_path available to other components

Signed-off-by: Franck LENORMAND <franck.lenormand@nxp.com>
The coverage data are data which are not encrypted and
should be available in clear for the user.

This patch adds an RPC allowing the TEE to write the
coverage data in REE FS in clear.

The configuration are:
 - CFG_GCOV_SUPPORT: if the RPC is enabled, default to y
 - CFG_TEE_CLIENT_COV_DIR: location where the data should be saved,
                           default to coverage subdir

Signed-off-by: Franck LENORMAND <franck.lenormand@nxp.com>
Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks clean to me.
Some minor comments on implementation.

#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>
#include <fcntl.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sort alphabetically

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -0,0 +1,139 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an SPDX licence tag.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

#include <linux/tee.h>

#include "tee_supp_fs.h"
#include "coverage.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

swap lines (alpha ordering)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok


static const char *s_storage_dir = CFG_TEE_CLIENT_COV_DIR"/";

static void fldump(const char* desc, const uint8_t *buf, uint32_t size)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess this will e removed :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes :)


TEEC_Result coverage_process(size_t num_params, struct tee_ioctl_param *params)
{
char *filepath;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a default value at each local variable definition.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

EMSG("Cannot retrieve filepath\n");
return TEEC_ERROR_BAD_PARAMETERS;
}
filepath_size = MEMREF_SIZE(params + 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/params + 0/params/

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

cov_data_size = MEMREF_SIZE(params + 1);

/* Check the filepath is a string (null terminated) */
if (filepath[filepath_size - 1] != '\0') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't search backward? (or forward)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The size of the string is passed to the function and the last character is expected to be the final character so no need to search for it, just check it is present, else something went wrong.

filepath);

/* Create the path of the file */
n = snprintf(path, sizeof(path), "%s%s", s_storage_dir, filepath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to see / here rather than at s_storage_dir[] definition.
But not a strong opinion.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, it is cleaner

return TEEC_ERROR_SHORT_BUFFER;
}

/* Create the path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer

	/*
	 * Create the path
	 * ...
	 */

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

#include <teec_trace.h>

#if defined(CFG_GCOV_SUPPORT)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can remove these empty lines

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@github-actions
Copy link

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

@github-actions github-actions bot added the Stale label Dec 23, 2021
@github-actions github-actions bot closed this Dec 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants