Capsicumize cpuset_*

[allanjude]

Summary

bhyve was recently converted to the capsicum sandbox, and needs to be able to control the CPU sets of its vcpu threads

[allanjude]

sys/kern/kern_cpuset.c

339 ↗

(On Diff #26746)

should this be allowed inside capsicum? check against IN_CAPABILITY_MODE(curthread) or something?

357 ↗

(On Diff #26746)

same here

516 ↗

(On Diff #26746)

allowed?

694 ↗

(On Diff #26746)

only if id = -1?

995 ↗

(On Diff #26746)

eventually this might allow setting on a thread, should the if consider that, so that if the following condition is changed/removed, it doesn't block things?

[cem]

sys/kern/kern_cpuset.c

50–51 ↗

(On Diff #26746)

These seem sorted wrong

339 ↗

(On Diff #26746)

Why not allow it? It's just a helper and can only restrict the set(s) further. I don't think a check is needed here.

516 ↗

(On Diff #26746)

Only if pid == -1, right?

694 ↗

(On Diff #26746)

Right.

995 ↗

(On Diff #26746)

Yeah, why not follow the same pattern you've used below?

[emaste]

Oops, forgot to hit "Submit," so sorry if some of these comments are OBE.

sys/kern/kern_cpuset.c

50 ↗

(On Diff #26746)

Hmm, this file seems to be mostly unsorted already :( I suppose supporting with another 'c' header is as good as anywhere.

339 ↗

(On Diff #26746)

Yep, this helper doesn't need to be aware of capsicum.

995 ↗

(On Diff #26746)

These CPU sets refer to a global namespace; these should not be permitted in capability mode at all I think?

1108–1112 ↗

(On Diff #26746)

This one looks good to me

1218 ↗

(On Diff #26746)

I think that's out of scope for Capsicum.

[allanjude]

Update based on feedback

sys/kern/kern_cpuset.c

995 ↗

(On Diff #26746)

the setid being a global thing? I suppose that makes sense.

[allanjude]

Address feedback from capsicum call

[emaste]

sys/kern/init_sysent.c

532–536 ↗

(On Diff #26871)

Presumably this was just not regenerated

sys/kern/kern_cpuset.c

527 ↗

(On Diff #26871)

leftover blank line from refactoring?

[rwatson]

I like the overall approach especially after various changes to do the checks only in the system calls themselves, not in the common helper functions.

sys/kern/capabilities.conf

137 ↗

(On Diff #26871)

This comment will need updating.

sys/kern/kern_cpuset.c

1106 ↗

(On Diff #26871)

I find this check pretty hard to read and understand. Is there some way to refactor it to use a nested if in an outside block conditioned in IN_CAPABILITY_MODE() and possibly a check on level to make it more readable?

1214 ↗

(On Diff #26871)

Ditto here. I find the complex boolean expression hard to follow, and hence convince myself it is right. Can this become a nested if..?

[emaste]

sys/kern/kern_cpuset.c

1106 ↗

(On Diff #26871)

So something like:

if (IN_CAPABILITY_MODE(td)) {
        if (level ...
                return (ECAPMODE);
}

Then do you think the inner if is more clear as
if (!(level == CPU_LEVEL_WHICH && (which == CPU_WHICH_TID || which == CPU_WHICH_PID) && id == -1))
or
if (level != CPU_LEVEL_WHICH || (which != CPU_WHICH_TID && which != CPU_WHICH_PID) || id != -1)

[emaste]

sys/kern/kern_cpuset.c

1106 ↗

(On Diff #26871)

or even something like

if (level != CPU_LEVEL_WHICH)
        return (ECAPMODE);
if (which != CPU_WHICH_TID && which != CPU_WHICH_PID)
        return (ECAPMODE);
if (id != -1)
        return (ECAPMODE);

[rwatson]

sys/kern/kern_cpuset.c

1106 ↗

(On Diff #26871)

Yes, this sounds good (one big outside check for IN_CAPABILITY_MODE(td), and then the sequence of if() checks). It's now much more obvious to me what the policy is.

[allanjude]

Update with feedback from rwatson

[rwatson]

sys/kern/capabilities.conf

136 ↗

(On Diff #27502)

Possibly should be "caller's" or "callers'" (depending on whether you intend the plural or not).

sys/kern/init_sysent.c

535 ↗

(On Diff #27502)

Do be sure to commit generated system-call files in a seperate commit (as @kib has rightly reminded me regarding recent commits).

[rwatson]

FYI, it may be desirable to add a note about scoping of cpuset*(2) to the capsicum(4) man page. We should probably extend that man page in other ways to describe other sorts of scoping in place, but that's a separate task...

[kib]

compat32 capabilities.conf requires the same changes, and corresponding tables must be regenerated as well, in the follow-up commit.

[emaste]

In D10170#215907, @rwatson wrote:

FYI, it may be desirable to add a note about scoping of cpuset*(2) to the capsicum(4) man page. We should probably extend that man page in other ways to describe other sorts of scoping in place, but that's a separate task...

Ah, yes, and also add ECAPMODE to .Sh ERRORS.

[emaste]

Looks good (with the recent comments by kib and rwatson addressed)

[grehan]

Any chance this could be committed ?

[allanjude]

In D10170#223427, @grehan wrote:

Any chance this could be committed ?

Yes, sorry, I just had not had time to write the man page changes for it

[allanjude]

Add man page changes

[allanjude]

The two 'sysent' files should be done as a second commit, is that all that needs to be done?

[kib]

In D10170#223438, @allanjude wrote:

The two 'sysent' files should be done as a second commit, is that all that needs to be done?

Yes, generated files should be committed separately.

[wblock]

lib/libc/sys/cpuset_getaffinity.2

152 ↗

(On Diff #28499)

Probably better to say "is only allowed to" instead of "may".

155 ↗

(On Diff #28499)

This needs to be added to the SEE ALSO section, too.

[emaste]

LGTM in general. Minor comment on the man page wording.

lib/libc/sys/cpuset_getaffinity.2

152 ↗

(On Diff #28499)

For consistency with typical error descriptions, should it not be something like "The calling process attempted to act on a process other than itself, in capability mode."? It seems a bit tortured, but the error descriptions always seem to be "this is what the caller did wrong."

[wblock]

head/lib/libc/sys/cpuset_getaffinity.2

158

This is in the wrong place. This list is sorted by category first, then alpha.