Page MenuHomeFreeBSD

Fix two issues with mprotect(PROT_WRITE) and wiring.
ClosedPublic

Authored by kib on Sep 25 2018, 8:16 PM.
Tags
None
Referenced Files
F106121980: D17323.id48487.diff
Wed, Dec 25, 6:39 PM
Unknown Object (File)
Mon, Dec 9, 5:51 PM
Unknown Object (File)
Sat, Dec 7, 6:05 AM
Unknown Object (File)
Oct 6 2024, 5:50 PM
Unknown Object (File)
Sep 25 2024, 12:33 AM
Unknown Object (File)
Sep 21 2024, 2:21 AM
Unknown Object (File)
Sep 18 2024, 12:13 AM
Unknown Object (File)
Sep 17 2024, 7:38 PM
Subscribers

Details

Summary

There are two fixes for vm_fault_copy_entry():

  • We should not assert that entry is charged if the dst_object is not of swap type. It can only happen when entry does not require copy, otherwise vm_map_protect() already adds the charge. So the assert was right for the case where swap object was allocated in the vm_fault_copy_entry(), but not when it was just copied from src_entry and its type is not swap.
  • if a wired map entry is backed by vnode and the file is truncated, corresponding pages are invalidated. vm_fault_copy_entry() should be aware of it and allow for invalid pages past end of file. Also, such pages should be not mapped into userspace.

Found by: andrew using syzkaller

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kib marked an inline comment as done.Sep 25 2018, 8:17 PM
kib added inline comments.
sys/vm/vm_fault.c
1745 ↗(On Diff #48461)

As I noted elsewere, this assert might be reasonable dropped at all due to msync(MS_INVALIDATE).

sys/vm/vm_fault.c
1653 ↗(On Diff #48461)

I think it clearer to include the new conditions here, and not in the KASSERT:

} else if ((dst_object->type == OBJT_DEFAULT ||
    dst_object->type == OBJT_SWAP)  &&
    dst_object->cred == NULL)

This will add to the run-time cost of handling DEFAULT and SWAP objects, but eliminate the pointless assignments for other types of objects.

1745 ↗(On Diff #48461)

Thinking ...

sys/vm/vm_fault.c
1642 ↗(On Diff #48461)

On an unrelated note, couldn't the assignments here to the object move up to the previous "else"? They don't need to be here any more than the assignments to pg_color or flags. (The object is effectively private for a short time after creation.)

kib marked an inline comment as done.

Move all dst_object initialization lines into creation block.
Rearrange conditional in if vs KASSERT.

sys/vm/vm_fault.c
1738 ↗(On Diff #48487)

Couldn't we simply break out of the enclosing "for" loop here if src_m is after the end of the object? Then, neither the KASSERT nor pmap_enter() call would have to change. Moreover, further iterations of the loop are pointless.

kib marked an inline comment as done.

End the loop if out of bounds.

sys/vm/vm_fault.c
1742–1749 ↗(On Diff #48530)

Can't you reorder things so that the xunbusy() isn't needed? In other words, move the xbusy() below the "if" statement. Otherwise, this looks good to me.

Remove xunbusy(). Reduce diff.

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