Do not expose vmm(4) VM IDs to the user, use vmd(8)'s IDs instead.

Each VM has two IDs: one from the kernel (vmm) and a different one
from userland (vmd).  The vmm ID is not consistent and incremented on
every boot during runtimg of the host system.  The vmd ID remains the
same during the lifetime of a configured VM, even after reboots.
Configured VMs will even get and keep their IDs when the configuration
is loaded.  This is more what users expect.

Pointed out and tested by otto@

OK deraadt@

diff --git a/usr.sbin/vmctl/main.c b/usr.sbin/vmctl/main.c
index c50cdac..da27fd6 100644 (file)
--- a/usr.sbin/vmctl/main.c
+++ b/usr.sbin/vmctl/main.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: main.c,v 1.24 2017/03/25 16:28:25 reyk Exp $  */
+/*     $OpenBSD: main.c,v 1.25 2017/04/06 18:07:13 reyk Exp $  */
 
 /*
  * Copyright (c) 2015 Reyk Floeter <reyk@openbsd.org>
@@ -194,8 +194,8 @@ vmmaction(struct parse_result *res)
 
        switch (res->action) {
        case CMD_START:
-               ret = vm_start(res->name, res->size, res->nifs, res->nets,
-                   res->ndisks, res->disks, res->path);
+               ret = vm_start(res->id, res->name, res->size, res->nifs,
+                   res->nets, res->ndisks, res->disks, res->path);
                if (ret) {
                        errno = ret;
                        err(1, "start VM operation failed");
@@ -533,8 +533,9 @@ ctl_start(struct parse_result *res, int argc, char *argv[])
        if (argc < 2)
                ctl_usage(res->ctl);
 
-       if ((res->name = strdup(argv[1])) == NULL)
-               errx(1, "strdup");
+       if (parse_vmid(res, argv[1]) == -1)
+               errx(1, "invalid id: %s", argv[1]);
+
        argc--;
        argv++;
 

diff --git a/usr.sbin/vmctl/vmctl.c b/usr.sbin/vmctl/vmctl.c
index 038f3b1..39d1615 100644 (file)
--- a/usr.sbin/vmctl/vmctl.c
+++ b/usr.sbin/vmctl/vmctl.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: vmctl.c,v 1.28 2017/03/30 03:39:35 claudio Exp $      */
+/*     $OpenBSD: vmctl.c,v 1.29 2017/04/06 18:07:13 reyk Exp $ */
 
 /*
  * Copyright (c) 2014 Mike Larkin <mlarkin@openbsd.org>
@@ -52,6 +52,7 @@ int info_console;
  * Request vmd to start the VM defined by the supplied parameters
  *
  * Parameters:
+ *  start_id: optional ID of the VM
  *  name: optional name of the VM
  *  memsize: memory size (MB) of the VM to create
  *  nnics: number of vionet network interfaces to create
@@ -65,8 +66,8 @@ int info_console;
  *  ENOMEM if a memory allocation failure occurred.
  */
 int
