hwpmc: Fix the exec handler

[markj]

Summary

When a process execve()s, pmc_process_exec() is supposed to evaluate
whether the new image is setuid/setgid and if so, whether to detach
PMCs. This was handled by pmc_can_attach(), which is effectively an
open-coded copy of cr_xids_subset().

Unfortunately, the test of the result of this function was inverted,
with the result that we'd detach PMCs only if the predicate said it was
okay to do so. I believe the bug has always been there; it seems the
intent was to return 0 on "success", i.e., it is okay to attach the
PMCs, much like p_candebug(). Commits 1c3c698ba4c4 and 1c40b15971f0
obscured this a bit.

I think this check is trying to be too clever. Let's make it simpler:
simply do not attach PMCs unless the owner is privileged. This is how,
e.g., ktrace works. I do not think it's worth trying to be more sophisticated
than this unless we can generalize the policy in a way that's applicable
to other subsystems; I am squinting at hwt as well, but that seems to be
limited to privileged users. For a security patch, I'd rather keep things simple.

Also fix a bug at the end of pmc_process_exec():
pmc_detach_one_process() will call pmc_remove_process_descriptor() for
us.

A subsequent patch by netchild adds some regression tests.

Reported by: netchild

[netchild]

My tests detect a regression:

pm_credentialschanged does not mean the credentials changed. kern_exec.c:1040 assigns it credential_changing, computed at :695-703 from the image's mode bits alone. It is only afterwards, at :710-715, that the set-id is suppressed when the mount is MNT_NOSUID, the target is P_TRACED, or the process is in capability mode — and the flag hwpmc receives carries the unsuppressed value. imgp->credential_setid (:716) is the one that means the credentials were actually replaced; P_SUGID is cleared at :975 in the suppressed case, and the uids and gids are untouched.

The old comparison absorbed that by accident: the owner's ids still matched the target's unchanged ids, so it said "may attach" and nothing was detached. With the comparison gone there is nothing left to notice, so an unprivileged owner now loses its PMC across an exec in which its target changed nothing.

Measured, main + D59102, my exec suite (32 cases, 31 pass, 1 fail; the same suite is 32/32 on a kernel carrying my series):

pmc_credexec_test exec_setid_traced_keeps_pmc failed:

the PMC was dropped although tracing left the target's credentials
unchanged and its owner still entitled

MNT_NOSUID is the same code path and I measured it KEPT under my series; capability mode is the third instance and I have no case for it yet.

