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

Update curve25519 and posixshm transport #73

Merged
merged 22 commits into from
Sep 18, 2024

Conversation

billphipps
Copy link
Contributor

  1. Update curve25519 to match ecc code structure.
  2. Update posix shm transport to work on MacOS and add optional DMA mappings
  3. Clean up tests
  4. Correct other typos

@billphipps billphipps requested review from jpbland1 and bigbrett and removed request for jpbland1 September 11, 2024 19:14
Copy link
Contributor

@bigbrett bigbrett left a comment

Choose a reason for hiding this comment

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

Partial review: Haven't yet looked at the crypto or TCP portions yet. But wanted some clarifications of how things are supposed to work re: client initialization

.github/workflows/build-and-test.yml Show resolved Hide resolved
test/Makefile Outdated Show resolved Hide resolved
test/Makefile Show resolved Hide resolved
port/posix/posix_transport_shm.c Outdated Show resolved Hide resolved

/* Unmap the header and remap the full area */
(void)munmap((void*)header, sizeof(*header));
ret = posixTransportShm_Map(fd, size, map);
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the rationale behind mapping just the header first, then unmapping and remapping the entire area? Vs just mapping the whole thing in one go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The client doesn't know the sizes of the buffers, so it maps the header first to find out, then remaps once the sizes are understood.

return wh_TransportMem_SendRequest(ctx->transportMemCtx, len, data);
/* Check connected status */
switch(ctx->state) {
case PTSHM_STATE_NONE:
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit confused about this logic (as well as the client initialization. Since the client init is what initially zeros the context and calls posixTransportShm_{Use/Handle}Map() on it, and we don't ever want a user to be sending requests without first initializing the client context, should we even be handling the NONE state here? I'd have figured that this logic should have been in the INITIALIZED state, since that is what the client init handler sets the state to once it successfully initializes. Is this some sort of deferred initialization that is allowed if the original client init fails? If so perhaps we just call client init again here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The server creates and zeroes the object. The client only has to map it. NONE is used when the client was not able to open the object OR the object was not completely setup by the server. This forces the client to attempt to reopen, map the header, and then remap the entire object. Once the client is able to successfully map the entire segment, it sets the intiitalzed member in the header and leaves the mapping intact. Recall, the client init only tries once to open and map the object, so the entire sequence is tried again here as well. Sorry for the misleading comments.

port/posix/posix_transport_shm.c Outdated Show resolved Hide resolved
port/posix/posix_transport_shm.c Show resolved Hide resolved
port/posix/posix_transport_shm.h Outdated Show resolved Hide resolved
port/posix/posix_transport_shm.h Show resolved Hide resolved
Copy link
Contributor

@bigbrett bigbrett left a comment

Choose a reason for hiding this comment

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

overall looks good

port/posix/posix_transport_tcp.h Outdated Show resolved Hide resolved
port/posix/posix_transport_tcp.c Show resolved Hide resolved
test/wh_test_comm.c Show resolved Hide resolved
test/wh_test_crypto.c Show resolved Hide resolved
test/wh_test_crypto.c Outdated Show resolved Hide resolved
test/wh_test_crypto.c Outdated Show resolved Hide resolved
wolfhsm/wh_client_crypto.h Show resolved Hide resolved
src/wh_client_crypto.c Outdated Show resolved Hide resolved
src/wh_crypto.c Outdated Show resolved Hide resolved
src/wh_crypto.c Show resolved Hide resolved
@bigbrett
Copy link
Contributor

Verified crypto tests pass on tc3xx

@billphipps billphipps requested a review from bigbrett September 18, 2024 14:20
Copy link
Contributor

@bigbrett bigbrett left a comment

Choose a reason for hiding this comment

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

just need to guard unistd.h include with WOLFHSM_CFG_TEST_POSIX and I can merge

@@ -26,6 +26,7 @@
#include <stdint.h>
#include <stdio.h> /* For printf */
#include <string.h> /* For memset, memcpy */
#include <unistd.h> /* For sleep */
Copy link
Contributor

Choose a reason for hiding this comment

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

needs to be protected by WOLFHSM_CFG_TEST_POSIX (block already present later in this file)

@bigbrett bigbrett merged commit c4fa47d into wolfSSL:main Sep 18, 2024
1 check passed
@billphipps billphipps deleted the update_curve25519 branch September 18, 2024 14:55
jefferyq2 pushed a commit to jefferyq2/wolfHSM that referenced this pull request Nov 10, 2024
Update curve25519 and posixshm transport
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants