Skip to content

set PROT_NONE when mmap fails in regions_quarantine_deallocate_pages#332

Open
rdevshp wants to merge 1 commit into
GrapheneOS:mainfrom
rdevshp:large_alloc_quarantine_mprot_fix
Open

set PROT_NONE when mmap fails in regions_quarantine_deallocate_pages#332
rdevshp wants to merge 1 commit into
GrapheneOS:mainfrom
rdevshp:large_alloc_quarantine_mprot_fix

Conversation

@rdevshp
Copy link
Copy Markdown
Contributor

@rdevshp rdevshp commented May 10, 2026

regions_quarantine_deallocate_pages in latest main commit 9a44297 keeps the pages writable when mmap fails. This PR fixes this issue.

@thestinger
Copy link
Copy Markdown
Member

There's little point in doing this because mprotect would likely fail if mmap with MAP_FIXED did. Just because it's an mmap call doesn't mean it's doing more allocation than mprotect.

@rdevshp
Copy link
Copy Markdown
Contributor Author

rdevshp commented May 10, 2026

Perhaps hardened_malloc should simply call fatal_error when mmap fails in this case to make sure that the pages aren't RW after free then?

@thestinger
Copy link
Copy Markdown
Member

Perhaps hardened_malloc should simply call fatal_error when mmap fails in this case to make sure that the pages aren't RW after free then?

It definitely shouldn't do that.

@rdevshp
Copy link
Copy Markdown
Contributor Author

rdevshp commented May 10, 2026

Perhaps hardened_malloc should simply call fatal_error when mmap fails in this case to make sure that the pages aren't RW after free then?

It definitely shouldn't do that.

There are already multiple instances of hardened_malloc calling fatal_error after mprotect fails in the code. It is undesirable to leave the pages RW after free.

@thestinger
Copy link
Copy Markdown
Member

Calling it during initialization is extremely different from calling it later. It's absolutely not acceptable to abort the process after initialization on out-of-memory errors and there should be no case of that happening.

@rdevshp rdevshp force-pushed the large_alloc_quarantine_mprot_fix branch from bd92965 to e2549dd Compare May 10, 2026 14:14
@rdevshp
Copy link
Copy Markdown
Contributor Author

rdevshp commented May 10, 2026

I've removed the call to fatal_error. Even if the mprotect call is likely to fail in this case, it is simply a last resort measure that would not normally have any performance impacts.

@thestinger
Copy link
Copy Markdown
Member

The point of the MAP_FIXED call is essentially doing the combination of madvise with MADV_DONTNEED combined with mprotect in a single system call. It's getting rid of the old pages and creating a new PROT_NONE mapping there. It's possible that mprotect might work due to just dropping memory but VMA limits are what's likely to cause a failure and Linux isn't very good at merging VMAs back together after protection changes.

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