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

Add mprotect Syscall #111

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add mprotect Syscall #111

wants to merge 6 commits into from

Conversation

ChinmayShringi
Copy link
Contributor

Description

Adds test cases for the mprotect syscall implementation to verify memory protection functionality.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • cargo build
  • cargo test ut_lind_fs_mprotect_unmapped_addr

Added the following test cases in src/RawPOSIX/src/tests/fs_tests.rs:

  • ut_lind_fs_mprotect_unmapped_addr: Tests error handling for unmapped addresses
    • Attempts to protect unmapped memory region
    • Verifies ENOMEM error is returned

Checklist:

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Test cases follow existing patterns in the codebase
  • Tests verify both successful operations and error conditions
  • All tests clean up resources properly

@rennergade
Copy link
Contributor

A few things off the back:

Title should be adding mprotect since youre adding the syscall, not just test cases

We need to at least update the vmmap region after a successful mprotect call, though I'm realizing there are two things we probably want to discuss as a group

A) Do we want to disallow mprotects to non-allocated regions? POSIX technically allows this but is labeled undefined behavior, allowing this could potentially be screwy for us and I don't believe allowing it would remove any compatibility

B) Do we want to disallow using PROT_EXEC?

@ChinmayShringi ChinmayShringi changed the title Add test cases for mprotect syscall Add test cases for mprotect Feb 17, 2025
@ChinmayShringi ChinmayShringi changed the title Add test cases for mprotect Add mprotect Syscall Feb 17, 2025
@rennergade
Copy link
Contributor

A few things off the back:

Title should be adding mprotect since youre adding the syscall, not just test cases

We need to at least update the vmmap region after a successful mprotect call, though I'm realizing there are two things we probably want to discuss as a group

A) Do we want to disallow mprotects to non-allocated regions? POSIX technically allows this but is labeled undefined behavior, allowing this could potentially be screwy for us and I don't believe allowing it would remove any compatibility

B) Do we want to disallow using PROT_EXEC?

@ChinmayShringi after discussing this with Qianxi I was mistaken, mprotects to unallocated regions should return ENOMEM. As for PROT_EXEC this isn't even possible with WASM so we'll disallow the host from doing it.

So you need to add a function (in mem.rs) before the syscall that checks if the region is in the vmmap, and if not return ENOMEM, also if PROT_EXEC is being attempted return EINVAL like in the mmap_handler function.

After the syscall, if the host mprotect was successful you have to update the protections on the vmmap regions that were changed. I believe if only part of a mapping is changed you'll have to split that mapping into two.

@Yaxuan-w
Copy link
Member

So you need to add a function (in mem.rs) before the syscall that checks if the region is in the vmmap, and if not return ENOMEM, also if PROT_EXEC is being attempted return EINVAL like in the mmap_handler function.

After the syscall, if the host mprotect was successful you have to update the protections on the vmmap regions that were changed. I believe if only part of a mapping is changed you'll have to split that mapping into two.

Will those come with this PR or on another PR?

@rennergade
Copy link
Contributor

So you need to add a function (in mem.rs) before the syscall that checks if the region is in the vmmap, and if not return ENOMEM, also if PROT_EXEC is being attempted return EINVAL like in the mmap_handler function.
After the syscall, if the host mprotect was successful you have to update the protections on the vmmap regions that were changed. I believe if only part of a mapping is changed you'll have to split that mapping into two.

Will those come with this PR or on another PR?

needs to be with this one

@@ -724,6 +724,20 @@ pub mod fs_tests {
lindrustfinalize();
}

#[test]
pub fn ut_lind_fs_mprotect_unmapped_addr() {
Copy link
Contributor

Choose a reason for hiding this comment

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

good first test, lets add a few more, especially one where it splits an existing region

Copy link
Contributor

@rennergade rennergade left a comment

Choose a reason for hiding this comment

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

Changes look good so far, need to implement protection updates and add some more tests

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.

3 participants