Page MenuHomeFreeBSD

bhyve: don't crash when guest writes TPM int_enable register
ClosedPublic

Authored by rosenfeld_grumpf.hope-2000.org on Sep 6 2024, 1:03 PM.
Tags
None
Referenced Files
F102246030: D46562.diff
Sat, Nov 9, 12:21 PM
Unknown Object (File)
Wed, Nov 6, 8:24 AM
Unknown Object (File)
Sun, Nov 3, 2:11 AM
Unknown Object (File)
Tue, Oct 22, 5:39 PM
Unknown Object (File)
Tue, Oct 22, 3:18 PM
Unknown Object (File)
Sun, Oct 20, 6:18 PM
Unknown Object (File)
Fri, Oct 18, 9:16 AM
Unknown Object (File)
Thu, Oct 17, 8:39 AM

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 59339
Build 56226: arc lint + arc unit

Event Timeline

Even FreeBSD's own TPM driver writes the INT_ENABLE register during attach, writing 0 to make sure interrupts are off. bhyve really shouldn't crash the VM when that happens.

corvink added inline comments.
usr.sbin/bhyve/tpm_intf_crb.c
341

I slightly prefer adding a warning at least. Ignoring all writes will be unexpected if someone ever tries to get a TPM driver using interrupts working in a bhyve vm.
We could also accept 0 here and exit on other values.

usr.sbin/bhyve/tpm_intf_crb.c
341

I don't mind adding a warning, but I really don't see much value in it, neither for a user nor from a developers perspective. The best case scenario is that it'll end up in some logfile and is going to be ignored, worst case the log fills up with pointless messages.

While it's probably trivial to accept only 0 writes and exit on other values, I'm not convinced it's a sensible idea to rig the hypervisor with booby-traps that will just kill a VM on a unsupported register access. Real hardware usually doesn't halt or catch fire when that happens, so why should the hypervisor do this? I think it makes more sense to carry on and not work against the guest OS. On the other hand, I suppose that's probably what most device emulations in bhyve do already, so maybe it's a feature and not a bug.

Of course, if you insist, I'll happily do either or both. Please let me know.

corvink added inline comments.
usr.sbin/bhyve/tpm_intf_crb.c
341

I don't have a strong preference, so just keep it as is.

This revision is now accepted and ready to land.Sep 18 2024, 12:40 PM

Do I need a more elaborate commit message here, too?

Also, and in general, where do testing notes go, assuming they are needed?

Do I need a more elaborate commit message here, too?

Also, and in general, where do testing notes go, assuming they are needed?

You should always explain in the commit message why you want to change something. IMO, testing notes should be added to the commit message too. The commit message will never get lost because it's part of the project itself.

This revision now requires review to proceed.Sep 23 2024, 2:06 PM
This revision was not accepted when it landed; it landed in state Needs Review.Tue, Oct 22, 12:07 PM
This revision was automatically updated to reflect the committed changes.