Implements LOGIN_APPLICATION and LOGIN_USER_APPLICATION

[Kh-Oleg]

This PR implements the GlobalPlatform's LOGIN_APPLICATION and LOGIN_USER_APPLICATION methods, by adding the executable path and name to the input string for the UUIDv5 hashing algorithm.

[jforissier]

Hi,

Thanks for the patch. Please see comments below.

[jforissier]

PATH_MAX

[Kh-Oleg]

done

[jforissier]

Please update the comment block above.

[Kh-Oleg]

done

[jforissier]

Please update the comment block above.

[Kh-Oleg]

done

[jforissier]

PATH_MAX

[Kh-Oleg]

done

[jenswi-linaro]

This has also been discussed at https://lists.trustedfirmware.org/archives/list/[email protected]/thread/6HLJ2GROVHZGY2LX7HGHOQ53YIFIC6PW/#6HLJ2GROVHZGY2LX7HGHOQ53YIFIC6PW

[jforissier]

Please squash all the changes in a single patch and add:

Acked-by: Jerome Forissier <[email protected]>

Please also consider the discussion mentioned by @jenswi-linaro.

[jenswi-linaro]

How accurate is this when taking chroot and different techniques into account?

[Kh-Oleg]

Hello, I squashed the commits and added "Acked-by".

With regards to the above mentioned discussion, there are several points being discussed:

1. Getting rid of TEE_UUID_NS_NAME_SIZE, by calling snprintf twice
   In my view, from performance reasons, it's probably, better to increase TEE_UUID_NS_NAME_SIZE to some reasonable size (let's say 1024) to support majority of the cases rather than always execute snprintf second time.

2. Adding Konfig parameter to choose between path and extended attributes.
   This goes beyond the scope of my PR. In my case, path based app id is enough. If the extended attributes method is necessary/desirable it can be done later as a separate PR.

3. chroot
   If I'm not wrong, chroot requires root privileges. If the attacker could get root privileges, he can simply replace the original CA binary.

4. d_path vs d_absolute_path
   With the current implementation running CA as my_app, ./my_app or /opt/bin/my_app will end up in generating different UUIDs, which has pros and cons. I feel that restricting to the absolute path only is too strong. The current implementation leaves it up to the user: if TA developer needs a fixed absolute path, he can calculate the UUID based on it and require CA to be always run using the absolute path.

Which one would you like to address in this PR to have it merged?

[jenswi-linaro]

Why are you proposing a PR to this repository instead of posting a patch on the LKML? A patch landed here is only temporary until we tire of it and remove it when we rebase the patches for a newer kernel. To get into the mainline kernel, it must follow the normal kernel process.

What you're proposing is an extension to the ABI. Once established, it cannot break userspace in further changes. So, it's important to get it right and try to be flexible enough for reasonable use cases. Kconfig is not a good way to select between two incompatible implementations.

What problem are you trying to solve with this patch?

[Kh-Oleg]

I created the PR to this repo, because this repo is used in the OPTEE manifests: https://github.com/OP-TEE/manifest/blob/master/common.xml#L13. I thought that contribution to this repo will end up in the next release of OPTEE.

Let me describe the problem and then we can decide whether we should proceed and how.

I am developing a TA, based on GlobalPlatform API, which shall be compiled and run for different TEEs. OPTEE running in QEMU is one of them. My TA uses LOGIN_APPLICATION and LOGIN_USER_APPLICATION methods to implement access control. It is desirable that OPTEE would support these login methods in some way, assuming that exec path is taken into account when calculating UUID. Otherwise, my TA and tests must have different implementations, depending on the TEE used, what I'd like to avoid.

I solved my problem by having a locally patched driver, but I thought that maybe it will be good to upstream the changes. Having a less flexible implementation which could be improved later is better than not having the feature at all. Breaking changes, if necessary, can be done via versioning and deprecation of the client library and driver.

If you're in principle not objecting from accepting the changes, please, let me know what is missing and what is the right way to contribute. Otherwise, the PR can be closed.

[jenswi-linaro]

I solved my problem by having a locally patched driver, but I thought that maybe it will be good to upstream the changes.

That makes a lot of sense and is the right thing to do. However, this isn't upstream for the kernel. This is where we keep local patches needed for our OP-TEE specific test setups. Your patch might have qualified for that if it wasn't for the fact that it touches on standardized interfaces. Once we support LOGIN_APPLICATION and LOGIN_USER_APPLICATION here, people will expect that this is the OP-TEE way. So I'd rather see that they can make it into the mainline kernel before we start to promise things.

Having a less flexible implementation which could be improved later is better than not having the feature at all. Breaking changes, if necessary, can be done via versioning and deprecation of the client library and driver.

We don't do breaking changes in the kernel and have no desire to accumulate baggage due to premature ABI changes.
If we've messed up and there's no way for a nice fix we may need to resort to fixes like that.

If you're in principle not objecting from accepting the changes, please, let me know what is missing and what is the right way to contribute. Otherwise, the PR can be closed.

We can continue the review here for a while longer. I'm interested in @vesajaaskelainen's thoughts on this. If the patch is received well on the LKML we can take it here too so it's available before we've stepped up the kernel.