-vm_start(const char *name, int memsize, int nnics, char **nics,
-    int ndisks, char **disks, char *kernel)
+vm_start(uint32_t start_id, const char *name, int memsize, int nnics,
+    char **nics, int ndisks, char **disks, char *kernel)
 {
        struct vmop_create_params *vmc;
        struct vm_create_params *vcp;
@@ -117,6 +118,7 @@ vm_start(const char *name, int memsize, int nnics, char **nics,
        vcp->vcp_ncpus = 1;
        vcp->vcp_ndisks = ndisks;
        vcp->vcp_nnics = nnics;
+       vcp->vcp_id = start_id;
 
        for (i = 0 ; i < ndisks; i++)
                strlcpy(vcp->vcp_disks[i], disks[i], VMM_MAX_PATH_DISK);
@@ -410,7 +412,7 @@ print_vm_info(struct vmop_info_result *list, size_t ct)
                        (void)fmt_scaled(vir->vir_memory_size * 1024 * 1024,
                            maxmem);
 
-                       if (vir->vir_id != 0) {
+                       if (vir->vir_creator_pid != 0 && vir->vir_id != 0) {
                                if (*vmi->vir_ttyname == '\0')
                                        tty = "-";
                                /* get tty - skip /dev/ path */
@@ -427,8 +429,8 @@ print_vm_info(struct vmop_info_result *list, size_t ct)
                                    tty, user, vir->vir_name);
                        } else {
                                /* disabled vm */
-                               printf("%5s %5s %5zd %7s %7s %7s %12s %s\n",
-                                   "-", "-",
+                               printf("%5u %5s %5zd %7s %7s %7s %12s %s\n",
+                                   vir->vir_id, "-",
                                    vir->vir_ncpus, maxmem, curmem,
                                    "-", user, vir->vir_name);
                        }

diff --git a/usr.sbin/vmctl/vmctl.h b/usr.sbin/vmctl/vmctl.h
index e47944f..58d03d2 100644 (file)
--- a/usr.sbin/vmctl/vmctl.h
+++ b/usr.sbin/vmctl/vmctl.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: vmctl.h,v 1.13 2017/03/01 21:22:57 reyk Exp $ */
+/*     $OpenBSD: vmctl.h,v 1.14 2017/04/06 18:07:13 reyk Exp $ */
 
 /*
  * Copyright (c) 2015 Reyk Floeter <reyk@openbsd.org>
@@ -77,7 +77,8 @@ __dead void
 
 /* vmctl.c */
 int     create_imagefile(const char *, long);
-int     vm_start(const char *, int, int, char **, int, char **, char *);
+int     vm_start(uint32_t, const char *, int, int, char **, int,
+           char **, char *);
 int     vm_start_complete(struct imsg *, int *, int);
 void    terminate_vm(uint32_t, const char *);
 int     terminate_vm_complete(struct imsg *, int *);

diff --git a/usr.sbin/vmd/vmd.c b/usr.sbin/vmd/vmd.c
index 77731c2..3bcfed3 100644 (file)
--- a/usr.sbin/vmd/vmd.c
+++ b/usr.sbin/vmd/vmd.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: vmd.c,v 1.55 2017/03/15 19:54:52 reyk Exp $   */
+/*     $OpenBSD: vmd.c,v 1.56 2017/04/06 18:07:13 reyk Exp $   */
 
 /*
  * Copyright (c) 2015 Reyk Floeter <reyk@openbsd.org>
@@ -115,9 +115,9 @@ vmd_dispatch_control(int fd, struct privsep_proc *p, struct imsg *imsg)
                                cmd = IMSG_VMDOP_TERMINATE_VM_RESPONSE;
                                break;
                        }
-                       id = vm->vm_params.vmc_params.vcp_id;
+                       id = vm->vm_vmid;
                } else
-                       vm = vm_getbyid(id);
+                       vm = vm_getbyvmid(id);
                if (vm_checkperm(vm, vid.vid_uid) != 0) {
                        res = EPERM;
                        cmd = IMSG_VMDOP_TERMINATE_VM_RESPONSE;
@@ -233,13 +233,13 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struct imsg *imsg)
                }
 
                log_info("%s: started vm %d successfully, tty %s",
-                   vcp->vcp_name, vcp->vcp_id, vm->vm_ttyname);
+                   vcp->vcp_name, vm->vm_vmid, vm->vm_ttyname);
                break;
        case IMSG_VMDOP_TERMINATE_VM_RESPONSE:
                IMSG_SIZE_CHECK(imsg, &vmr);
                memcpy(&vmr, imsg->data, sizeof(vmr));
                proc_forward_imsg(ps, imsg, PROC_CONTROL, -1);
-               if ((vm = vm_getbyid(vmr.vmr_id)) == NULL)
+               if ((vm = vm_getbyvmid(vmr.vmr_id)) == NULL)
                        break;
                if (vmr.vmr_result == 0) {
                        /* Mark VM as shutting down */
@@ -249,7 +249,7 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struct imsg *imsg)
        case IMSG_VMDOP_TERMINATE_VM_EVENT:
                IMSG_SIZE_CHECK(imsg, &vmr);
                memcpy(&vmr, imsg->data, sizeof(vmr));
-               if ((vm = vm_getbyid(vmr.vmr_id)) == NULL)
+               if ((vm = vm_getbyvmid(vmr.vmr_id)) == NULL)
                        break;
                if (vmr.vmr_result == 0) {
                        if (vm->vm_from_config)
@@ -265,7 +265,7 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struct imsg *imsg)
        case IMSG_VMDOP_GET_INFO_VM_DATA:
                IMSG_SIZE_CHECK(imsg, &vir);
                memcpy(&vir, imsg->data, sizeof(vir));
-               if ((vm = vm_getbyid(vir.vir_info.vir_id)) != NULL) {
+               if ((vm = vm_getbyvmid(vir.vir_info.vir_id)) != NULL) {
                        memset(vir.vir_ttyname, 0, sizeof(vir.vir_ttyname));
                        if (vm->vm_ttyname != NULL)
                                strlcpy(vir.vir_ttyname, vm->vm_ttyname,
@@ -295,7 +295,7 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struct imsg *imsg)
                TAILQ_FOREACH(vm, env->vmd_vms, vm_entry) {
                        if (!vm->vm_running) {
                                memset(&vir, 0, sizeof(vir));
-                               vir.vir_info.vir_id = 0;
+                               vir.vir_info.vir_id = vm->vm_vmid;
                                strlcpy(vir.vir_info.vir_name,
                                    vm->vm_params.vmc_params.vcp_name,
                                    VMM_MAX_NAME_LEN);
@@ -656,6 +656,8 @@ vm_getbyvmid(uint32_t vmid)
 {
        struct vmd_vm   *vm;
 
+       if (vmid == 0)
+               return (NULL);
        TAILQ_FOREACH(vm, env->vmd_vms, vm_entry) {
                if (vm->vm_vmid == vmid)
                        return (vm);
@@ -669,6 +671,8 @@ vm_getbyid(uint32_t id)
 {
        struct vmd_vm   *vm;
 
+       if (id == 0)
+               return (NULL);
        TAILQ_FOREACH(vm, env->vmd_vms, vm_entry) {
                if (vm->vm_params.vmc_params.vcp_id == id)
                        return (vm);
@@ -677,6 +681,26 @@ vm_getbyid(uint32_t id)
        return (NULL);
 }
 
+uint32_t
+vm_id2vmid(uint32_t id, struct vmd_vm *vm)
+{
+       if (vm == NULL && (vm = vm_getbyid(id)) == NULL)
+               return (0);
+       dprintf("%s: vmm id %u is vmid %u", __func__,
+           id, vm->vm_vmid);
+       return (vm->vm_vmid);
+}
+
+uint32_t
+vm_vmid2id(uint32_t vmid, struct vmd_vm *vm)
+{
+       if (vm == NULL && (vm = vm_getbyvmid(vmid)) == NULL)
+               return (0);
+       dprintf("%s: vmid %u is vmm id %u", __func__,
+           vmid, vm->vm_params.vmc_params.vcp_id);
+       return (vm->vm_params.vmc_params.vcp_id);
+}
+
 struct vmd_vm *
 vm_getbyname(const char *name)
 {
@@ -770,7 +794,8 @@ vm_register(struct privsep *ps, struct vmop_create_params *vmc,
        errno = 0;
        *ret_vm = NULL;
 
-       if ((vm = vm_getbyname(vcp->vcp_name)) != NULL) {
+       if ((vm = vm_getbyname(vcp->vcp_name)) != NULL ||
+           (vm = vm_getbyvmid(vcp->vcp_id)) != NULL) {
                if (vm_checkperm(vm, uid) != 0 || vmc->vmc_flags != 0) {
                        errno = EPERM;
                        goto fail;
@@ -808,6 +833,9 @@ vm_register(struct privsep *ps, struct vmop_create_params *vmc,
        } else if (strlen(vcp->vcp_kernel) == 0 && vcp->vcp_ndisks == 0) {
                log_warnx("no kernel or disk specified");
                goto fail;
+       } else if (strlen(vcp->vcp_name) == 0) {
+               log_warnx("invalid VM name");
+               goto fail;
        }
 
        if ((vm = calloc(1, sizeof(*vm))) == NULL)

diff --git a/usr.sbin/vmd/vmd.h b/usr.sbin/vmd/vmd.h
index 762be48..3e51d96 100644 (file)
--- a/usr.sbin/vmd/vmd.h
+++ b/usr.sbin/vmd/vmd.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: vmd.h,v 1.49 2017/03/25 16:28:25 reyk Exp $   */
+/*     $OpenBSD: vmd.h,v 1.50 2017/04/06 18:07:13 reyk Exp $   */
 
 /*
  * Copyright (c) 2015 Mike Larkin <mlarkin@openbsd.org>
@@ -159,7 +159,6 @@ TAILQ_HEAD(switchlist, vmd_switch);
 struct vmd_vm {
        struct vmop_create_params vm_params;
        pid_t                    vm_pid;
-       /* Userspace ID of VM. The user never sees this */
        uint32_t                 vm_vmid;
        int                      vm_kernel;
        int                      vm_disks[VMM_MAX_DISKS_PER_VM];
@@ -201,8 +200,10 @@ struct vmd {
 
 /* vmd.c */
 void    vmd_reload(unsigned int, const char *);
-struct vmd_vm *vm_getbyvmid(uint32_t);
 struct vmd_vm *vm_getbyid(uint32_t);
+struct vmd_vm *vm_getbyvmid(uint32_t);
+uint32_t vm_id2vmid(uint32_t, struct vmd_vm *);
+uint32_t vm_vmid2id(uint32_t, struct vmd_vm *);
 struct vmd_vm *vm_getbyname(const char *);
 struct vmd_vm *vm_getbypid(pid_t);
 void    vm_stop(struct vmd_vm *, int);

diff --git a/usr.sbin/vmd/vmm.c b/usr.sbin/vmd/vmm.c
index 9802ab2..f0ba6d9 100644 (file)
--- a/usr.sbin/vmd/vmm.c
+++ b/usr.sbin/vmd/vmm.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: vmm.c,v 1.67 2017/03/15 18:06:18 reyk Exp $   */
+/*     $OpenBSD: vmm.c,v 1.68 2017/04/06 18:07:13 reyk Exp $   */
 
 /*
  * Copyright (c) 2015 Mike Larkin <mlarkin@openbsd.org>
@@ -103,7 +103,7 @@ vmm_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg)
 {
        struct privsep          *ps = p->p_ps;
        int                      res = 0, cmd = 0, verbose;
-       struct vmd_vm           *vm;
+       struct vmd_vm           *vm = NULL;
        struct vm_terminate_params vtp;
        struct vmop_result       vmr;
        uint32_t                 id = 0;
@@ -133,6 +133,9 @@ vmm_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg)
                break;
        case IMSG_VMDOP_START_VM_END:
                res = vmm_start_vm(imsg, &id);
+               /* Check if the ID can be mapped correctly */
+               if ((id = vm_id2vmid(id, NULL)) == 0)
+                       res = ENOENT;
                cmd = IMSG_VMDOP_START_VM_RESPONSE;
                break;
        case IMSG_VMDOP_TERMINATE_VM_REQUEST:
@@ -140,7 +143,9 @@ vmm_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg)
                memcpy(&vtp, imsg->data, sizeof(vtp));
                id = vtp.vtp_vm_id;
 
-               if ((vm = vm_getbyid(id)) != NULL &&
+               if (id == 0) {
+                       res = ENOENT;
+               } else if ((vm = vm_getbyvmid(id)) != NULL &&
                    vm->vm_shutdown == 0) {
                        log_debug("%s: sending shutdown request to vm %d",
                            __func__, id);
@@ -160,6 +165,7 @@ vmm_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg)
                                res = 0;
                } else {
                        /* Terminate VMs that are unknown or shutting down */
+                       vtp.vtp_vm_id = vm_vmid2id(vm->vm_vmid, vm);
                        res = terminate_vm(&vtp);
                        vm_remove(vm);
                }
@@ -206,6 +212,8 @@ vmm_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg)
                        if ((vm = vm_getbyvmid(imsg->hdr.peerid)) != NULL)
                                vm_remove(vm);
                }
+               if (id == 0)
+                       id = imsg->hdr.peerid;
        case IMSG_VMDOP_TERMINATE_VM_RESPONSE:
                memset(&vmr, 0, sizeof(vmr));
                vmr.vmr_result = res;
@@ -265,16 +273,16 @@ vmm_sighdlr(int sig, short event, void *arg)
                                if (terminate_vm(&vtp) == 0) {
                                        memset(&vmr, 0, sizeof(vmr));
                                        vmr.vmr_result = ret;
-                                       vmr.vmr_id = vmid;
+                                       vmr.vmr_id = vm_id2vmid(vmid, vm);
                                        if (proc_compose_imsg(ps, PROC_PARENT,
                                            -1, IMSG_VMDOP_TERMINATE_VM_EVENT,
                                            0, -1, &vmr, sizeof(vmr)) == -1)
                                                log_warnx("could not signal "
                                                    "termination of VM %u to "
-                                                   "parent", vmid);
+                                                   "parent", vm->vm_vmid);
                                } else
                                        log_warnx("could not terminate VM %u",
-                                           vmid);
+                                           vm->vm_vmid);
 
                                vm_remove(vm);
                        } else
@@ -298,7 +306,7 @@ vmm_shutdown(void)
        struct vmd_vm *vm, *vm_next;
 
        TAILQ_FOREACH_SAFE(vm, env->vmd_vms, vm_entry, vm_next) {
-               vtp.vtp_vm_id = vm->vm_params.vmc_params.vcp_id;
+               vtp.vtp_vm_id = vm_vmid2id(vm->vm_vmid, vm);
 
                /* XXX suspend or request graceful shutdown */
                (void)terminate_vm(&vtp);
@@ -370,11 +378,9 @@ vmm_dispatch_vm(int fd, short event, void *arg)
                if (n == 0)
                        break;
 
-#if DEBUG > 1
-               log_debug("%s: got imsg %d from %s",
+               dprintf("%s: got imsg %d from %s",
                    __func__, imsg.hdr.type,
                    vm->vm_params.vmc_params.vcp_name);
-#endif
 
                switch (imsg.hdr.type) {
                case IMSG_VMDOP_VM_SHUTDOWN:
@@ -618,6 +624,7 @@ get_info_vm(struct privsep *ps, struct imsg *imsg, int terminate)
                        continue;
                }
                memcpy(&vir.vir_info, &info[i], sizeof(vir.vir_info));
+               vir.vir_info.vir_id = vm_id2vmid(info[i].vir_id, NULL);
                if (proc_compose_imsg(ps, PROC_PARENT, -1,
                    IMSG_VMDOP_GET_INFO_VM_DATA, imsg->hdr.peerid, -1,
                    &vir, sizeof(vir)) == -1)