Skip to content

Set ptracer permissions for IPC handle creation #1018

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
Closed
wants to merge 1 commit into from
Closed

Set ptracer permissions for IPC handle creation #1018

wants to merge 1 commit into from

Conversation

PatKamin
Copy link
Contributor

@PatKamin PatKamin commented Jan 7, 2025

Fixes: #979

Checklist

  • Code compiles without errors locally
  • All tests pass locally
  • CI workflows execute properly
  • CI workflows, not executed per PR (e.g. Nightly), execute properly
  • New tests added, especially if they will fail without my changes
  • Added/extended example(s) to cover this functionality
  • Extended the README/documentation
  • All newly added source files have a license
  • All newly added source files are referenced in CMake files
  • Logger (with debug/info/... messages) is used

@PatKamin PatKamin requested review from vinser52 and ldorau January 7, 2025 07:54
@PatKamin PatKamin requested a review from a team as a code owner January 7, 2025 07:54
lukaszstolarczuk
lukaszstolarczuk requested changes Jan 7, 2025
View reviewed changes
@lplewa
Copy link
Contributor

lplewa commented Jan 7, 2025

  1. Should the library call it? imho this should be something done by the user, not by the library. So we should only update examples, and documentation, so user knows how to use this API. Imho this is security variability, if we do it always, without user consent.

  2. Do it even work? We set permission to parent process. Our tests will work as producer and customer shares the same parent process. What if producer, and consumer are different parents? I.E are start from different bash processes.

@PatKamin
Copy link
Contributor Author

PatKamin commented Jan 8, 2025

  1. Should the library call it? imho this should be something done by the user, not by the library. So we should only update examples, and documentation, so user knows how to use this API. Imho this is security variability, if we do it always, without user consent.

It's for the ease-of-use of the UMF IPC API. Perhaps at least we should enable tracing with a new CMake option so the user enables tracing explicitly? And this would be disabled by default as UMF users not using IPC API would not be interested in setting ptrace scope wider.

  1. Do it even work? We set permission to parent process. Our tests will work as producer and customer shares the same parent process. What if producer, and consumer are different parents? I.E are start from different bash processes.

It works for MPI ranks communication. That's the case covered in our tests where producer and consumer share the same parent process. As for the different parents case, it's not tested but I think it could work if they are using the same shared umf library.

@lplewa
Copy link
Contributor

lplewa commented Jan 8, 2025

As for the different parents case, it's not tested but I think it could work if they are using the same shared umf library.

This is about kernel level process security protection/permissions - common shared library between processes do not have to influence here.

ldorau
ldorau requested changes Jan 9, 2025
View reviewed changes
Copy link
Contributor

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

26 tests out of 56 fail on WSL with the following error:

umf_example_ipc_ipcapi_producer: unified-memory-framework/src/ipc_cache.c:136: umfIpcHandleMappedCacheCreate: Assertion `IPC_MAPPED_CACHE_GLOBAL != NULL' failed.
Aborted (core dumped)

@lukaszstolarczuk
Copy link
Contributor

just a note here: we've established to rather updated docs, examples and our tests, than use the current solution...

@PatKamin PatKamin force-pushed the ptrace branch 2 times, most recently from 7cfadea to c86f19b Compare January 14, 2025 11:48
@PatKamin
Copy link
Contributor Author

Updated PR with prctl() call removed from the umf library but used in tests and mentioned in comments and README.

@PatKamin
Use prctl() call before IPC handle creation
e56d1f3 
Use the prctl() call to let the parent process and its children open
IPC handle.
This is a more secure way than setting the ptrace_scope to 0 globally
for all processes in the system.

Co-authored-by: [email protected]
lukaszstolarczuk
lukaszstolarczuk reviewed Jan 14, 2025
View reviewed changes
Copy link
Contributor

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

beside minor issues, LGTM

lplewa
lplewa requested changes Jan 15, 2025
View reviewed changes
examples/ipc_ipcapi/ipc_ipcapi_producer.c
// to copy producer's file descriptor, even when ptrace_scope is set to 1.
ret = prctl(PR_SET_PTRACER, getppid());
if (ret == -1) {
printf("prctl() call failed with errno %d (%s). This may indicate that "
Copy link
Contributor

Choose a reason for hiding this comment

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

perror?

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

test/common/ipc_common.c
@@ -336,6 +338,14 @@ int run_producer(int port, umf_memory_pool_ops_t *pool_ops, void *pool_params,
int producer_socket = -1;
char consumer_message[MSG_SIZE];

ret = prctl(PR_SET_PTRACER, getppid());
if (ret == -1) {
printf("prctl() call failed with errno %d (%s). This may indicate that "
Copy link
Contributor

Choose a reason for hiding this comment

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

perror

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

@lukaszstolarczuk lukaszstolarczuk mentioned this pull request Jan 15, 2025
Merged
@lukaszstolarczuk
Copy link
Contributor

Patryk is off at the moment, so finishing up the minor issues left here - contd in #1040

@PatKamin PatKamin deleted the ptrace branch January 28, 2025 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukaszstolarczuk lukaszstolarczuk lukaszstolarczuk requested changes

@ldorau ldorau ldorau requested changes

@lplewa lplewa lplewa requested changes

@KFilipek KFilipek KFilipek approved these changes

@vinser52 vinser52 Awaiting requested review from vinser52

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

Successfully merging this pull request may close these issues.

Use prctl when ptrace is restricted
5 participants
@PatKamin @lplewa @lukaszstolarczuk @KFilipek @ldorau