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

Secure Storage object getting lost when accessed by multiple applications simultaneously #2310

Closed
sahilnxp opened this issue May 11, 2018 · 18 comments

Comments

@sahilnxp
Copy link
Contributor

sahilnxp commented May 11, 2018

Hi All,

We are working on a solution in which we are using the OPTEE as secure storage for our keys.
These keys will be accessed when a connection is made to the system and there can be a number of connections to this system at same time so these keys can be accessed simultaneously.

Connections are being handled in non-secure world and make a call to OPTEE only when this connection needs the key.

When we are trying to make more than 15 simultaneous connections TEEC_OpenSession() started getting failed with error 0xFFFF000C(TEEC_ERROR_OUT_OF_MEMORY).

Debugged this issue and found that the Private shared memory which we have allocated for OPTEE driver data structures is getting exhausted, Increased this size from 1 PAGE to 2 PAGE.
The number of connections increased a little bit.

But we got into another weird problem on running the simultaneous connections again and again, the Secure Object gets lost from the system.

So to narrow down the problem we made a simple host/ta application.
You can look at code here: https://github.com/sahilnxp/secure_storage_ta

This application is doing 2 things

  • Generating an Persistent RSA Key pair (command: ./secure_storage 1)
  • RSA Decrypt operation using this Key. (command: ./secure_storage 2)

Please Note: I have hardcoded the Object ID of persistent object to be 0, So please run (./secure_storage 1) command only once.

I made following script for running the application over night.

for (( i = 1; i <= 1000; i++ ))      ### Outer for loop ###
do
    for (( j = 1 ; j <= 10; j++ )) ### Inner for loop ###
    do
          ./secure_storage 2 &
    done
    echo "******* Sleeping for $i *******"
    sleep 10
done

This script ran successfully for around 37 time and after that the object gets lost.
Please look at the attached script for the test output.
test_log.txt

I am not able to understand what goes wrong.
Can anybody please help on this, why the object is getting deleted ?

Please Note: This test is done on NXP LS1046 Board.

Regards,
Sahil Malhotra

@jenswi-linaro
Copy link
Contributor

Thanks to the easy way of reproducing the problem I could easily spot the error. Fix in #2316

@jenswi-linaro
Copy link
Contributor

The way you're opening sessions consumes a lot of memory. By making the TA single instance (TA_FLAG_SINGLE_INSTANCE) you would save some memory.

You could also try to find a way of limiting the number of open sessions. A session is a precious resource in secure world, much more so than a file descriptor in normal world.

@sahilnxp
Copy link
Contributor Author

Thanks for quick fix.
@jenswi-linaro yes, we will review the way we are opening the sessions.

1 Out of context query: Where can I see the roadmap for the OPTEE releases ?

@jbech-linaro
Copy link
Contributor

1 Out of context query: Where can I see the roadmap for the OPTEE releases ?

Unfortunately that still hasn't been published which might sound a bit weird for an open source project. But the intention is to share it, either here on GitHub directly or at optee.org. But to give you a sneak-peek, here is the plan for the coming months. Note that some of them are sub-set of full implementations (for example PKCS#11 work) and some things are taken piece by piece (keymaster).

OP-TEE 3.2.0 target date 29/Jun/18
-----
PKCS#11 user space library leveraging OP-TEE
Android Verified Boot 2.0 in U-Boot
Improve the build system for OP-TEE
Secret libraries for Trusted Applications
X.509 certificate support
Keymaster - AOSP normal world stubs
Keymaster - secure world stubbed OP-TEE Keymaster TA
Keymaster - Communication with OP-TEE
Keymaster - Key Attestation

OP-TEE 3.3.0 target date 05/Oct/18
-----
AOSP Gatekeeper (TEE)
AOSP Keymaster (TEE)

@sahilnxp
Copy link
Contributor Author

X.509 Certifiacate support will be added as part of PKCS#11 implementation ??

Thanks @jbech-linaro

@jbech-linaro
Copy link
Contributor

X.509 Certifiacate support will be added as part of PKCS#11 implementation ??

That one is a "pre-condition"/dependency for both PKCS#11 as well as when working with attestation in Keymaster. So with that done we get lots of "bang for the buck" :)

@sahilnxp
Copy link
Contributor Author

When we are trying to make more than 15 simultaneous connections TEEC_OpenSession() started getting failed with error 0xFFFF000C(TEEC_ERROR_OUT_OF_MEMORY).

After some study I understood If I enable TA_FLAG_SINGLE_INSTANCE and TA_FLAG_MULTIPLE_SESSION in TA, still will get the above error, since this memory is being managed in kernel.
This will get exhausted when a large number of request for private data structure is made.

So if this is a good idea to make this OPTEE_SHM_NUM_PRIV_PAGES as kernel config option which can be changed while compiling the kernel to support these kind of use cases ??

Please correct me if my understanding is not correct.
Thanks.

@jenswi-linaro
Copy link
Contributor

So if this is a good idea to make this OPTEE_SHM_NUM_PRIV_PAGES as kernel config option which can be changed while compiling the kernel to support these kind of use cases ??

That will not help, it's the heap in secure world that is exhausted (CFG_CORE_HEAP_SIZE).

I'd recommend review the number of sessions you're opening.

@sahilnxp
Copy link
Contributor Author

sahilnxp commented May 24, 2018

I got into some details on this.
The error is coming from following code snippet in optee_open_session() in linux driver.

shm = get_msg_arg(ctx, arg->num_params + 2, &msg_arg, &msg_parg);
if (IS_ERR(shm))
		return PTR_ERR(shm);

This get_msg_arg() function is failing at following:

shm = tee_shm_alloc(ctx, OPTEE_MSG_GET_ARG_SIZE(num_params),
			    TEE_SHM_MAPPED);
if (IS_ERR(shm))
		return shm;

From this it looks like the PRIVATE memory area for driver structures are getting exhausted.

Sorry for again and again pointing that error is coming from here, But I not able to understand how the heap in secure world is getting exhausted.

Thanks

@jenswi-linaro
Copy link
Contributor

Aha, it looks like it's indeed the private memory area that is too small.

@sahilnxp
Copy link
Contributor Author

Yeah, so can we have a Kernel CONFIG option for setting the number of pages to be configured in PRIVATE Memory so that user need not to change the code ?

@jenswi-linaro
Copy link
Contributor

Yes, sounds good.

@sahilnxp
Copy link
Contributor Author

Thanks @jenswi-linaro :)

So how patching for driver is done, Will it become available in next kernel release ?
What is the process for this ?

@jenswi-linaro
Copy link
Contributor

This seems like something straight forward so I you can post it on LKML with me in CC and we can review it there.

If you'd like me or someone to take a look at it before you post it at LKML you can create a pull request at https://github.com/linaro-swg/linux/pulls

Once the patch has been posted and reviewed on LKML I'll add it to my tree and send a pull request to arm-soc. Since we're at the end of the current kernel cycle we're likely miss the nearest upcoming merge window.

@sahilnxp
Copy link
Contributor Author

Will raise the pull request on https://github.com/linaro-swg/linux/pulls first..
Thanks.

@sahilnxp
Copy link
Contributor Author

Raised a pull request for this on: linaro-swg/linux#62
Please check.

@sahilnxp
Copy link
Contributor Author

Hi @jenswi-linaro
In which Linux kernel version we can expect this patch to be available ?

@jenswi-linaro
Copy link
Contributor

4.19

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

No branches or pull requests

3 participants