From eb1cd41d55d3b3be2be9d6542531f075dfd8fa52 Mon Sep 17 00:00:00 2001 From: reyk Date: Thu, 6 Apr 2017 18:07:13 +0000 Subject: [PATCH] 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@ --- usr.sbin/vmctl/main.c | 11 +++++----- usr.sbin/vmctl/vmctl.c | 14 +++++++------ usr.sbin/vmctl/vmctl.h | 5 +++-- usr.sbin/vmd/vmd.c | 46 +++++++++++++++++++++++++++++++++--------- usr.sbin/vmd/vmd.h | 7 ++++--- usr.sbin/vmd/vmm.c | 27 ++++++++++++++++--------- 6 files changed, 75 insertions(+), 35 deletions(-) diff --git a/usr.sbin/vmctl/main.c b/usr.sbin/vmctl/main.c index c50cdaceec0..da27fd6c913 100644 --- 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 @@ -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 038f3b11c92..39d1615d8f2 100644 --- 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 @@ -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 e47944f3b12..58d03d20bd1 100644 --- 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 @@ -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 77731c2462f..3bcfed30965 100644 --- 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 @@ -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 762be489c9e..3e51d964702 100644 --- 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 @@ -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 9802ab2ca91..f0ba6d9ac3e 100644 --- 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 @@ -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) -- 2.20.1