Page MenuHomeFreeBSD

Simplify capsicumization of bhyve.
ClosedPublic

Authored by araujo on Fri, Jan 4, 12:45 PM.

Details

Summary

Use capsicum_helpers(3) that allow us to simplify the code and
its functions will return success when the kernel is built without
support of the capability mode.

NOTE: This is the first round of capsicum review, the next round would be to apply cap_fileargs(3).
Test Plan
  • Smoke test built without CAPSICUM and CASPER. Ex.: "-DWITHOUT_CAPSICUM"
  • Tested with guests: Linux Fedora 29 and FreeBSD HEAD.
  • Tested connection with VNC.
  • Tested connection with CONSOLE (nmdm).

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

araujo created this revision.Fri, Jan 4, 12:45 PM
araujo edited the summary of this revision. (Show Details)Fri, Jan 4, 12:49 PM
araujo added reviewers: oshogbo, bhyve.
emaste added a comment.Fri, Jan 4, 1:09 PM

Smoke test built without CAPSICUM and CASPER. Ex.: "-DWITHOUT_CAPSICUM"

But all of the WITHOUT_CAPSICUM #ifdefs have been removed?

usr.sbin/bhyve/Makefile
78 ↗(On Diff #52549)

Is this part of the next change?

araujo added a comment.Fri, Jan 4, 1:11 PM

Smoke test built without CAPSICUM and CASPER. Ex.: "-DWITHOUT_CAPSICUM"

But all of the WITHOUT_CAPSICUM #ifdefs have been removed?

Most of it were remove with one single exception that is related with a header vmm_dev.h that I need to check if it is possible to merge with vmm.h.

araujo added inline comments.Fri, Jan 4, 1:13 PM
usr.sbin/bhyve/Makefile
78 ↗(On Diff #52549)

Yes it is! Also as I'm working on review D12419, I thought would be OK to add cap_fileargs right now.

araujo marked an inline comment as done.Fri, Jan 4, 4:10 PM
jhb added a subscriber: jhb.Fri, Jan 4, 5:11 PM

I stopped saying "same" after a while to avoid being too repetitious. I think it would make sense to have one commit that first fixes existing places not yet using helpers to use the helpers by replacing all uses of cap_rights_limit and cap_ioctls_limit with the helper variants. I would keep the == -1 checks though. The syscalls are defined to return -1 on error, not a random -ve value. From cap_rights_limit(2):

RETURN VALUES
     Upon successful completion, the value 0 is returned; otherwise the value -1
     is returned and the global variable errno is set to indicate the error.

You could instead perhaps fail for any return value that isn't 0 (e.g use '!= 0'), but using '< 0' is just kind of odd (we fail for half of the undefined values (negative ones), but succeed for the other half (positive values > 0). Using either '== -1' or '!= 0' is more consistent in that all undefined values are treated the same. Personally I would probably use != 0, but the existing code already uses == -1, so I would leave it as is to minimize the diff.

I would then probably do a second commit to remove WITHOUT_CAPSICUM and a third commit to add casper for the audio stuff.

usr.sbin/bhyve/bhyverun.c
942 ↗(On Diff #52549)

I don't think this comment adds much value so would probably leave it out.

944 ↗(On Diff #52549)

This needs to be caph_rights_limit instead of cap_rights_limit? Also, checking for == -1 as the existing code did is ok. (Syscalls are generally defined as returning exactly -1 on error)

950 ↗(On Diff #52549)

Similar, s/cap_/caph_/ and I would keep the == -1 to minimize the diff.

1198 ↗(On Diff #52549)

I think == -1 is fine and would just leave that as is. The only real change here is to remove the WITHOUT_CAPSICUM it seems.

usr.sbin/bhyve/block_if.c
471 ↗(On Diff #52549)

Similar comments, s/cap_/caph_/ and I would keep the == -1.

500 ↗(On Diff #52549)

Same.

usr.sbin/bhyve/consport.c
136 ↗(On Diff #52549)

Same.

usr.sbin/bhyve/dbgport.c
165 ↗(On Diff #52549)

Same.

usr.sbin/bhyve/gdb.c
164 ↗(On Diff #52549)

I would keep the == -1.

araujo updated this revision to Diff 52604.Sun, Jan 6, 7:13 AM

Following @jhb suggestion, I will break down these updates in chunks.

First of all, I will replace functions that are included on capsicum_helpers(3),
then after a while I will remove the #ifndef.

Second of all, I need to apologize, I have used devel/coccinelle to make this
code transformation and I wrote coccinelle's script wrong regarding the
replacement of s/cap_/caph_/. I won't do that again.

araujo marked 9 inline comments as done.Sun, Jan 6, 7:16 AM
jhb accepted this revision.Tue, Jan 15, 6:32 PM

Thanks! Hopefully this means 'grep -c ENOSYS' in bhyve is now zero. :)

This revision is now accepted and ready to land.Tue, Jan 15, 6:32 PM
In D18744#402454, @jhb wrote:

Thanks! Hopefully this means 'grep -c ENOSYS' in bhyve is now zero. :)

I was curious too:
[araujo@pipoca] /usr/src/usr.sbin/bhyve# grep -c ENOSYS .
0

:P

This revision was automatically updated to reflect the committed changes.