Page MenuHomeFreeBSD

Fix a use-after-free in pmap_release().
ClosedPublic

Authored by markj on Jan 2 2019, 6:17 PM.

Details

Summary
  • Don't zero the page after freeing it.
  • Don't zero the page before removing it from allpmaps.

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

markj created this revision.Jan 2 2019, 6:17 PM
kib added inline comments.Jan 2 2019, 11:39 PM
sys/riscv/riscv/pmap.c
1306 ↗(On Diff #52488)

Why zeroing it at all ?

jhb accepted this revision.Jan 2 2019, 11:42 PM

One weird oddity. mips uses vm_page_free_zero() for it's top-level page but it doesn't zero it first. arm64 does the same. 32-bit arm v6 just leaks the pages (???), i386 uses vm_page_free() instead of vm_page_free_zero(). sparc64 uses vm_page_free_zero() without zero'ing the pages.

So it seems like mips, arm64, and sparc64 should all use vm_page_free() instead of vm_page_free_zero(). risc-v could also just do that instead of doing an explicit bzero here as well perhaps?

This revision is now accepted and ready to land.Jan 2 2019, 11:42 PM
kib added a comment.Jan 2 2019, 11:55 PM
In D18720#399435, @jhb wrote:

One weird oddity. mips uses vm_page_free_zero() for it's top-level page but it doesn't zero it first. arm64 does the same. 32-bit arm v6 just leaks the pages (???), i386 uses vm_page_free() instead of vm_page_free_zero(). sparc64 uses vm_page_free_zero() without zero'ing the pages.
So it seems like mips, arm64, and sparc64 should all use vm_page_free() instead of vm_page_free_zero(). risc-v could also just do that instead of doing an explicit bzero here as well perhaps?

I believe that the arches that use vm_page_free_zero() without zeroing, rely on the fact that all ptes are cleared before pmap is released. I.e. the page must be zero at this point. At least on arm64 vm_page_free() should be able to assert that the page is zeroed if DIAGNOSTIC is enabled.

And from the first hand experience, if the non-zero page returned to the free pool with PG_ZERO flag set, things breaks very fast, so it is visible, even if in puzzling manner.

markj added inline comments.Jan 3 2019, 12:03 AM
sys/riscv/riscv/pmap.c
1306 ↗(On Diff #52488)

Hmm, I think there's no reason. I will change it.

markj updated this revision to Diff 52499.Jan 3 2019, 12:06 AM
markj marked an inline comment as done.
  • Don't bother zeroing the top-level page.
This revision now requires review to proceed.Jan 3 2019, 12:06 AM
kib accepted this revision.Jan 3 2019, 12:19 AM
This revision is now accepted and ready to land.Jan 3 2019, 12:19 AM
This revision was automatically updated to reflect the committed changes.