From owner-svn-src-all@freebsd.org Tue Feb 14 16:28:58 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id E8E03CDF33C; Tue, 14 Feb 2017 16:28:58 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from mail.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id C093B1B52; Tue, 14 Feb 2017 16:28:58 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from ralph.baldwin.cx (c-73-231-226-104.hsd1.ca.comcast.net [73.231.226.104]) by mail.baldwin.cx (Postfix) with ESMTPSA id 96AE410A7DB; Tue, 14 Feb 2017 11:28:57 -0500 (EST) From: John Baldwin To: Bartek Rutkowski 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 Date: Tue, 14 Feb 2017 07:14:15 -0800 Message-ID: <2453150.oLT9KULcP9@ralph.baldwin.cx> User-Agent: KMail/4.14.10 (FreeBSD/11.0-STABLE; KDE/4.14.10; amd64; ; ) In-Reply-To: <201702141335.v1EDZxr8057062@repo.freebsd.org> References: <201702141335.v1EDZxr8057062@repo.freebsd.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.4.3 (mail.baldwin.cx); Tue, 14 Feb 2017 11:28:57 -0500 (EST) X-Virus-Scanned: clamav-milter 0.99.2 at mail.baldwin.cx X-Virus-Status: Clean X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 14 Feb 2017 16:28:59 -0000 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 > 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