Simplify capsicumization of bhyve (cont'd)

[araujo]

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)

[oshogbo]

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]

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]

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

[araujo]

In D21053#456794, @oshogbo wrote:

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

[pmooney_pfmooney.com]

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?

[araujo]

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]

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]

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]

In D21053#459032, @markj wrote:

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?