Page MenuHomeFreeBSD

Simplify capsicumization of bhyve (cont'd)
AcceptedPublic

Authored by araujo on Jul 24 2019, 7:45 AM.

Details

Reviewers
oshogbo
jhb
markj
Group Reviewers
bhyve
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.

This change was initially part of review: https://reviews.freebsd.org/D18744
however instead to clean up all the WITHOUT_CAPSICUM at once, we opted to do incremental changes for safety only.

Test Plan
  • Smoke test built without CAPSICUM and CASPER Ex.: src.conf with "WITHOUT_CASPER and WITHOUT_CAPSICUM"
  • Tested with Fedora 30 and FreeBSD HEAD as guests. (basic devices: virtio-net, virtio-blk, ahci-cd and virtio-console)

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 25489
Build 24108: arc lint + arc unit

Event Timeline

araujo created this revision.Jul 24 2019, 7:45 AM

The problem here is that you will not be able to build this code on other platforms like Solaris.
I'm not sure if this is applicaple for bhyve, if not then the change looks good to me.

usr.sbin/bhyve/Makefile
80 ↗(On Diff #60083)

Why this is needed?
If you don't use any services, you don't have to include casper.

araujo added inline comments.Jul 24 2019, 8:00 AM
usr.sbin/bhyve/Makefile
80 ↗(On Diff #60083)

Ouch, it came from the previous review and I thought it were needed. Gonna remove and update the patch in a few.

araujo updated this revision to Diff 60085.Jul 24 2019, 8:00 AM

Remove CASPER from Makefile as we don't use any service.

The problem here is that you will not be able to build this code on other platforms like Solaris.
I'm not sure if this is applicaple for bhyve, if not then the change looks good to me.

Yes, I'm aware we have consumers of bhyve that are not based on FreeBSD.
I'm not sure either what would be the impact for them.

For Illumos-joyent seems it might be an issue in case they don't support CAPSICUM: https://github.com/joyent/illumos-joyent/blob/master/usr/src/cmd/bhyve/bhyverun.c

You're correct that illumos bhyve is not being built with any kind of Capsicum support. If the WITHOUT_CAPSICUM guards are removed, we'll just shim the cap_/caph_ interfaces as successful no-ops.

usr.sbin/bhyve/bhyverun.c
59

Why is vmm_dev.h needed here if Capsicum is enabled? If it's a matter of exposing the ioctl definitions for whitelisting, perhaps that should be part of libvmmapi?

markj accepted this revision as: markj.Jul 24 2019, 2:43 PM
This revision is now accepted and ready to land.Jul 24 2019, 2:43 PM
araujo added inline comments.Jul 25 2019, 4:29 AM
usr.sbin/bhyve/bhyverun.c
59

That is correct! It is used on vm_get_ioctls() and mostly to expose the VM_* definitions!

What I could do later on is to remove this vmm_dev.h dependency from bhyverun.c as it is the only place that uses it . If that sounds like a plan, I can work on it in the next weeks and perhaps come up with another patch.

Does it sounds good @pmooney_pfmooney.com?

jhb added a comment.Aug 1 2019, 12:01 AM

I worry this might be a bit premature. New features for bhyve are often developed without capsicum because capsicum is a pain to work with. The debug server didn't use it at first, save/restore currently disables it as does the 9p patch. Removing WITHOUT_CAPSICUM makes coordinating that existing work harder. It also adds a pretty good barrier to entry when trying to add new things that add new ioctls, etc. I think ideally I would like to have a way to disable capsicum on a given process dynamically (e.g. via proccontrol). Even better would be if you could disable it but still get ktrace records for when you would get a cap failure.

markj added a comment.Aug 1 2019, 2:25 PM
In D21053#458959, @jhb wrote:

I worry this might be a bit premature. New features for bhyve are often developed without capsicum because capsicum is a pain to work with. The debug server didn't use it at first, save/restore currently disables it as does the 9p patch. Removing WITHOUT_CAPSICUM makes coordinating that existing work harder. It also adds a pretty good barrier to entry when trying to add new things that add new ioctls, etc.

It's not much more work to add a new right in a driver that already works under capsicum than it is to add -DWITHOUT_CAPSICUM to the build flags.

I suspect you could address most of the pain by simply conditionalizing the call to caph_enter() and leaving the rest unconditional. But, I don't know how problematic the rights-limiting has been in practice.

I think ideally I would like to have a way to disable capsicum on a given process dynamically (e.g. via proccontrol).

We would have to be careful to ensure that this can't be used on setuid/setgid executables, at least.

Even better would be if you could disable it but still get ktrace records for when you would get a cap failure.

I'll see if I can get that done.

araujo added a comment.Aug 2 2019, 3:30 AM
In D21053#458959, @jhb wrote:

I worry this might be a bit premature. New features for bhyve are often developed without capsicum because capsicum is a pain to work with. The debug server didn't use it at first, save/restore currently disables it as does the 9p patch. Removing WITHOUT_CAPSICUM makes coordinating that existing work harder. It also adds a pretty good barrier to entry when trying to add new things that add new ioctls, etc.

It's not much more work to add a new right in a driver that already works under capsicum than it is to add -DWITHOUT_CAPSICUM to the build flags.

I need to agree with @markj here, although I had some difficulties with the HDA driver before and the driver got stuck, but nowadays capsicum got much easier to implement!!! @oshogbo do you have any insights or anything to say?

dch added a subscriber: dch.Mon, Nov 25, 2:19 PM