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@

3 files changed, 58 insertions, 22 deletions

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 <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 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 <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 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 <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)