Suggested fix, which keeps your simplification. Feed the flag from the suppressed-aware value (kern_exec.c:1040):

	if (PMC_SYSTEM_SAMPLING_ACTIVE() || PMC_PROC_IS_USING_PMCS(p)) {
		VOP_UNLOCK(imgp->vp);
-		pe.pm_credentialschanged = credential_changing;
+		pe.pm_credentialschanged = imgp->credential_setid;

filemon already does it this way — filemon_wrapper.c:127-129 pairs the same priv_check_cred(cred, PRIV_DEBUG_DIFFCRED) with imgp->newcred != NULL && imgp->credential_setid.
With this change, my 32 test cases pass (= no regression to what we have right now, except the security issue gone).

One note on the summary text: "do not attach PMCs unless the owner is privileged" reads as a policy on PMC_OP_PMCATTACH, which this does not touch — pmc_can_attach(), despite the name, had exactly one caller and it was the exec path. So this applies at a credential-changing exec.

[markj]

Handle review feedback

[markj]

In D59102#1355583, @netchild wrote:

My tests detect a regression:

pm_credentialschanged does not mean the credentials changed. kern_exec.c:1040 assigns it credential_changing, computed at :695-703 from the image's mode bits alone. It is only afterwards, at :710-715, that the set-id is suppressed when the mount is MNT_NOSUID, the target is P_TRACED, or the process is in capability mode — and the flag hwpmc receives carries the unsuppressed value. imgp->credential_setid (:716) is the one that means the credentials were actually replaced; P_SUGID is cleared at :975 in the suppressed case, and the uids and gids are untouched.

I see, thanks. credentials_changed isn't a great variable name.

The old comparison absorbed that by accident: the owner's ids still matched the target's unchanged ids, so it said "may attach" and nothing was detached. With the comparison gone there is nothing left to notice, so an unprivileged owner now loses its PMC across an exec in which its target changed nothing.

Measured, main + D59102, my exec suite (32 cases, 31 pass, 1 fail; the same suite is 32/32 on a kernel carrying my series):

pmc_credexec_test exec_setid_traced_keeps_pmc failed:

the PMC was dropped although tracing left the target's credentials
unchanged and its owner still entitled

MNT_NOSUID is the same code path and I measured it KEPT under my series; capability mode is the third instance and I have no case for it yet.

Suggested fix, which keeps your simplification. Feed the flag from the suppressed-aware value (kern_exec.c:1040):

	if (PMC_SYSTEM_SAMPLING_ACTIVE() || PMC_PROC_IS_USING_PMCS(p)) {
		VOP_UNLOCK(imgp->vp);
-		pe.pm_credentialschanged = credential_changing;
+		pe.pm_credentialschanged = imgp->credential_setid;

filemon already does it this way — filemon_wrapper.c:127-129 pairs the same priv_check_cred(cred, PRIV_DEBUG_DIFFCRED) with imgp->newcred != NULL && imgp->credential_setid.
With this change, my 32 test cases pass (= no regression to what we have right now, except the security issue gone).

Yes, thanks. Probably exec should just be passing imgp directly to these hooks. (HWT has a nearly duplicate hook, but it doesn't seem to check for setuid/setgid switches. OTOH it is currently only usable by privileged users, at least today. Something to fix, but not in this patch.)

One note on the summary text: "do not attach PMCs unless the owner is privileged" reads as a policy on PMC_OP_PMCATTACH, which this does not touch — pmc_can_attach(), despite the name, had exactly one caller and it was the exec path. So this applies at a credential-changing exec.

Ok, I will fix the commit log message. Thanks for taking a look.

[netchild]

In D59102#1356034, @markj wrote:

In D59102#1355583, @netchild wrote:

	if (PMC_SYSTEM_SAMPLING_ACTIVE() || PMC_PROC_IS_USING_PMCS(p)) {
		VOP_UNLOCK(imgp->vp);
-		pe.pm_credentialschanged = credential_changing;
+		pe.pm_credentialschanged = imgp->credential_setid;

filemon already does it this way — filemon_wrapper.c:127-129 pairs the same priv_check_cred(cred, PRIV_DEBUG_DIFFCRED) with imgp->newcred != NULL && imgp->credential_setid.
With this change, my 32 test cases pass (= no regression to what we have right now, except the security issue gone).

Yes, thanks. Probably exec should just be passing imgp directly to these hooks. (HWT has a nearly duplicate hook, but it doesn't seem to check for setuid/setgid switches. OTOH it is currently only usable by privileged users, at least today. Something to fix, but not in this patch.)

I'm not sure I follow for your "only usable by privileged users", maybe I'm misunderstanding. Are you talking about that in general or for this particular case of the patch? PMC can be used by unprivileged users (not system level stuff, but process level stuff).

[markj]

In D59102#1356093, @netchild wrote:

In D59102#1356034, @markj wrote:

In D59102#1355583, @netchild wrote:

	if (PMC_SYSTEM_SAMPLING_ACTIVE() || PMC_PROC_IS_USING_PMCS(p)) {
		VOP_UNLOCK(imgp->vp);
-		pe.pm_credentialschanged = credential_changing;
+		pe.pm_credentialschanged = imgp->credential_setid;

filemon already does it this way — filemon_wrapper.c:127-129 pairs the same priv_check_cred(cred, PRIV_DEBUG_DIFFCRED) with imgp->newcred != NULL && imgp->credential_setid.
With this change, my 32 test cases pass (= no regression to what we have right now, except the security issue gone).

Yes, thanks. Probably exec should just be passing imgp directly to these hooks. (HWT has a nearly duplicate hook, but it doesn't seem to check for setuid/setgid switches. OTOH it is currently only usable by privileged users, at least today. Something to fix, but not in this patch.)

I'm not sure I follow for your "only usable by privileged users", maybe I'm misunderstanding. Are you talking about that in general or for this particular case of the patch? PMC can be used by unprivileged users (not system level stuff, but process level stuff).

I'm talking about hwt(4), which has similar hooks to hwpmc(4). It uses an ioctl interface to configure tracing, and /dev/hwt is 0660 root:wheel.