On Sat, Oct 27, 2018 at 02:53:16PM -0700, Ori Bernstein wrote:
> On Fri, 26 Oct 2018 01:57:15 +0200, Reyk Floeter <r...@openbsd.org> wrote:
>
> > On Tue, Oct 23, 2018 at 10:21:08PM -0700, Ori Bernstein wrote:
> > > On Mon, 8 Oct 2018 07:59:15 -0700, Bob Beck <b...@openbsd.org> wrote:
> > >
> > > > works here and I like it. but probably for after unlock
> > > >
> > >
> > > It's after unlock -- pinging for OKs.
> > >
> >
> > Not yet. Please include the VM's uid in the claim, e.g.
> >
> > claim_vmid(const char *name, uid_t uid)
> >
> > It is not a strong protection, but it doesn't make sense that other
> > users can run a VM with the same name and get the claimed Id.
> >
> > Reyk
> >
>
> Updated.
>
> Ok?
>
Sorry for the delay...
Two minor things:
- I think config_purge() should clear env->vmd_known as it does with
vmd_vms and vmd_switches.
- The use of static functions (static uint32_t vm_claimid) is disputable
but somewhat uncommon in OpenBSD. But even static functions do need a
prototype ("All functions are prototyped somewhere." - style(9)).
Otherwise OK reyk@
> diff --git usr.sbin/vmd/config.c usr.sbin/vmd/config.c
> index a749e3595b5..31f46bf3c5b 100644
> --- usr.sbin/vmd/config.c
> +++ usr.sbin/vmd/config.c
> @@ -60,7 +60,10 @@ config_init(struct vmd *env)
> if (what & CONFIG_VMS) {
> if ((env->vmd_vms = calloc(1, sizeof(*env->vmd_vms))) == NULL)
> return (-1);
> + if ((env->vmd_known = calloc(1, sizeof(*env->vmd_known))) ==
> NULL)
> + return (-1);
> TAILQ_INIT(env->vmd_vms);
> + TAILQ_INIT(env->vmd_known);
> }
> if (what & CONFIG_SWITCHES) {
> if ((env->vmd_switches = calloc(1,
> diff --git usr.sbin/vmd/vmd.c usr.sbin/vmd/vmd.c
> index 8053b02620f..0683812f9f0 100644
> --- usr.sbin/vmd/vmd.c
> +++ usr.sbin/vmd/vmd.c
> @@ -1169,6 +1169,28 @@ vm_remove(struct vmd_vm *vm, const char *caller)
> free(vm);
> }
>
> +static uint32_t
> +vm_claimid(const char *name, int uid)
> +{
> + struct name2id *n2i = NULL;
> +
> + TAILQ_FOREACH(n2i, env->vmd_known, entry)
> + if (strcmp(n2i->name, name) == 0 && n2i->uid == uid)
> + return n2i->id;
> +
> + if (++env->vmd_nvm == 0)
> + fatalx("too many vms");
> + if ((n2i = calloc(1, sizeof(struct name2id))) == NULL)
> + fatalx("could not alloc vm name");
> + n2i->id = env->vmd_nvm;
> + n2i->uid = uid;
> + if (strlcpy(n2i->name, name, sizeof(n2i->name)) >= sizeof(n2i->name))
> + fatalx("overlong vm name");
> + TAILQ_INSERT_TAIL(env->vmd_known, n2i, entry);
> +
> + return n2i->id;
> +}
> +
> int
> vm_register(struct privsep *ps, struct vmop_create_params *vmc,
> struct vmd_vm **ret_vm, uint32_t id, uid_t uid)
> @@ -1300,11 +1322,8 @@ vm_register(struct privsep *ps, struct
> vmop_create_params *vmc,
> vm->vm_cdrom = -1;
> vm->vm_iev.ibuf.fd = -1;
>
> - if (++env->vmd_nvm == 0)
> - fatalx("too many vms");
> -
> /* Assign a new internal Id if not specified */
> - vm->vm_vmid = id == 0 ? env->vmd_nvm : id;
> + vm->vm_vmid = (id == 0) ? vm_claimid(vcp->vcp_name, uid) : id;
>
> log_debug("%s: registering vm %d", __func__, vm->vm_vmid);
> TAILQ_INSERT_TAIL(env->vmd_vms, vm, vm_entry);
> diff --git usr.sbin/vmd/vmd.h usr.sbin/vmd/vmd.h
> index 7ae4e4bd65e..047b5bb3e00 100644
> --- usr.sbin/vmd/vmd.h
> +++ usr.sbin/vmd/vmd.h
> @@ -284,6 +284,14 @@ struct vmd_user {
> };
> TAILQ_HEAD(userlist, vmd_user);
>
> +struct name2id {
> + char name[VMM_MAX_NAME_LEN];
> + int uid;
> + int32_t id;
> + TAILQ_ENTRY(name2id) entry;
> +};
> +TAILQ_HEAD(name2idlist, name2id);
> +
> struct address {
> struct sockaddr_storage ss;
> int prefixlen;
> @@ -308,6 +316,7 @@ struct vmd {
>
> uint32_t vmd_nvm;
> struct vmlist *vmd_vms;
> + struct name2idlist *vmd_known;
> uint32_t vmd_nswitches;
> struct switchlist *vmd_switches;
> struct userlist *vmd_users;
>
> --
> Ori Bernstein
