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.

Sign up for GitHub

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

Jump to bottom

xtest: pkcs11: Add check that CK_TOKEN_INFO outputs correct CK_ULONG's #718

Merged
merged 1 commit into from
Feb 5, 2024
Merged

xtest: pkcs11: Add check that CK_TOKEN_INFO outputs correct CK_ULONG's #718

merged 1 commit into from
Feb 5, 2024

Conversation

vesajaaskelainen
Copy link
Contributor

@vesajaaskelainen vesajaaskelainen commented Dec 6, 2023 •
edited
Loading

OP-TEE's PKCS#11 TA TEE<->REE interface treats CK_ULONG's as 32 bit.

In 64 bit machines this creates a problem for CK_ULONG values that are architecture dependent.

One of those defines is CK_UNAVAILABLE_INFORMATION.

This adds test case to make sure that when OP-TEE's PKCS#11 TA should give CK_UNAVAILABLE_INFORMATION it actually does that.

Depends on:

@vesajaaskelainen vesajaaskelainen mentioned this pull request Dec 6, 2023
Merged
@vesajaaskelainen vesajaaskelainen force-pushed the pkcs11-fix-get-token-info branch from 8d225a5 to 1cc4565 Compare December 22, 2023 07:52
@vesajaaskelainen
Copy link
Contributor Author

Just rebased to master.

@vesajaaskelainen vesajaaskelainen force-pushed the pkcs11-fix-get-token-info branch from 1cc4565 to 9093469 Compare January 12, 2024 12:47
@vesajaaskelainen
Copy link
Contributor Author

Just rebased to master.

@vesajaaskelainen
Copy link
Contributor Author

@jforissier now that the client part is merged -- could you trigger test run again for this PR?

@jforissier
Copy link
Contributor

@vesajaaskelainen sure. Done!

etienne-lms
etienne-lms reviewed Jan 17, 2024
View reviewed changes
Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to mention pkcs11_1001 in the commit header line, as:
xtest: pkcs11_1001: Test CK_UNAVAILABLE_INFORMATION output value

host/xtest/pkcs11_1000.c
if (!ADBG_EXPECT_COMPARE_UNSIGNED(c,
token_info.ulMaxSessionCount, ==,
CK_UNAVAILABLE_INFORMATION))
goto out;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation make weird alignments.
prefer:

		if (!ADBG_EXPECT_COMPARE_UNSIGNED(c,
				token_info.ulMaxSessionCount, ==,
				CK_UNAVAILABLE_INFORMATION))
			goto out;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

host/xtest/pkcs11_1000.c Outdated
@@ -309,6 +309,36 @@ static void xtest_pkcs11_test_1001(ADBG_Case_t *c)
if (!ADBG_EXPECT_CK_OK(c, rv))
goto out;

/**
* Verify that fields with CK_UNAVAILABLE_INFORMATION gets
* correct value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to highlight here that the pkcs11 always return CK_UNAVAILABLE_INFORMATION for ulMaxSessionCount, ulMaxRwSessionCount, ulTotalPublicMemory and ulFreePublicMemory so we can test them safely.

As per ulTotalPrivateMemory and ulFreePrivateMemory I think we can implement them one day, so I would not enforce there values in this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@vesajaaskelainen vesajaaskelainen force-pushed the pkcs11-fix-get-token-info branch from 9093469 to 2d80c07 Compare February 4, 2024 09:24
@vesajaaskelainen
Copy link
Contributor Author

Sorry for a bit of delay on updates for the PR.

But now all review comments are addressed and PR has been re-based and re-tested.

etienne-lms
etienne-lms reviewed Feb 5, 2024
View reviewed changes
Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-by: Etienne Carriere <[email protected]>

@vesajaaskelainen
xtest: pkcs11_1001: Test CK_UNAVAILABLE_INFORMATION output value
4b6baca 
OP-TEE's PKCS#11 TA TEE<->REE interface treats CK_ULONG's as 32 bit.

In 64 bit machines this creates a problem for CK_ULONG values that are
architecture dependent.

One of those defines is CK_UNAVAILABLE_INFORMATION.

This adds test case to make sure that when OP-TEE's PKCS#11 TA should give
CK_UNAVAILABLE_INFORMATION it actually does that.

Signed-off-by: Vesa Jääskeläinen <[email protected]>
Reviewed-by: Etienne Carriere <[email protected]>
@vesajaaskelainen vesajaaskelainen force-pushed the pkcs11-fix-get-token-info branch from 2d80c07 to 4b6baca Compare February 5, 2024 12:47
@vesajaaskelainen
Copy link
Contributor Author

Reviewed-by: Etienne Carriere <[email protected]>

Thanks for the review!

Applied tag.

@jforissier jforissier merged commit 99d5c29 into OP-TEE:master Feb 5, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etienne-lms etienne-lms etienne-lms left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vesajaaskelainen @jforissier @etienne-lms