pkcs11: Add TEE Identity based authentication support

[vesajaaskelainen]

Please see OP-TEE/optee_os#4222 for testing and usage instructions.

Implements:
OP-TEE/optee_os#3946

[etienne-lms]

Only minor comments.
For both commits, prefer start log header line with libckteec: and add () to function labels. Also add a description line between header line and S-o-b tag in commit "C_Login: Allow NULL_PTR PIN entry.". i.e.

libckteec: Allow NULL_PTR PIN entry in C_Login()

Allow C_Login() to transport a null PIN reference as when protected
authentication path is used.

Signed-off-by: Vesa Jääskeläinen <[email protected]>

[vesajaaskelainen]

Only minor comments.
For both commits, prefer start log header line with libckteec: and add () to function labels. Also add a description line between header line and S-o-b tag in commit "C_Login: Allow NULL_PTR PIN entry.". i.e.

libckteec: Allow NULL_PTR PIN entry in C_Login()

Allow C_Login() to transport a null PIN reference as when protected
authentication path is used.

Signed-off-by: Vesa Jääskeläinen <[email protected]>

OK

[vesajaaskelainen]

Only minor comments.
For both commits, prefer start log header line with libckteec: and add () to function labels. Also add a description line between header line and S-o-b tag in commit "C_Login: Allow NULL_PTR PIN entry.". i.e.

libckteec: Allow NULL_PTR PIN entry in C_Login()

Allow C_Login() to transport a null PIN reference as when protected
authentication path is used.

Signed-off-by: Vesa Jääskeläinen <[email protected]>

OK

Updated commit messages.

[vesajaaskelainen]

@etienne-lms I believe I have addressed all your comments.

Also tested (and fixed) PIN == NULL_PTR cases for methods that use PIN as input.

[vesajaaskelainen]

I was thinking about could xtest be used here somehow but seems rather challenging as there is no API to reset the token back to initial state without performing clearing secure storage and reboot 😞. Also switching to different identities and somehow having common identities can be a bit of problem.

[etienne-lms]

Maybe a debug command to reset a token even without the SO credentials?
The TA supports multi tokens, a token could be reserved for debug purpose, identified from its token label.

[vesajaaskelainen]

Maybe a debug command to reset a token even without the SO credentials?
The TA supports multi tokens, a token could be reserved for debug purpose, identified from its token label.

If it would have some magic name then perhaps that could enable reset token feature. This magic name would just be something that normally would not be used.

What I would like to have is that we can run xtest (and other tests) on to be released TA binaries to verify that everything is still looking good and if automated tests pass then mark release gate for those TA's as tests has passed. Eg. so that the binary would be intact and no need to compile it differently.

[etienne-lms]

Currently xtest lists the slots and picks the last last to run its tests.
We could have xtest to check the token label is "Test token for OP-TEE xtest " so we can safely scratch all even without rights.

[etienne-lms]

@ruchi393, does this change look ok to you?
@vesajaaskelainen, LGTM (with checkpatch report addressed) but I wait for OP-TEE/optee_os#4222 before posting my R-b tag.
I think this is a nice feature, thanks for sharing.

[etienne-lms]

CI/checkpatch issue report:

4e56bdb libckteec: Use environment variables to determine OP-TEE TA login
CHECK: Unnecessary parentheses around 'errno == ERANGE'
#68: FILE: libckteec/src/invoke_ta.c:267:
+			if ((errno == ERANGE) || (tmpconv > (gid_t)-1) ||
+			    (login_gid_env + strlen(login_gid_env) != endp)) {
total: 0 errors, 0 warnings, 1 checks, 63 lines checked

I.e.:

			if (errno == ERANGE || tmpconv > (gid_t)-1 ||
			    (login_gid_env + strlen(login_gid_env) != endp)) {
				...
			}

[vesajaaskelainen]

$ CHECKPATCH=../linux/scripts/checkpatch.pl ../optee_os/scripts/checkpatch.sh 0eaddfc570b557c39148ff2e7faccaba8e07d55a
Checking commit(s):
0eaddfc libckteec: Use environment variables to determine OP-TEE TA login
total: 0 errors, 0 warnings, 110 lines checked

"[PATCH] libckteec: Use environment variables to determine OP-TEE TA" has no obvious style problems and is ready for submission.

Where as linux == v5.10-rc1.

Then wondering the difference:

$ cp ../optee_os/scripts/checkpatch.sh scripts/
$ CHECKPATCH=../linux/scripts/checkpatch.pl scripts/checkpatch.sh 0eaddfc570b557c39148ff2e7faccaba8e07d55a
Checking commit(s):
0eaddfc libckteec: Use environment variables to determine OP-TEE TA login
CHECK: Unnecessary parentheses around 'errno == ERANGE'
#121: FILE: libckteec/src/invoke_ta.c:267:
+			if ((errno == ERANGE) || (tmpconv > (gid_t)-1) ||
+			    (login_gid_env + strlen(login_gid_env) != endp)) {

total: 0 errors, 0 warnings, 1 checks, 110 lines checked

Shall we copy the checkpatch.sh also to this repo?

[vesajaaskelainen]

Just rebased on top of master.

[etienne-lms]

Reviewed-by: Etienne Carriere <[email protected]> with comment on 1st commit.

It would be nice we have something in optee_test/host/xtest/pkcts11_1000.c to test a bit this but it may tricky to test invalid identities.

[vesajaaskelainen]

Reviewed-by: Etienne Carriere <[email protected]> with comment on 1st commit.

Thanks for the review!

Fixed and applied tags.

[vesajaaskelainen]

It would be nice we have something in optee_test/host/xtest/pkcts11_1000.c to test a bit this but it may tricky to test invalid identities.

I believe we need to improve the testing support for tokens in overall first and then we can try to add tests also for these. This is kinda integration testing not unit testing level problem as we would need to navigate to different identities in a way that people can still run xtest successfully.

I would prefer that we do these as separate things.