fix: Make PATCH /machine-config requests transactional

Ensure that modifications to the machine configuration are "all or
nothing", e.g. no modifications happen if any part of the update causes
an error (such as incorrect guest memory size). This fixes various
possible bugs where, say, the validation of an updated vcpu field
succeeds (meaning the vcpu field of the machine configuration is
updated), but then a subsequent validation of the mem_size_mib field
fails (say, because its 0), causing a error response to be returned to
the user, yet not reverting the update to the vcpu field (in maybe more
severe situations, it is possible to bypass the check that the balloon
device is not configured to have a balloon larger than the total guest
memory size, as mem_size_mib is updated before this check is being
done).

We fix this by using a Haskell-style approach to updating the
configuration: In the update method, construct a new VmConfig object and
return this; then only update the vm_config field on the VmResources
struct if all validations have passed with the new vm_config object.

Signed-off-by: Patrick Roy <[email protected]>

Author: roypat

CHANGELOG.md

@@ -36,6 +36,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   Now cpu-template-helper will print warnings if it encounters SVE registers
   during the conversion process. This is because cpu templates are limited
   to only modify registers less than 128 bits.
+- [#4414](https://github.com/firecracker-microvm/firecracker/pull/4360):
+  Made `PATCH` requests to the `/machine-config` endpoint transactional, meaning
+  Firecracker's configuration will be unchanged if the request returns an error.
+  This fixes a bug where a microVM with incompatible balloon and guest memory size
+  could be booted, due to the check for this condition happening after Firecracker's
+  configuration was updated.
 
 ## [v1.6.0]
 

src/vmm/src/resources.rs

@@ -242,12 +242,12 @@ impl VmResources {
 
     /// Updates the configuration of the microVM.
     pub fn update_vm_config(&mut self, update: &MachineConfigUpdate) -> Result<(), VmConfigError> {
-        self.vm_config.update(update)?;
+        let updated = self.vm_config.update(update)?;
 
         // The VM cannot have a memory size smaller than the target size
         // of the balloon device, if present.
         if self.balloon.get().is_some()
-            && self.vm_config.mem_size_mib
+            && updated.mem_size_mib
                 < self
                     .balloon
                     .get_config()
@@ -257,6 +257,8 @@ impl VmResources {
             return Err(VmConfigError::IncompatibleBalloonSize);
         }
 
+        self.vm_config = updated;
+
         Ok(())
     }
 

src/vmm/src/vmm_config/machine_config.rs

@@ -122,11 +122,12 @@ impl VmConfig {
         self.cpu_template = Some(CpuTemplateType::Custom(cpu_template));
     }
 
-    /// Updates `VmConfig` with `MachineConfigUpdate`.
-    /// Mapping for cpu tempalte update:
+    /// Updates [`VmConfig`] with [`MachineConfigUpdate`].
+    /// Mapping for cpu template update:
     /// StaticCpuTemplate::None -> None
-    /// StaticCpuTemplate::Other -> Some(CustomCpuTemplate::Static(Other))
-    pub fn update(&mut self, update: &MachineConfigUpdate) -> Result<(), VmConfigError> {
+    /// StaticCpuTemplate::Other -> Some(CustomCpuTemplate::Static(Other)),
+    /// Returns the updated `VmConfig` object.
+    pub fn update(&self, update: &MachineConfigUpdate) -> Result<VmConfig, VmConfigError> {
         let vcpu_count = update.vcpu_count.unwrap_or(self.vcpu_count);
 
         let smt = update.smt.unwrap_or(self.smt);
@@ -146,29 +147,25 @@ impl VmConfig {
             return Err(VmConfigError::InvalidVcpuCount);
         }
 
-        self.vcpu_count = vcpu_count;
-        self.smt = smt;
-
         let mem_size_mib = update.mem_size_mib.unwrap_or(self.mem_size_mib);
 
         if mem_size_mib == 0 {
             return Err(VmConfigError::InvalidMemorySize);
         }
 
-        self.mem_size_mib = mem_size_mib;
-
-        if let Some(cpu_template) = update.cpu_template {
-            self.cpu_template = match cpu_template {
-                StaticCpuTemplate::None => None,
-                other => Some(CpuTemplateType::Static(other)),
-            };
-        }
-
-        if let Some(track_dirty_pages) = update.track_dirty_pages {
-            self.track_dirty_pages = track_dirty_pages;
-        }
-
-        Ok(())
+        let cpu_template = match update.cpu_template {
+            None => self.cpu_template.clone(),
+            Some(StaticCpuTemplate::None) => None,
+            Some(other) => Some(CpuTemplateType::Static(other)),
+        };
+
+        Ok(VmConfig {
+            vcpu_count,
+            mem_size_mib,
+            smt,
+            cpu_template,
+            track_dirty_pages: update.track_dirty_pages.unwrap_or(self.track_dirty_pages),
+        })
     }
 }