Details

Reviewers

trasz
markj

Commits

rS367290: linux(4) prctl(2): Implement PR_[GS]ET_DUMPABLE

Summary

Proxy the flag to the roughly analogous FreeBSD procctl 'TRACE'.

TRACE-disabled processes are not coredumped, and Linux !DUMPABLE processes
can not be ptraced. There are some additional semantics around ownership of
files in the /proc/[pid] pseudo-filesystem, which we do not attempt to
emulate correctly at this time.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

cem requested review of this revision.Oct 29 2020, 9:10 PM

cem created this revision.

Harbormaster completed remote builds in B34488: Diff 78928.Oct 29 2020, 9:10 PM

Stub wasn't sufficient; chrome self-tests that disabling dump is reflected in
subsequent GET. Instead, proxy the prctl to the similar procctl PROC_TRACE.

Harbormaster completed remote builds in B34489: Diff 78929.Oct 29 2020, 9:26 PM

This diff doesn't appear to match the description.

sys/compat/linux/linux_misc.c
1918 ↗	(On Diff #78929)	aldo -> also

In D27015#602732, @markj wrote:

This diff doesn't appear to match the description.

Ah, yes. Arc diff update doesn’t update the commit message text on phabricator. I’ve rewritten the text somewhat and will update it manually in a bit.

Thanks for taking a look at all of these.

sys/compat/linux/linux_misc.c
1918 ↗	(On Diff #78929)	Fixed locally, will update in a minute. I should probably also drop the comment about ownership— SUID_DUMP_ROOT was short-lived and is no longer an acceptable mode.
1939 ↗	(On Diff #78929)	Caveat: I have no idea if Linux inherits this flag across exec or not.

Fix typo

Harbormaster completed remote builds in B34512: Diff 78973.Oct 30 2020, 6:24 PM

cem retitled this revision from linux(4) prctl(2): Stub implement PR_[GS]ET_DUMPABLE to linux(4) prctl(2): Implement PR_[GS]ET_DUMPABLE.Oct 30 2020, 6:25 PM

cem edited the summary of this revision. (Show Details)

Update the comment. No functional change.

Harbormaster completed remote builds in B34513: Diff 78974.Oct 30 2020, 6:30 PM

markj added inline comments.Nov 2 2020, 2:33 PM

sys/compat/linux/linux_misc.c
1932 ↗	(On Diff #78974)	Why is `id` 0 here? I don't see any special handling in kern_procctl() for that case: `id` gets passed to pfind().

cem added inline comments.Nov 2 2020, 8:09 PM

sys/compat/linux/linux_misc.c
1932 ↗	(On Diff #78974)	Copied from `PROC_PDEATHSIG_STATUS` above, but now I see that has special handling in `kern_procctl`. So yes, we need to explicitly include curthread's pid.

Fix PID passed to kern_procctl. Previously, the syscall would manipulate the tracability of the kernel 0 process, which is probably meaningless and certainly not intended.

Harbormaster completed remote builds in B34574: Diff 79089.Nov 2 2020, 8:30 PM

markj accepted this revision.Nov 2 2020, 10:41 PM

markj added inline comments.

sys/compat/linux/linux_misc.c
1981 ↗	(On Diff #79089)	For LINUX_PR_SET_DUMPABLE we unconditionally return but here we only do so upon error.
1939 ↗	(On Diff #78929)	By my reading of Linux the "dumpable" flags are preserved across execve, so I think this is right. bprm_mm_init() calls mm_alloc(), which preserves flags from the current mm if they are in MM_INIT_MASK, which includes MMF_DUMPABLE_MASK.

This revision is now accepted and ready to land.Nov 2 2020, 10:41 PM

cem added inline comments.Nov 3 2020, 1:05 AM

sys/compat/linux/linux_misc.c
1981 ↗	(On Diff #79089)	The `break` is essentially just `return (error)`. The prevailing pattern seems to use `break`, but given we don't have other cleanup to perform and we exit early in a number of error cases, I'm inclined to reconcile this by explicitly returning in `GET_DUMPABLE` as well.
1939 ↗	(On Diff #78929)	Thanks for checking.