search

cyberus-technology / virtualbox-kvm

KVM Backend for VirtualBox. With our current development model, we cannot easily accept pull requests here. If you'd like to contribute, feel free to reach out to us, we are happy to find a solution.
GNU General Public License v3.0
947 stars 117 forks source link

Possible patch to allow hardened build to boot VMs #30

Open laniakea64 opened 4 months ago

laniakea64 commented 4 months ago

First, thank you very much for this KVM backend for VirtualBox! It works well for me, no more micro-managing kernel versions :tada:

Saw that building without --disable-hardening is now an option with the latest patch set, so tried it out in case a hardened build might be more secure. I was able to get the hardened build to run, but it would not boot any VM: the hardened VirtualBoxVM binary expects to be SUID root, but with KVM backend it is not SUID and doesn't need root at all.

The VM host and build machine are both running Xubuntu 22.04 with kernel 6.8.0-76060800daily20240311-generic from the System76 Driver PPA. Non-hardened build works without issue on this system (VirtualBox 7.0.14a with KVM backend patches from 9d202d47d432a176d21ed32904cbe369114725b7). A guest OS is not required to reproduce the issue with hardened builds: so far I have been testing this with new, blank VMs with no guest OS.

To enable the hardened build to boot VMs, this patch seems to do the job -

diff --git a/src/VBox/HostDrivers/Support/SUPLibInternal.h b/src/VBox/HostDrivers/Support/SUPLibInternal.h
index 4c788fac..caef2de4 100644
--- a/src/VBox/HostDrivers/Support/SUPLibInternal.h
+++ b/src/VBox/HostDrivers/Support/SUPLibInternal.h
@@ -77,7 +77,7 @@
 /** @def SUP_HARDENED_SUID
  * Whether we're employing set-user-ID-on-execute in the hardening.
  */
-#if (!defined(RT_OS_OS2) && !defined(RT_OS_WINDOWS) && !defined(RT_OS_L4)) || defined(DOXYGEN_RUNNING)
+#if (!defined(VBOX_WITH_KVM) && !defined(RT_OS_OS2) && !defined(RT_OS_WINDOWS) && !defined(RT_OS_L4)) || defined(DOXYGEN_RUNNING)
 # define SUP_HARDENED_SUID
 #else
 # undef  SUP_HARDENED_SUID
diff --git a/src/VBox/HostDrivers/Support/SUPR3HardenedMain.cpp b/src/VBox/HostDrivers/Support/SUPR3HardenedMain.cpp
index 25c6b4c5..879d5ba5 100644
--- a/src/VBox/HostDrivers/Support/SUPR3HardenedMain.cpp
+++ b/src/VBox/HostDrivers/Support/SUPR3HardenedMain.cpp
@@ -470,7 +470,7 @@
 *   Defined Constants And Macros                                                                                                 *
 *********************************************************************************************************************************/
 /* This mess is temporary after eliminating a define duplicated in SUPLibInternal.h. */
-#if !defined(RT_OS_OS2) && !defined(RT_OS_WINDOWS) && !defined(RT_OS_L4)
+#if !defined(VBOX_WITH_KVM) && !defined(RT_OS_OS2) && !defined(RT_OS_WINDOWS) && !defined(RT_OS_L4)
 # ifndef SUP_HARDENED_SUID
 #  error "SUP_HARDENED_SUID is NOT defined?!?"
 # endif

Is this a reasonable fix that would be safe to use in production? If so, could you please add it to the patches in this repo? Or is there a better way to get this working?

Thanks!

tpressure commented 4 months ago

Not having to set SUID certainly makes sense as we do not have any kernel module to load. I will look into this.

ping @eugenesan @dionorgua