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

What to do with accumlated compatibility code #4017

Open
jenswi-linaro opened this issue Aug 6, 2020 · 8 comments
Open

What to do with accumlated compatibility code #4017

jenswi-linaro opened this issue Aug 6, 2020 · 8 comments

Comments

@jenswi-linaro
Copy link
Contributor

jenswi-linaro commented Aug 6, 2020

In #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
Copy link
Contributor

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

☮️? 😆 😉

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
Copy link
Contributor Author

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

☮️? 😆 😉

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
Copy link
Contributor

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
Copy link
Contributor Author

jenswi-linaro commented Aug 6, 2020

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

@etienne-lms
Copy link
Contributor

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
Copy link
Contributor

@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@82d8f8695622, seems to work well enough!

@jenswi-linaro
Copy link
Contributor Author

@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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants