OP-TEE session login checks for client UUID generation by vesajaaskelainen · Pull Request #406 · OP-TEE/optee_test

vesajaaskelainen · 2020-03-26T20:32:47Z

These changes adds testing for handling of open session client UUID generation within Linux kernel.

This is related to: OP-TEE/optee_os#3642

host/xtest/Makefile

jforissier

Minor comments below. Other than that this looks pretty good to me :)

For commit "xtest: add helper to convert UUID string to TEEC_UUID":
Acked-by: Jerome Forissier <jerome@forissier.org>
All other commits:
Reviewed-by: Jerome Forissier <jerome@forissier.org>

host/xtest/regression_1000.c

jforissier · 2020-04-09T11:37:12Z

One more thing: I think it would be good to accept UID 1001 and GID 1001, too. Those are created by our buildroot environment:
https://github.com/OP-TEE/build/blob/3.8.0/br-ext/package/optee_client/optee_client.mk#L26-L31

$ id test
uid=1001(test) gid=1003(test) groups=1003(test),1001(teeclnt),1002(ion)

I rarely run xtest as root, I normally use this test login.

Note, in this case perhaps we should also update optee_client.mk and assign hard-coded UID and GID values (1001) instead of letting buildroot choose (-1).

vesajaaskelainen · 2020-04-20T06:50:00Z

One more thing: I think it would be good to accept UID 1001 and GID 1001, too. Those are created by our buildroot environment:
https://github.com/OP-TEE/build/blob/3.8.0/br-ext/package/optee_client/optee_client.mk#L26-L31
$ id test
uid=1001(test) gid=1003(test) groups=1003(test),1001(teeclnt),1002(ion)
I rarely run xtest as root, I normally use this test login.

Note, in this case perhaps we should also update optee_client.mk and assign hard-coded UID and GID values (1001) instead of letting buildroot choose (-1).

I can change it to use uid and gid dynamically aka. calculate UUIDv5 on the fly.

jforissier · 2020-04-20T06:52:39Z

I can change it to use uid and gid dynamically aka. calculate UUIDv5 on the fly.

Sounds good!

vesajaaskelainen · 2020-04-21T19:10:53Z

@jforissier I have now updated the PR with UUIDv5 calculation. There was this endianess thing that took a while. It would have been nice if big endian format (byte array) would have been used for UUID structure like what it was in Linux kernel.

I added your ack/reviews for older commits and for those that were changed afterwards don't have those.

I have also now updated uid/gid scheme in Linux kernel driver PR.

jforissier · 2020-04-22T17:42:29Z

@vesajaaskelainen thanks for the update, this looks pretty good to me -- I will review more closely tomorrow.

I realize the code introduced in "xtest: add UUIDv5 calculation helper" is non-trivial and has to be done on the client side in any application that wants to use the user- or group-based login methods. Could we make things easier by adding a function to libteec? The annoying things is, SHA-1 is needed and I'd rather avoid introducing dependencies on e.g. OpenSSL etc.
Perhaps we could use a simple SHA-1 implementation?

vesajaaskelainen · 2020-04-23T07:43:07Z

@vesajaaskelainen thanks for the update, this looks pretty good to me -- I will review more closely tomorrow.

I realize the code introduced in "xtest: add UUIDv5 calculation helper" is non-trivial and has to be done on the client side in any application that wants to use the user- or group-based login methods. Could we make things easier by adding a function to libteec? The annoying things is, SHA-1 is needed and I'd rather avoid introducing dependencies on e.g. OpenSSL etc.
Perhaps we could use a simple SHA-1 implementation?

Now there is a confusion here.

Kernel is expected to form the Client UUID so that it has any trustworthiness.

The code for that is in Linaro's Linux PR:
linaro-swg/linux#74

So there is no need to do this operation in user space at all. Only in group based login user space is expected to provide group ID otherwise the connection data is NULL.

The test here is so that we query the Client ID from TA, eg we want to see that the kernel generated the right thing. And in order to verify it in case where test executor can change the logic is copied from kernel code so that verification can be done.

So there is no need to have SHA-1 in client library at all. At least for this operation. It is only needed for running xtest.

Earlier version was expecting that the xtest was executed as root and this now makes it as dynamic.

host/xtest/regression_1000.c

jforissier · 2020-04-23T09:23:09Z

@vesajaaskelainen

Now there is a confusion here.

Kernel is expected to form the Client UUID so that it has any trustworthiness.

The code for that is in Linaro's Linux PR:
linaro-swg/linux#74

So there is no need to do this operation in user space at all. Only in group based login user space is expected to provide group ID otherwise the connection data is NULL.

Ah, I see. Makes sense now. I should have waited until properly reading the code before commenting ;-). Sorry for the confusion.

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

etienne-lms

Looks good. Some requested changes and some suggestions.

host/xtest/regression_1000.c

etienne-lms · 2020-04-24T14:09:26Z

host/xtest/xtest_uuid_helpers.c

+	if (p[8] != '-' || p[13] != '-' || p[18] != '-' || p[23] != '-')
+		return TEEC_ERROR_BAD_FORMAT;
+
+	u.timeLow = parse_hex(p, 8, &res);


suggest:

static TEEC_Result advance_dash(const char **s) { if (**s != '-') return TEEC_ERROR_BAD_FORMAT; (**s)++; return TEEC_SUCCESS; } static TEEC_Result parse_hex32_and_adance(const char **s, size_t nchars, uint32_t *dst) { uint32_t v = 0; size_t n = 0; int c = 0; for (n = 0; n < nchars; n++) { c = hex((*s)[n]); if (c == -1) return TEE_ERROR_BAD_FORMAT; v = (v << 4) + c; } *dest = v; *s += nchars; return TEE_SUCCESS; } TEEC_Result xtest_uuid_from_str(TEEC_UUID *uuid, const char *s) { TEEC_UUID u = { }; const char *p = s; size_t i = 0; if (get_hex32_and_advance(&p, 8, &u.timeLow) advance_dash(&p) || get_hex32_and_advance(&p, 8, &u.timeMid) advance_dash(&p) || get_hex32_and_advance(&p, 4, &u.timeHiAndVersion) advance_dash(&p)) return TEEC_ERROR_BAD_FORMAT; for (i = 0; i < 8; i++) { if (get_hex32_and_advance(&p, 2, u.clockSeqAndNode + i)) return TEEC_ERROR_BAD_FORMAT; if (i < 7 && advance_dash(&p)) return TEEC_ERROR_BAD_FORMAT; } *uuid = u; return TEE_SUCCESS; }

Is it easier to read? I don't think so. And, the commit simply imports existing code, there is no reason to change it IMO.

Left as is.

etienne-lms · 2020-04-24T14:40:48Z

host/xtest/xtest_uuid_helpers.h

@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: BSD-2-Clause */
+/*
+ * Copyright (c) 2014, STMicroelectronics International N.V.


As this is a copy from same project other git tree I would not like to change it. But in here I can put what ever is recommended action from project team. Probably should be own commit after the copy or so.

When copying an existing file I don't change the copyrights, I simply add mine if I modify the code.

Left as is.

jforissier · 2020-04-30T13:38:26Z

I'm restarting IBART. It should pass now that OP-TEE/optee_client#195 is merged.

vesajaaskelainen · 2020-04-30T14:01:06Z

@jforissier updated the PR based on feedback.

jforissier · 2020-04-30T14:02:23Z

For the two commits: "xtest: regression_1000: add test for session login as user" and "xtest: regression_1000: add test for session login as group":

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

vesajaaskelainen · 2020-04-30T14:05:26Z

For the two commits: "xtest: regression_1000: add test for session login as user" and "xtest: regression_1000: add test for session login as group":

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

Thanks for the review!

Updated the PR with those included.

vesajaaskelainen · 2020-04-30T14:07:22Z

I have now tested this with PATCH v2 for kernel changes that is on mailing list.

Note: What is in Linaro tree there is no practical change in the code.

I do not have access to full test suite so cannot say if there are any effects for that. Hopefully not. Only issue might be that if application based logins are tested and they do not pass as kernel says not supported.

jforissier · 2020-04-30T14:57:30Z

I do not have access to full test suite so cannot say if there are any effects for that. Hopefully not.

I have run it on QEMU, all is good. In fact there are no tests with login methods other than TEEC_LOGIN_PUBLIC.

root@Vexpress:/ xtest
Run test suite with level=0

TEE test application started with device [(null)]
######################################################
#
# regression+gp
#
######################################################

[snip]

747 test cases of which 0 failed
0 test cases were skipped
TEE test application done!
root@Vexpress:/

vesajaaskelainen · 2020-05-03T06:55:15Z

I do not have access to full test suite so cannot say if there are any effects for that. Hopefully not.

I have run it on QEMU, all is good. In fact there are no tests with login methods other than TEEC_LOGIN_PUBLIC.

Thanks for testing it out!

jforissier · 2020-05-05T07:30:28Z

@etienne-lms @jenswi-linaro gentle reminder in case you'd like to ack before I merge this, thanks.

jenswi-linaro

Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

etienne-lms

Acked-by: Etienne Carriere <etienne.carriere@linaro.org> with a minor header file inclusion ordering issue.

host/xtest/xtest_uuid_helpers.c

vesajaaskelainen · 2020-05-05T09:56:09Z

@etienne-lms @jenswi-linaro gentle reminder in case you'd like to ack before I merge this, thanks.

I can fix @etienne-lms's comment later today and will include those acks and update the PR.

Add helper method to query current client identity for session login testing. Acked-by: Etienne Carriere <etienne.carriere@linaro.org> Acked-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Jerome Forissier <jerome@forissier.org> Signed-off-by: Vesa Jääskeläinen <vesa.jaaskelainen@vaisala.com>

Verifies that Linux REE kernel authentication and authorization works for public session login. Acked-by: Etienne Carriere <etienne.carriere@linaro.org> Acked-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Jerome Forissier <jerome@forissier.org> Signed-off-by: Vesa Jääskeläinen <vesa.jaaskelainen@vaisala.com>

This is copied and adapted from optee-os repository: lib/libutee/tee_uuid_from_str.c Renamed as xtest_uuid_helpers.c|h Acked-by: Etienne Carriere <etienne.carriere@linaro.org> Acked-by: Jens Wiklander <jens.wiklander@linaro.org> Acked-by: Jerome Forissier <jerome@forissier.org> Signed-off-by: Vesa Jääskeläinen <vesa.jaaskelainen@vaisala.com>

UUIDv5 is specific in RFC 4122. This implements section (for SHA-1): 4.3. Algorithm for Creating a Name-Based UUID Acked-by: Etienne Carriere <etienne.carriere@linaro.org> Acked-by: Jens Wiklander <jens.wiklander@linaro.org> Acked-by: Jerome Forissier <jerome@forissier.org> Signed-off-by: Vesa Jääskeläinen <vesa.jaaskelainen@vaisala.com>

Verifies that Linux REE kernel authentication and authorization works for session login for user. Acked-by: Etienne Carriere <etienne.carriere@linaro.org> Acked-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Jerome Forissier <jerome@forissier.org> Signed-off-by: Vesa Jääskeläinen <vesa.jaaskelainen@vaisala.com>

Verifies that Linux REE kernel authentication and authorization works for session login with specified group. Acked-by: Etienne Carriere <etienne.carriere@linaro.org> Acked-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Jerome Forissier <jerome@forissier.org> Signed-off-by: Vesa Jääskeläinen <vesa.jaaskelainen@vaisala.com>

vesajaaskelainen · 2020-05-07T16:03:09Z

@jforissier updated the PR with acks and #include reordering.

jforissier · 2020-05-07T16:05:07Z

@vesajaaskelainen cool, I'm merging this, thanks.

Edit: I just wanted to add it was a nice contribution, several people asked about this login feature in the past, but no one really had a clue how to implement it 😉

etienne-lms · 2020-05-07T16:15:28Z

Now that this is merged... I am wondering if we won't face the same issue as for #415: one testing latest OP-TEE (os/client/test) with latest mainline Linux kernel will get its xtest regression tests to fail on these new tests since mainline Linux kernel does not integrate linaro-swg/linux#74.
:(

jforissier · 2020-05-07T16:35:06Z

one testing latest OP-TEE (os/client/test) with latest mainline Linux kernel will get its xtest regression tests to fail on these new tests

Yes, but it is just a matter of time until the code lands upstream, and it is one of the reasons we have linaro-swg/linux branch optee.
However... now that you're mentioning it... we do have platforms in our manifest that are not based on linaro-swg optee...

$ git remote -v | grep origin
origin	https://github.com/OP-TEE/manifest.git (fetch)
origin	https://github.com/OP-TEE/manifest.git (push)
$ $ git grep linux | grep -v 'revision="optee'
am43xx.xml:        <project path="linux"          name="ti-linux-kernel/ti-linux-kernel.git" revision="00a3beacce33dc14fa301eb5f3fb5a341212e9b4" remote="ti" />
am57xx.xml:        <project path="linux"          name="ti-linux-kernel/ti-linux-kernel.git" revision="00a3beacce33dc14fa301eb5f3fb5a341212e9b4" remote="ti" />
dra7xx.xml:        <project path="linux"          name="ti-linux-kernel/ti-linux-kernel.git" revision="00a3beacce33dc14fa301eb5f3fb5a341212e9b4" remote="ti" />
poplar.xml:        <project path="linux"                name="96boards-poplar/linux.git"             revision="poplar-4.9" clone-depth="1" />
rpi3.xml:        <project path="linux"                name="linaro-swg/linux.git"                  revision="rpi3-optee-4.14" clone-depth="1" />
verdin.xml:        <project path="linux"                name="igoropaniuk/linux"               revision="mainline_verdin" clone-depth="1" />

~~I will create~~ PRs for Poplar [done: link] and RPi3 [done: link]. The TI boards are stuck to a fixed revision so not sure there is a branch to update for them. Ping @OP-TEE/plat-ti.
For Verdin: ping @igoropaniuk.

=> The commits to cherry-pick from https://github.com/linaro-swg/linux/commits/optee are:

Commit baa151f2924f ("tee: add support for session's client UUID generation")
Commit ad19acdcdbc5 ("tee: optee: Add support for session login client UUID generation")

vesajaaskelainen · 2020-05-07T18:44:35Z

PATCH v2 has been sent for those Linux kernel patches in the mailing list about a week ago. Now kernel is in rc4 and in weekend that would be rc5. So probably the earliest we have code in the kernel is 3-4 weeks (for 5.8-rc1) assuming that there are no blocking issues found.

Best would be if we could cherry-pick changes from upstream kernel for those vendor trees.

But I suppose you were planning to make op-tee release in about 3 week timeline which is kinda problematic.

vesajaaskelainen changed the title ~~Optee login checks~~ Optee session login checks for client UUID generation Mar 26, 2020

vesajaaskelainen changed the title ~~Optee session login checks for client UUID generation~~ OP-TEE session login checks for client UUID generation Mar 26, 2020

jenswi-linaro reviewed Mar 30, 2020

View reviewed changes

host/xtest/Makefile Outdated Show resolved Hide resolved

vesajaaskelainen force-pushed the optee-login-checks branch from a66181b to 7ee0d32 Compare April 5, 2020 17:13

jforissier approved these changes Apr 9, 2020

View reviewed changes

host/xtest/regression_1000.c Outdated Show resolved Hide resolved

host/xtest/regression_1000.c Outdated Show resolved Hide resolved

vesajaaskelainen force-pushed the optee-login-checks branch from 7ee0d32 to bcec8ab Compare April 21, 2020 18:54

jenswi-linaro reviewed Apr 23, 2020

View reviewed changes

host/xtest/regression_1000.c Outdated Show resolved Hide resolved

host/xtest/regression_1000.c Outdated Show resolved Hide resolved

etienne-lms suggested changes Apr 24, 2020

View reviewed changes

vesajaaskelainen force-pushed the optee-login-checks branch 2 times, most recently from ecb76e2 to 0ecc4ee Compare April 30, 2020 14:00

vesajaaskelainen force-pushed the optee-login-checks branch from 0ecc4ee to e6db8ab Compare April 30, 2020 14:05

jenswi-linaro reviewed May 5, 2020

View reviewed changes

etienne-lms reviewed May 5, 2020

View reviewed changes

host/xtest/xtest_uuid_helpers.c Show resolved Hide resolved

vesajaaskelainen added 5 commits May 7, 2020 18:23

vesajaaskelainen force-pushed the optee-login-checks branch from e6db8ab to 14e3f38 Compare May 7, 2020 15:23

etienne-lms approved these changes May 7, 2020

View reviewed changes

jforissier merged commit 30dd087 into OP-TEE:master May 7, 2020

jforissier mentioned this pull request Sep 2, 2020

How are TAs loaded under the hood? OP-TEE/optee_os#4062

Closed

Conversation

vesajaaskelainen commented Mar 26, 2020

Uh oh!

Uh oh!

jforissier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jforissier commented Apr 9, 2020

Uh oh!

vesajaaskelainen commented Apr 20, 2020

Uh oh!

jforissier commented Apr 20, 2020

Uh oh!

vesajaaskelainen commented Apr 21, 2020

Uh oh!

jforissier commented Apr 22, 2020

Uh oh!

vesajaaskelainen commented Apr 23, 2020

Uh oh!

Uh oh!

Uh oh!

jforissier commented Apr 23, 2020

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

etienne-lms Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

jforissier Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

vesajaaskelainen Apr 30, 2020

Choose a reason for hiding this comment

Uh oh!

etienne-lms Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

vesajaaskelainen Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

jforissier Apr 30, 2020

Choose a reason for hiding this comment

Uh oh!

vesajaaskelainen Apr 30, 2020

Choose a reason for hiding this comment

Uh oh!

jforissier commented Apr 30, 2020

Uh oh!

vesajaaskelainen commented Apr 30, 2020

Uh oh!

jforissier commented Apr 30, 2020

Uh oh!

vesajaaskelainen commented Apr 30, 2020

Uh oh!

vesajaaskelainen commented Apr 30, 2020

Uh oh!

jforissier commented Apr 30, 2020

Uh oh!

vesajaaskelainen commented May 3, 2020

Uh oh!

jforissier commented May 5, 2020

Uh oh!

jenswi-linaro left a comment

Choose a reason for hiding this comment

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vesajaaskelainen commented May 5, 2020

Uh oh!

vesajaaskelainen commented May 7, 2020

Uh oh!

jforissier commented May 7, 2020 •

edited

Loading

jforissier commented May 7, 2020 •

edited

Loading