OP-TEE / optee_os

Trusted side of the TEE
Other
1.58k stars 1.07k forks source link

What to do with accumlated compatibility code #4017

Open jenswi-linaro opened 4 years ago

jenswi-linaro commented 4 years ago

In https://github.com/OP-TEE/optee_os/pull/3693 we're adding ta_elf_set_init_fini_info_compat() to handle TAs compiled for OP-TEE 3.9.0.

Prior to this we have alloc_temp_sec_mem() and everything past that line in tee_svc_copy_param(). This also pulls in mobj_seccpy_shm_alloc() in case paging of TAs is enabled. This is needed to handle TAs compiled for OP-TEE older than 3.6.0.

With https://projects.linaro.org/browse/LOC-342 we're updating to GlobalPlatform TEE Internal Core API Specification v1.2.1. This involves replacing the algorithm identifiers TEE_ALG_ECDSA and TEE_ALG_ECDH since incorrect values was assumed in OP-TEE. Doing this in a backwards compatible (avoiding recompiling TAs) way will be quite painful and leave complicated code to handle new and old algorithm identifiers.

We have a header ta_head which has some special treatment in ldelf depending on depr_entry and is also fixed at the load address of the TA while it could have been looked up via its name instead. ta_head is also the reason why the load address passed to a debugger is offset with 0x20.

These are the major parts I'm currently aware of, I'll update as more are found.

This compatibility code is not covered by our tests. While the code is new it probably works as expected, but as OP-TEE progresses it's questionable. We should deal with this in some way. Here's the options I've come up with so far:

  1. Delete the compatibility code and make a 4.0.0 release
  2. Enclose each piece of compatibility code with CFG_COMPAT_xxx
  3. Run regression tests based on older TAs
  4. Do nothing and hope that accumulated compatibility code doesn't break in a bad way.

1 Would be the easiest for the maintainers, but perhaps not for the downstream users.

2 Would give the option for downstream to disable unneeded compatibility. We could also cycle out old compatibility gradually after a number of releases, it may wreck the semantic versioning scheme though.

3 This is harder that one might expect. Currently we only make sure that the latest on the three optee_os, optee_client and optee_test passes all tests. We have test cases that are updated to test for fixed bugs in OP-TEE that make it hard to run successfully with older versions of OP-TEE. So we will probably need to craft special TAs that tests a particular compatibility issue, one by one.

4 This is our current track.

For some downstream users I imagine that a combination of 2 and 3 would be appealing. This would however require a bit of work to get in place.

For upstream it would be very nice to have a path where we can get rid of old unused cruft that wastes memory might get in the way of better designs.

What is the way forward? I hope we can find a middle way that works with downstream and upstream.

jforissier commented 4 years ago

I think 5, 6, 7, 8 should be 1, 2, 3, 4 above ;-)

You're raising valid concerns I think. We probably have enough compatibility code that we need to address the issue it raises, and do it in a sensible way (hopefully).

  1. Enclose each peace of compatibility code with CFG_COMPAT_xxx

:peace_symbol:? :laughing: :wink:

That would certainly be useful for people working with constrained environments, and not hard to do in general. We could have them enabled by default, and test all = n in CI.

  1. Run regression tests based on older TAs This is harder that one might expect.

Perhaps not so much. We could write some script or Makefile which would checkout two environments (OLD and NEW), build both, then assemble an hybrid version (basically copy xtest from OLD into NEW) and run the tests again. It would obviously take more time than current regressions, and even more as OLD versions are added. But we could run it only as a sanity check at release time.

jenswi-linaro commented 4 years ago

I think 5, 6, 7, 8 should be 1, 2, 3, 4 above ;-)

The markdown was playing tricks with me. :-)

You're raising valid concerns I think. We probably have enough compatibility code that we need to address the issue it raises, and do it in a sensible way (hopefully).

  1. Enclose each peace of compatibility code with CFG_COMPAT_xxx

:peace_symbol:? :laughing: :wink:

Hrm, that one is on me. :-)

That would certainly be useful for people working with constrained environments, and not hard to do in general. We could have them enabled by default, and test all = n in CI.

Agree, 2 will likely be part of the deal.

  1. Run regression tests based on older TAs This is harder that one might expect.

Perhaps not so much. We could write some script or Makefile which would checkout two environments (OLD and NEW), build both, then assemble an hybrid version (basically copy xtest from OLD into NEW) and run the tests again. It would obviously take more time than current regressions, and even more as OLD versions are added. But we could run it only as a sanity check at release time.

An old xtest environment doesn't necessarily work on a new OP-TEE. You're welcome to try though. ;-)

jforissier commented 4 years ago

An old xtest environment doesn't necessarily work on a new OP-TEE. You're welcome to try though. ;-)

Hmmm, OK. I would think somehow we need to copy the CA and TAs to the root filesystem and it should be it. I am missing something?

jenswi-linaro commented 4 years ago

OK, it might work I thought there was some issues in the past but perhaps I'm confusing it with something.

etienne-lms commented 4 years ago

Thanks Jens to put that on the table. I think 2) and 3) together with 1) as a long term target is my preferred path. Hmm... this though does not help much.

Hacking into build/br-ext/ content, i think we should be able to fetch & build a rootfs filesystem with CAs/TAs based on an old optee tag. It would allow to test them over the latest mainline optee Core (optee_os.git) itan optee Linux kernel linaro-swg/linux.git optee branch). Is it such setup you would like to test?

jforissier commented 4 years ago

@jenswi-linaro

An old xtest environment doesn't necessarily work on a new OP-TEE. You're welcome to try though. ;-)

Very crude attempt at https://github.com/jforissier/optee_compat_test/commit/82d8f8695622, seems to work well enough!

jenswi-linaro commented 4 years ago

@jenswi-linaro

An old xtest environment doesn't necessarily work on a new OP-TEE. You're welcome to try though. ;-)

Very crude attempt at jforissier/optee_compat_test@82d8f86, seems to work well enough!

Nice. We'll need to compile this for a few different OP-TEE versions. The coverage should be quite good, the only drawback is that it probably too time consuming to run in travis. But we don't need to run this all the time. It should be enough to do this when we suspect something and a few weeks before next release.

github-actions[bot] commented 4 years ago

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