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

Secstor anti-rollback #1630

Merged
merged 4 commits into from
Jun 27, 2017
Merged

Secstor anti-rollback #1630

merged 4 commits into from
Jun 27, 2017

Conversation

jenswi-linaro
Copy link
Contributor

No description provided.

Copy link
Contributor

@lorc lorc left a comment

Choose a reason for hiding this comment

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

Hello.
I have spoted some styling issues and one suspicious refcount handling.

@@ -56,7 +56,7 @@ struct tee_fs_dirfile_fileh {
* @commit_writes: commits changes since the file was opened
*/
struct tee_fs_dirfile_operations {
TEE_Result (*open)(bool create, const TEE_UUID *uuid,
TEE_Result (*open)(bool create, uint8_t *hash, const TEE_UUID *uuid,
Copy link
Contributor

Choose a reason for hiding this comment

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

You forgot to update comment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry, I don't get it. Which comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I'm sorry. That was structure description, not function description. Please ignore this.

if (res)
return res;


Copy link
Contributor

Choose a reason for hiding this comment

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

Remove extra line.

@@ -476,7 +476,7 @@ static TEE_Result get_dirh(struct tee_fs_dirfile_dirh **dirh)
}
}
assert(ree_fs_dirh);
ree_fs_dirh_refcount++;
assert(ree_fs_dirh_refcount);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any sense in this assert if you increase refcount in the beginning of the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The assert is in case it has wrapped.

@@ -462,10 +462,10 @@ static size_t ree_fs_dirh_refcount;

static TEE_Result get_dirh(struct tee_fs_dirfile_dirh **dirh)
{
if (!ree_fs_dirh_refcount) {
ree_fs_dirh_refcount++;
Copy link
Contributor

Choose a reason for hiding this comment

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

In case of error (at L471), ree_fs_dirh_refcount will not be decreased.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right. I'll fix.

@jenswi-linaro
Copy link
Contributor Author

update

@lorc
Copy link
Contributor

lorc commented Jun 22, 2017

You have fixed issue with reference counting, but I'm not sure, if it is right approach.

Now you must call put_dirh() even if get_dirh() has failed. This is not intuitive because you expect that if get failed then there are nothing to put back.

Compare this with malloc/free: you can call free() on NULL if malloc() failed, but you are not obligated to do this.

There are nothing bad with current code, but it can confuse anyone else, who will try to extend it. Actually, it confused me for a moment :)

@jenswi-linaro
Copy link
Contributor Author

Yes, I agree it's a bit confusing. I'll rewrite it.

Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

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

I think the secure storage protection level should be set to 100 when this anti-rollback feature is active.

Also, the documentation needs some minor updates in order to properly reflect these changes:

@@ -69,10 +69,14 @@ struct tee_fs_dirfile_operations {

/**
* tee_fs_dirfile_open() - opens a dirfile handle
* @create: true if a new dirfile is to be created, else the dirfile
* is read opened and verified
* @hash: hash of underlaying file
Copy link
Contributor

Choose a reason for hiding this comment

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

s/underlaying/underlying/

@@ -86,8 +90,10 @@ void tee_fs_dirfile_close(struct tee_fs_dirfile_dirh *dirh);
/**
* tee_fs_dirfile_commit_writes() - commit updates of dirfile
* @dirh: dirfile handle
* @hash: hash of underlaying file is copied here if not NULL
Copy link
Contributor

Choose a reason for hiding this comment

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

s/underlaying/underlying/

@jenswi-linaro
Copy link
Contributor Author

Update

@jenswi-linaro
Copy link
Contributor Author

Revised {get,put}_dirh() pattern:

  • Failure from get_dirh() results in *dirh = NULL
  • put_dirh() with dirh == NULL is ignored

put_dirh() after failing get_dirh() isn't needed, but harmless if done since dirh will be NULL.

@jforissier
Copy link
Contributor

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

@lorc
Copy link
Contributor

lorc commented Jun 26, 2017

I'm not so familiar with tee_fs, to give R-B. But you have my:
Acked-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

Adds a hash parameter to the dirfile interface. The hash is used in the
same way as in the htree interface, that is, used to verify integrity on
open and used to get updated hash on writes.

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Acked-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
REE FS closes the dirfile if returning error from a function that may
have changed the content of a secure storage object. This effectively
undoes previous operation.

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Acked-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Provides tee_rpmb_fs_raw_open() use by OP-TEE OS.

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Acked-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
@jenswi-linaro
Copy link
Contributor Author

Squashed, rebased and tags applied.

#ifdef CFG_RPMB_FS
static const uint32_t ts_antiroll_prot_lvl = 1000;
#else
static const uint32_t ts_antiroll_prot_lvl
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing ;

REE FS uses RPMB (if available) for storage of dirfile hash.

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Acked-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
@jenswi-linaro
Copy link
Contributor Author

Sqashed in the missing ";"

@jforissier
Copy link
Contributor

Thanks @jenswi-linaro! Now we can tick the "anti-rollback" box on the OP-TEE feature list, that's good.

@jforissier jforissier merged commit b4b1a20 into OP-TEE:master Jun 27, 2017
@jenswi-linaro jenswi-linaro deleted the secstor branch June 27, 2017 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants