svn commit: r313727 - in head: lib/libvmmapi usr.sbin/bhyve

[John Baldwin]

On Tuesday, February 14, 2017 01:35:59 PM Bartek Rutkowski wrote:
> Author: robak (ports committer)
> Date: Tue Feb 14 13:35:59 2017
> New Revision: 313727
> URL: https://svnweb.freebsd.org/changeset/base/313727
> 
> Log:
>   Capsicum support for bhyve(8).
>   
>   Adds Capsicum sandboxing to bhyve.
>   
>   Submitted by:	Pawel Biernacki <pawel.biernacki at gmail.com>
>   Reviewed by:	grehan, oshogbo
>   Approved by:	emaste, grehan
>   Sponsored by:	Mysterious Code Ltd.
>   Differential Revision:	https://reviews.freebsd.org/D8290

Neat, though it adds a bit more work to future changes.  (I have WIP to add
a GDB server to bhyve that adds more ioctls, etc.  I can probably just
compile without capsicum though until the set of ioctls stabilizes.)

> Modified: head/lib/libvmmapi/vmmapi.c
> ==============================================================================
> --- head/lib/libvmmapi/vmmapi.c	Tue Feb 14 04:52:24 2017	(r313726)
> +++ head/lib/libvmmapi/vmmapi.c	Tue Feb 14 13:35:59 2017	(r313727)
> @@ -1416,3 +1416,45 @@ vm_restart_instruction(void *arg, int vc
>  
>  	return (ioctl(ctx->fd, VM_RESTART_INSTRUCTION, &vcpu));
>  }
> +
> +int
> +vm_get_device_fd(struct vmctx *ctx)
> +{
> +
> +	return (ctx->fd);
> +}
> +
> +const cap_ioctl_t *
> +vm_get_ioctls(size_t *len)
> +{
> +	cap_ioctl_t *cmds;
> +	/* keep in sync with machine/vmm_dev.h */
> +	static const cap_ioctl_t vm_ioctl_cmds[] = { VM_RUN, VM_SUSPEND, VM_REINIT,
> +	    VM_ALLOC_MEMSEG, VM_GET_MEMSEG, VM_MMAP_MEMSEG, VM_MMAP_MEMSEG,
> +	    VM_MMAP_GETNEXT, VM_SET_REGISTER, VM_GET_REGISTER,
> +	    VM_SET_SEGMENT_DESCRIPTOR, VM_GET_SEGMENT_DESCRIPTOR,
> +	    VM_INJECT_EXCEPTION, VM_LAPIC_IRQ, VM_LAPIC_LOCAL_IRQ,
> +	    VM_LAPIC_MSI, VM_IOAPIC_ASSERT_IRQ, VM_IOAPIC_DEASSERT_IRQ,
> +	    VM_IOAPIC_PULSE_IRQ, VM_IOAPIC_PINCOUNT, VM_ISA_ASSERT_IRQ,
> +	    VM_ISA_DEASSERT_IRQ, VM_ISA_PULSE_IRQ, VM_ISA_SET_IRQ_TRIGGER,
> +	    VM_SET_CAPABILITY, VM_GET_CAPABILITY, VM_BIND_PPTDEV,
> +	    VM_UNBIND_PPTDEV, VM_MAP_PPTDEV_MMIO, VM_PPTDEV_MSI,
> +	    VM_PPTDEV_MSIX, VM_INJECT_NMI, VM_STATS, VM_STAT_DESC,
> +	    VM_SET_X2APIC_STATE, VM_GET_X2APIC_STATE,
> +	    VM_GET_HPET_CAPABILITIES, VM_GET_GPA_PMAP, VM_GLA2GPA,
> +	    VM_ACTIVATE_CPU, VM_GET_CPUS, VM_SET_INTINFO, VM_GET_INTINFO,
> +	    VM_RTC_WRITE, VM_RTC_READ, VM_RTC_SETTIME, VM_RTC_GETTIME,
> +	    VM_RESTART_INSTRUCTION };
> +
> +	if (len == NULL) {
> +		cmds = malloc(sizeof(vm_ioctl_cmds));
> +		if (cmds == NULL)
> +			return (NULL);
> +		bcopy(vm_ioctl_cmds, cmds, sizeof(vm_ioctl_cmds));
> +		return (cmds);

Given you are returning a const array, why do you have to malloc a separate
copy vs just returning vm_ioctl_cmds[] directly?

It would seem that the interface for this could be that it always returns
vm_ioctl_cmds and sets *len if len is not NULL.

> Modified: head/usr.sbin/bhyve/bhyverun.c
> ==============================================================================
> --- head/usr.sbin/bhyve/bhyverun.c	Tue Feb 14 04:52:24 2017	(r313726)
> +++ head/usr.sbin/bhyve/bhyverun.c	Tue Feb 14 13:35:59 2017	(r313727)
> @@ -744,6 +759,21 @@ do_open(const char *vmname)
>  		exit(1);
>  	}
>  
> +#ifndef WITHOUT_CAPSICUM
> +	cap_rights_init(&rights, CAP_IOCTL, CAP_MMAP_RW);
> +	if (cap_rights_limit(vm_get_device_fd(ctx), &rights) == -1 &&
> +	    errno != ENOSYS)
> +		errx(EX_OSERR, "Unable to apply rights for sandbox");
> +	vm_get_ioctls(&ncmds);
> +	cmds = vm_get_ioctls(NULL);
> +	if (cmds == NULL)
> +		errx(EX_OSERR, "out of memory");
> +	if (cap_ioctls_limit(vm_get_device_fd(ctx), cmds, ncmds) == -1 &&
> +	    errno != ENOSYS)
> +		errx(EX_OSERR, "Unable to apply rights for sandbox");
> +	free((cap_ioctl_t *)cmds);
> +#endif
> + 

I wonder if instead of doing this in the executable and exposing
vm_get_ioctls, etc. if the API shouldn't really be something like
'vm_limit_rights(ctx)' and this code should be in that function in
libvmmapi?  It would be something like (assuming you drop
vm_get_ioctls() entirely and expose vm_ioctl_cmds[] as a static
global in the library):

int
vm_limit_rights(struct vm_ctx *ctx)
{
	...
	cap_rights_init(&rights, CAP_IOCTL, CAP_MMAP_RW);
	if (cap_rights_limit(ctx->fd, &rights) == -1) {
 		if (errno == ENOSYS)
			return (0);
		else
			return (-1);
	}
	/* Shouldn't get ENOSYS here if cap_rights_limit worked. */
	return (cap_ioctls_limit(ctx->fd, vm_ioctl_cmds, nitems(vm_ioctl_cmds));
}

You wouldn't need vm_get_device_fd() either in this API.

> Modified: head/usr.sbin/bhyve/consport.c
> ==============================================================================
> --- head/usr.sbin/bhyve/consport.c	Tue Feb 14 04:52:24 2017	(r313726)
> +++ head/usr.sbin/bhyve/consport.c	Tue Feb 14 13:35:59 2017	(r313727)
> @@ -123,6 +133,13 @@ console_handler(struct vmctx *ctx, int v
>  		return (-1);
>  
>  	if (!opened) {
> +#ifndef WITHOUT_CAPSICUM
> +	cap_rights_init(&rights, CAP_EVENT, CAP_IOCTL, CAP_READ, CAP_WRITE);
> +	if (cap_rights_limit(STDIN_FILENO, &rights) == -1 && errno != ENOSYS)
> +		errx(EX_OSERR, "Unable to apply rights for sandbox");
> +	if (cap_ioctls_limit(STDIN_FILENO, cmds, nitems(cmds)) == -1 && errno != ENOSYS)
> +		errx(EX_OSERR, "Unable to apply rights for sandbox");
> +#endif
>  		ttyopen();
>  		opened = 1;
>  	}

Indentation looks wrong?

-- 
John Baldwin