Page MenuHomeFreeBSD

Control implicit PROT_MAX() using proccontrol(1) and the FreeBSD note feature bit.
ClosedPublic

Authored by kib on Jun 28 2019, 6:16 PM.

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

kib created this revision.Jun 28 2019, 6:16 PM

Overall looks reasonable, a few small alpha ordering nits.

sys/compat/freebsd32/freebsd32_misc.c
3337 ↗(On Diff #59163)

alpha order

sys/vm/vm_mmap.c
59 ↗(On Diff #59163)

can this go in alpha order, other misordered headers notwithstanding?

usr.bin/proccontrol/proccontrol.c
76 ↗(On Diff #59163)

alpha order

bcr added a subscriber: bcr.Jun 28 2019, 6:42 PM

Suggested rewording of a sentence.

lib/libc/sys/procctl.2
129 ↗(On Diff #59163)

The image flag might still prevent the enablement.

kib marked 4 inline comments as done.Jun 28 2019, 7:31 PM
kib added inline comments.
sys/vm/vm_mmap.c
59 ↗(On Diff #59163)

Not sure what would be the right place then. Moved before filedesc.h.

markj added inline comments.Jun 30 2019, 6:08 PM
lib/libc/sys/procctl.2
119 ↗(On Diff #59163)

and .Xr mprotect 2

128 ↗(On Diff #59163)

It would be nice to reference the sysctl here, "disabled system-wide by vm.imply_prot_max"

131 ↗(On Diff #59163)

Should be "be disabled."

133 ↗(On Diff #59163)

Extra _ between PROT and MAX.

136 ↗(On Diff #59163)

"Use the system-wide ..."

kib updated this revision to Diff 59221.Jun 30 2019, 7:06 PM
kib marked an inline comment as done.

Man page update and minor code rearrangement.

markj accepted this revision.Jul 2 2019, 3:00 PM
This revision is now accepted and ready to land.Jul 2 2019, 3:00 PM
emaste accepted this revision.Jul 2 2019, 3:08 PM
brooks added inline comments.Jul 2 2019, 4:23 PM
lib/libc/sys/procctl.2
119 ↗(On Diff #59163)

We don't imply PROT_MAX on mprotect at this time. One could make a case for it, but we're never tested it and it's not as clear a case IMO.

sys/sys/syscallsubr.h
178 ↗(On Diff #59163)

It seems a little odd to me that this is here and not in some sys/vm/* header, but don't have a strong opinion.

This revision was automatically updated to reflect the committed changes.