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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
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. | |
| 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 | |
| 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. | |
Comment Actions
Move the credential check to vmmdev_lookup. Note that this means the error code will be EINVAL in sysctl_vmm_destroy, rather than EPERM.
| 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. | |