Page MenuHomeFreeBSD

bhyve: Use MAP_GUARD guard pages
ClosedPublic

Authored by lattera-gmail.com on Aug 21 2018, 1:32 AM.

Details

Summary

The memory space of virtual machines is protected by guard pages (one guard mapping at the beginning of the VM space and one guard mapping at the end of the VM space) . Instead of relying solely on PROT_NONE mappings, use MAP_GUARD mappings as the guard pages. Note that the size of the guard mapping (4MB) is left unchanged.

Sponsored by: HardendBSD and G2, Inc

Test Plan
  1. Apply patch
  2. Run your bhyve VMs as you normally do

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

lattera-gmail.com edited the summary of this revision. (Show Details)Aug 21 2018, 1:34 AM
mmokhi added a subscriber: mmokhi.Aug 28 2018, 2:46 PM
jhb added a reviewer: kib.Aug 28 2018, 7:52 PM
jhb added a subscriber: jhb.Aug 28 2018, 8:03 PM

Adding @kib since he added MAP_GUARD. I think you should instead MAP_GUARD the entire range first and then remap the middle. The middle is already remapped on line 518, so instead of adding new mmap()'s just for the guards, you should replace the mmap() on line 496 with this:

base = mmap(NULL, len2, PROT_NONE, MAP_GUARD | MAP_ALIGNED_SUPER, -1, 0);
In D16822#361106, @jhb wrote:

Adding @kib since he added MAP_GUARD. I think you should instead MAP_GUARD the entire range first and then remap the middle. The middle is already remapped on line 518, so instead of adding new mmap()'s just for the guards, you should replace the mmap() on line 496 with this:

base = mmap(NULL, len2, PROT_NONE, MAP_GUARD | MAP_ALIGNED_SUPER, -1, 0);

Thanks for the feedback! I'll give that a shot and report back soon.

Update the patch to use John Baldwin's suggestion on mapping the entire range first with MAP_GUARD.

Missed a spot. Cover another mapping with MAP_GUARD.

kib added a comment.Aug 28 2018, 8:55 PM

The only useful feature of the phabricator is to easily see context around the patch, which you successfully botched.

lib/libvmmapi/vmmapi.c
392 ↗(On Diff #47401)

flags is single-use and thus pointless.

402 ↗(On Diff #47401)

I munmap(2) fails, the interesting errno value is obliterated.

503 ↗(On Diff #47401)

Again flags use is pointless. Before your patch, it was used to avoid line wrapping in the mmap() line, most likely.

510 ↗(On Diff #47401)

Weird line space use.

jhb added a comment.Aug 28 2018, 9:06 PM

You should only be replacing the existing MAP_ANON / PROT_NONE mmap() calls with MAP_GUARD instead. You shouldn't be adding new mmap() calls. The "real" thing is already being mmap()'d later. As @kib notes, it's good to use 'svn diff -x -U 99999' or the like to generate context when uploading to phabricator.

Reflect changes requested by both kib and jhb.

lattera-gmail.com marked 4 inline comments as done.Aug 28 2018, 9:23 PM
In D16822#361129, @kib wrote:

The only useful feature of the phabricator is to easily see context around the patch, which you successfully botched.

I'm still getting used to Phabricator. Please be patient.

jhb added inline comments.Aug 28 2018, 9:45 PM
lib/libvmmapi/vmmapi.c
397 ↗(On Diff #47406)

You don't need this second call to mmap(). setup_memory_segment() maps the appropriate regions of memory via mmap() with MAP_FIXED below. You should just keep the existing line that does 'baseaddr = ptr + VM_MMAP_GUARD_SIZE;'.

499 ↗(On Diff #47406)

I would probably leave the existing comment as it is already accurate. I would also probably leave the len2 helper variable to minimize the diff.

lib/libvmmapi/vmmapi.c
397 ↗(On Diff #47406)

Ah, okay. I didn't realize setup_memory_segment did that. I'll remove this call to mmap.

499 ↗(On Diff #47406)

I'll add the comment back in.

As far as len2 is concerned, I figured that since kib complained about the already-existing flags variable being single-use, I'd also get rid of len2, since it, too, is single-use. I can revert that change, though.

Minimize diff with suggestions by jhb.

lattera-gmail.com marked 4 inline comments as done.Aug 28 2018, 9:55 PM
jhb added a comment.Aug 28 2018, 11:04 PM

This looks right to me now. I'll try to test it locally in the next day or so.

In D16822#361197, @jhb wrote:

This looks right to me now. I'll try to test it locally in the next day or so.

Thanks a lot for the review and the suggestions! For what it's worth, I've been running with this patch with various operating systems (Win10, FreeBSD, HardenedBSD, and CentOS) since the last revision. No issues to report on my end.

araujo accepted this revision as: araujo.Sep 4 2018, 1:51 AM

I have tested it with FreeBSD HEAD as a guest running for couple days.

This revision is now accepted and ready to land.Sep 4 2018, 1:51 AM
This revision was automatically updated to reflect the committed changes.