vmm: Add credential to cdev object

[cyril_freebsdfoundation.org]

Summary

Add a credential to the cdev object in sysctl_vmm_create, then check that we have the correct credentials in sysctl_vmm_destroy. This prevents a process in one jail from opening or destroying the /dev/vmm file corresponding to a vm in another jail.

[markj]

Could you add a couple of short atf-sh tests for this using bhyvectl --create and --destroy? I suspect they should live in tests/sys/vmm.

sys/amd64/vmm/vmm_dev.c

1028

I think it's probably ok to borrow the cdev's credential reference like this but it'd be a bit nicer to maintain a separate reference (to the same credential) in the VM softc as well. The overhead of doing that is negligible, and it might avoid some tricky bugs that would arise if something tries to access the VM's credential after the cdev is destroyed.

For example, this function sets sc->cdev = NULL before triggering asynchronous destruction of the VM (which may take a long time) by calling destroy_dev_sched_cb(). If someone naively modifies vmmdev_destroy() such that it tries to dereference sc->cdev->si_cred (perhaps through several layers of indirection), we'll get a null pointer dereference. Keeping a separate credential reference avoids coupling between VMM and devfs object lifecycles, so I'd tend to prefer that approach even though it requires a bit more code.

1030

I think EPERM is a more suitable error number here.

[cyril_freebsdfoundation.org]

Add a credential to the softc struct as well. Also add some basic tests.

[cyril_freebsdfoundation.org]

Add copyright notices to test files.

[markj]

tests/sys/vmm/vmm_cred_jail.sh

41

These tests should first check whether the vmm kernel module is loaded. Something like

if ! kldstat -qn vmm; then
    atf_skip "vmm is not loaded"
fi

[cyril_freebsdfoundation.org]

Check that vmm module is loaded before running tests.

[markj]

sys/amd64/vmm/vmm_dev.c

1032

I think that we should move this check into vmmdev_lookup() and have it return NULL if the check fails. Otherwise every caller of vmmdev_lookup() will have to be careful to check the credential.

[cyril_freebsdfoundation.org]

Move the credential check to vmmdev_lookup. Note that this means the error code will be EINVAL in sysctl_vmm_destroy, rather than EPERM.

[jhb]

sys/amd64/vmm/vmm_dev.c

192

This is ok, but another way you could have handled this would be:

SLIST_FOREACH(...) {
   if (strcmp(...) == 0)
       break;
}
if (sc == NULL)
  return (NULL);

if (!cr_cansee(curthread->td_ucred, sc->ucred))
  return (NULL);

return (sc);

Layering the checks like that would make the new code a bit smaller (you are just adding a new check, not changing the existing loop). It also avoids deep nesting if we wanted to add additional checks in the future.

[cyril_freebsdfoundation.org]

Simplify the loop in vmmdev_lookup.

[cyril_freebsdfoundation.org]

Made a mistake in the last diff - accidentally omitted the test files.