Date: Tue, 14 Feb 2017 07:14:15 -0800 From: John Baldwin <jhb@freebsd.org> To: Bartek Rutkowski <robak@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r313727 - in head: lib/libvmmapi usr.sbin/bhyve Message-ID: <2453150.oLT9KULcP9@ralph.baldwin.cx> In-Reply-To: <201702141335.v1EDZxr8057062@repo.freebsd.org> References: <201702141335.v1EDZxr8057062@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?2453150.oLT9KULcP9>