Page MenuHomeFreeBSD

vmm: Add credential to cdev object
ClosedPublic

Authored by cyril_freebsdfoundation.org on Jul 12 2021, 7:58 PM.

Details

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.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

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
1018

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.

1020

I think EPERM is a more suitable error number here.

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

tests/sys/vmm/vmm_cred_jail.sh
40 ↗(On Diff #92199)

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

Check that vmm module is loaded before running tests.

sys/amd64/vmm/vmm_dev.c
1021

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.

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

This revision is now accepted and ready to land.Jul 21 2021, 2:30 PM
jhb added a subscriber: jhb.
jhb added inline comments.
sys/amd64/vmm/vmm_dev.c
185

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 edited the summary of this revision. (Show Details)

Simplify the loop in vmmdev_lookup.

This revision now requires review to proceed.Jul 30 2021, 6:21 PM

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

This revision was not accepted when it landed; it landed in state Needs Review.Aug 18 2021, 5:44 PM
This revision was automatically updated to reflect the committed changes.