procctl(2) and proccontrol(1): add W^X control
ClosedPublic
Actions

Authored by kib on Sep 2 2021, 3:26 AM.

Details

Reviewers

emaste
markj
brooks

Commits

rG796a8e1ad1ae: procctl(2): Add PROC_WXMAP_CTL/STATUS
rGac8af1938085: proccontrol(1): Add wxmap control
rG1349891a0eed: Style

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

I checked that go binaries that have no PT_GNU_STACK segment and no elfctl note to disable wxorx for the image, do start with kern.elf64.allow_wx=0 and 'proccontrol -m wxorx -s disable <go program>'

markj added inline comments.Sep 2 2021, 3:22 PM

lib/libc/sys/procctl.2
618 ↗	(On Diff #94525)
622 ↗	(On Diff #94525)
629 ↗	(On Diff #94525)
643 ↗	(On Diff #94525)
644 ↗	(On Diff #94525)
648 ↗	(On Diff #94525)
653 ↗	(On Diff #94525)
654 ↗	(On Diff #94525)
708 ↗	(On Diff #94525)
711 ↗	(On Diff #94525)	I would perhaps s/like JIT// and add another sentence along the lines of, "This may be legitimately required by some programs, such as JIT compilers."
sys/kern/kern_procctl.c
605 ↗	(On Diff #94525)	I think you need a P_WEXIT check here as you have below.
643 ↗	(On Diff #94525)	What's the point of unlocking the proc at all?

kib marked 12 inline comments as done.Sep 2 2021, 6:26 PM

kib added inline comments.

sys/kern/kern_procctl.c
643 ↗	(On Diff #94525)	vmspace_acquire_ref() potentially results in the call to vmspace_free()->vmspace_dofree()->VM locking (e.g. sleepable vm map lock take)

Regularize prologue in wxorx_status/ctl, check P_WEXIT and assert process lock.
Man page language fixes.

markj accepted this revision.Sep 10 2021, 1:13 PM

markj added inline comments.

lib/libc/sys/procctl.2
608 ↗	(On Diff #94568)

This revision is now accepted and ready to land.Sep 10 2021, 1:13 PM

kib marked an inline comment as done.Sep 10 2021, 1:25 PM

emaste added inline comments.Sep 10 2021, 6:12 PM

lib/libc/sys/procctl.2
606 ↗	(On Diff #94568)	I'm concerned by the different sense of the two flags, WXORX being enabled is equivalent to allow_wx=0.
617 ↗	(On Diff #94568)	this is why I desire positive sense flags (and why sysctl(9) recommends positive sense): this `*_DISABLE` flag enables something, which can be confusing
622 ↗	(On Diff #94568)	and `_ENABLE_` prevents something

markj added inline comments.Sep 16 2021, 5:26 PM

lib/libc/sys/procctl.2
617 ↗	(On Diff #94568)	This problem exists no matter what, it's just a question of what you're disabling: the mitigation, or the ability to create writeable, executable mappings. PROC_WXORX_DISABLE and PROC_WXORX_ENABLE_ON_EXEC both clearly refer to the mitigation. A more explicit naming scheme would be to have PROC_PERMIT_WX_MAPPINGS and PROC_PERMIT_WX_MAPPINGS_ON_EXEC or something similar. That is, instead of referring to the mitigation, refer to the underlying capability. I think I slightly prefer that approach since it's a bit clearer and agrees with the sysctl.