pvmfw: sanitize bootargs
during the fdt sanitization, the bootargs string is parsed and then
only the allowed args are filtered. crashkernel= and console= are
conditionaly filtered; they are included only when the corresponding
debug policy is turned on.
Bug: 275306568
Test: inject foo=bar to bootargs and see if it's filtered out
Change-Id: I617bbf888575674f9544fca9b2ea11a7795a6e95
diff --git a/pvmfw/Android.bp b/pvmfw/Android.bp
index 41abb91..0571c36 100644
--- a/pvmfw/Android.bp
+++ b/pvmfw/Android.bp
@@ -41,6 +41,20 @@
cmd: "touch $(out)",
}
+rust_test {
+ name: "libpvmfw.bootargs.test",
+ host_supported: true,
+ // For now, only bootargs.rs is written to be conditionally compiled with std.
+ srcs: ["src/bootargs.rs"],
+ test_suites: ["general-tests"],
+ test_options: {
+ unit_test: true,
+ },
+ rustlibs: [
+ "libzeroize",
+ ],
+}
+
cc_binary {
name: "pvmfw",
defaults: ["vmbase_elf_defaults"],
diff --git a/pvmfw/src/bootargs.rs b/pvmfw/src/bootargs.rs
new file mode 100644
index 0000000..f9c8278
--- /dev/null
+++ b/pvmfw/src/bootargs.rs
@@ -0,0 +1,211 @@
+// Copyright 2023, The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! Routines for parsing bootargs
+
+#[cfg(not(test))]
+use alloc::format;
+#[cfg(not(test))]
+use alloc::string::String;
+use core::ffi::CStr;
+
+/// A single boot argument ex: "panic", "init=", or "foo=1,2,3".
+pub struct BootArg<'a> {
+ arg: &'a str,
+ equal_sign: Option<usize>,
+}
+
+impl AsRef<str> for BootArg<'_> {
+ fn as_ref(&self) -> &str {
+ self.arg
+ }
+}
+
+impl BootArg<'_> {
+ /// Name of the boot argument
+ pub fn name(&self) -> &str {
+ if let Some(n) = self.equal_sign {
+ &self.arg[..n]
+ } else {
+ self.arg
+ }
+ }
+
+ /// Optional value of the boot aragument. This includes the '=' prefix.
+ pub fn value(&self) -> Option<&str> {
+ Some(&self.arg[self.equal_sign?..])
+ }
+}
+
+/// Iterator that iteratos over bootargs
+pub struct BootArgsIterator<'a> {
+ arg: &'a str,
+}
+
+impl<'a> BootArgsIterator<'a> {
+ /// Creates a new iterator from the raw boot args. The input has to be encoded in ASCII
+ pub fn new(bootargs: &'a CStr) -> Result<Self, String> {
+ let arg = bootargs.to_str().map_err(|e| format!("{e}"))?;
+ if !arg.is_ascii() {
+ return Err(format!("{arg:?} is not ASCII"));
+ }
+
+ Ok(Self { arg })
+ }
+
+ // Finds the end of a value in the given string `s`, and returns the index of the end. A value
+ // can have spaces if quoted. The quote character can't be escaped.
+ fn find_value_end(s: &str) -> usize {
+ let mut in_quote = false;
+ for (i, c) in s.char_indices() {
+ if c == '"' {
+ in_quote = !in_quote;
+ } else if c.is_whitespace() && !in_quote {
+ return i;
+ }
+ }
+ s.len()
+ }
+}
+
+impl<'a> Iterator for BootArgsIterator<'a> {
+ type Item = BootArg<'a>;
+
+ fn next(&mut self) -> Option<Self::Item> {
+ // Skip spaces to find the start of a name. If there's nothing left, that's the end of the
+ // iterator.
+ let arg = self.arg.trim_start();
+ self.arg = arg; // advance before returning
+ if arg.is_empty() {
+ return None;
+ }
+ // Name ends with either whitespace or =. If it ends with =, the value comes immediately
+ // after.
+ let name_end = arg.find(|c: char| c.is_whitespace() || c == '=').unwrap_or(arg.len());
+ let (arg, equal_sign) = match arg.chars().nth(name_end) {
+ Some(c) if c == '=' => {
+ let value_end = name_end + Self::find_value_end(&arg[name_end..]);
+ (&arg[..value_end], Some(name_end))
+ }
+ _ => (&arg[..name_end], None),
+ };
+ self.arg = &self.arg[arg.len()..]; // advance before returning
+ Some(BootArg { arg, equal_sign })
+ }
+}
+
+#[cfg(test)]
+#[allow(dead_code)]
+mod helpers;
+
+#[cfg(test)]
+mod tests {
+
+ use super::*;
+ use crate::cstr;
+
+ fn check(raw: &CStr, expected: Result<&[(&str, Option<&str>)], ()>) {
+ let actual = BootArgsIterator::new(raw);
+ assert_eq!(actual.is_err(), expected.is_err(), "Unexpected result with {raw:?}");
+ if actual.is_err() {
+ return;
+ }
+ let mut actual = actual.unwrap();
+
+ for (name, value) in expected.unwrap() {
+ let actual = actual.next();
+ assert!(actual.is_some(), "Expected ({}, {:?}) from {raw:?}", name, value);
+ let actual = actual.unwrap();
+ assert_eq!(name, &actual.name(), "Unexpected name from {raw:?}");
+ assert_eq!(value, &actual.value(), "Unexpected value from {raw:?}");
+ }
+ let remaining = actual.next();
+ assert!(
+ remaining.is_none(),
+ "Unexpected extra item from {raw:?}. Got ({}, {:?})",
+ remaining.as_ref().unwrap().name(),
+ remaining.as_ref().unwrap().value()
+ );
+ }
+
+ #[test]
+ fn empty() {
+ check(cstr!(""), Ok(&[]));
+ check(cstr!(" "), Ok(&[]));
+ check(cstr!(" \n "), Ok(&[]));
+ }
+
+ #[test]
+ fn single() {
+ check(cstr!("foo"), Ok(&[("foo", None)]));
+ check(cstr!(" foo"), Ok(&[("foo", None)]));
+ check(cstr!("foo "), Ok(&[("foo", None)]));
+ check(cstr!(" foo "), Ok(&[("foo", None)]));
+ }
+
+ #[test]
+ fn single_with_value() {
+ check(cstr!("foo=bar"), Ok(&[("foo", Some("=bar"))]));
+ check(cstr!(" foo=bar"), Ok(&[("foo", Some("=bar"))]));
+ check(cstr!("foo=bar "), Ok(&[("foo", Some("=bar"))]));
+ check(cstr!(" foo=bar "), Ok(&[("foo", Some("=bar"))]));
+
+ check(cstr!("foo="), Ok(&[("foo", Some("="))]));
+ check(cstr!(" foo="), Ok(&[("foo", Some("="))]));
+ check(cstr!("foo= "), Ok(&[("foo", Some("="))]));
+ check(cstr!(" foo= "), Ok(&[("foo", Some("="))]));
+ }
+
+ #[test]
+ fn single_with_quote() {
+ check(cstr!("foo=hello\" \"world"), Ok(&[("foo", Some("=hello\" \"world"))]));
+ }
+
+ #[test]
+ fn invalid_encoding() {
+ check(CStr::from_bytes_with_nul(&[255, 255, 255, 0]).unwrap(), Err(()));
+ }
+
+ #[test]
+ fn multiple() {
+ check(
+ cstr!(" a=b c=d e= f g "),
+ Ok(&[("a", Some("=b")), ("c", Some("=d")), ("e", Some("=")), ("f", None), ("g", None)]),
+ );
+ check(
+ cstr!(" a=b \n c=d e= f g"),
+ Ok(&[("a", Some("=b")), ("c", Some("=d")), ("e", Some("=")), ("f", None), ("g", None)]),
+ );
+ }
+
+ #[test]
+ fn incomplete_quote() {
+ check(
+ cstr!("foo=incomplete\" quote bar=y"),
+ Ok(&[("foo", Some("=incomplete\" quote bar=y"))]),
+ );
+ }
+
+ #[test]
+ fn complex() {
+ check(cstr!(" a a1= b=c d=e,f,g x=\"value with quote\" y=val\"ue with \"multiple\" quo\"te "), Ok(&[
+ ("a", None),
+ ("a1", Some("=")),
+ ("b", Some("=c")),
+ ("d", Some("=e,f,g")),
+ ("x", Some("=\"value with quote\"")),
+ ("y", Some("=val\"ue with \"multiple\" quo\"te")),
+ ]));
+ }
+}
diff --git a/pvmfw/src/fdt.rs b/pvmfw/src/fdt.rs
index d15eaba..c68fc6d 100644
--- a/pvmfw/src/fdt.rs
+++ b/pvmfw/src/fdt.rs
@@ -14,12 +14,14 @@
//! High-level FDT functions.
+use crate::bootargs::BootArgsIterator;
use crate::cstr;
use crate::helpers::flatten;
use crate::helpers::GUEST_PAGE_SIZE;
use crate::helpers::SIZE_4KB;
use crate::memory::BASE_ADDR;
use crate::memory::MAX_ADDR;
+use crate::Box;
use crate::RebootReason;
use alloc::ffi::CString;
use alloc::vec::Vec;
@@ -97,7 +99,9 @@
fn patch_bootargs(fdt: &mut Fdt, bootargs: &CStr) -> libfdt::Result<()> {
let mut node = fdt.chosen_mut()?.ok_or(FdtError::NotFound)?;
- // TODO(b/275306568) filter out dangerous options
+ // This function is called before the verification is done. So, we just copy the bootargs to
+ // the new FDT unmodified. This will be filtered again in the modify_for_next_stage function
+ // if the VM is not debuggable.
node.setprop(cstr!("bootargs"), bootargs.to_bytes_with_nul())
}
@@ -675,6 +679,7 @@
new_instance: bool,
strict_boot: bool,
debug_policy: Option<&mut [u8]>,
+ debuggable: bool,
) -> libfdt::Result<()> {
fdt.unpack()?;
@@ -690,6 +695,12 @@
info!("No debug policy found.");
}
+ if debuggable {
+ if let Some(bootargs) = read_bootargs_from(fdt)? {
+ filter_out_dangerous_bootargs(fdt, &bootargs)?;
+ }
+ }
+
fdt.pack()?;
Ok(())
@@ -745,3 +756,48 @@
}
Ok(())
}
+
+fn read_common_debug_policy(fdt: &Fdt, debug_feature_name: &CStr) -> libfdt::Result<bool> {
+ if let Some(node) = fdt.node(cstr!("/avf/guest/common"))? {
+ if let Some(value) = node.getprop_u32(debug_feature_name)? {
+ return Ok(value == 1);
+ }
+ }
+ Ok(false) // if the policy doesn't exist or not 1, don't enable the debug feature
+}
+
+fn filter_out_dangerous_bootargs(fdt: &mut Fdt, bootargs: &CStr) -> libfdt::Result<()> {
+ let has_crashkernel = read_common_debug_policy(fdt, cstr!("ramdump"))?;
+ let has_console = read_common_debug_policy(fdt, cstr!("log"))?;
+
+ let accepted: &[(&str, Box<dyn Fn(Option<&str>) -> bool>)] = &[
+ ("panic", Box::new(|v| if let Some(v) = v { v == "=-1" } else { false })),
+ ("crashkernel", Box::new(|_| has_crashkernel)),
+ ("console", Box::new(|_| has_console)),
+ ];
+
+ // parse and filter out unwanted
+ let mut filtered = Vec::new();
+ for arg in BootArgsIterator::new(bootargs).map_err(|e| {
+ info!("Invalid bootarg: {e}");
+ FdtError::BadValue
+ })? {
+ match accepted.iter().find(|&t| t.0 == arg.name()) {
+ Some((_, pred)) if pred(arg.value()) => filtered.push(arg),
+ _ => debug!("Rejected bootarg {}", arg.as_ref()),
+ }
+ }
+
+ // flatten into a new C-string
+ let mut new_bootargs = Vec::new();
+ for (i, arg) in filtered.iter().enumerate() {
+ if i != 0 {
+ new_bootargs.push(b' '); // separator
+ }
+ new_bootargs.extend_from_slice(arg.as_ref().as_bytes());
+ }
+ new_bootargs.push(b'\0');
+
+ let mut node = fdt.chosen_mut()?.ok_or(FdtError::NotFound)?;
+ node.setprop(cstr!("bootargs"), new_bootargs.as_slice())
+}
diff --git a/pvmfw/src/main.rs b/pvmfw/src/main.rs
index 662d705..a773f1a 100644
--- a/pvmfw/src/main.rs
+++ b/pvmfw/src/main.rs
@@ -19,6 +19,7 @@
extern crate alloc;
+mod bootargs;
mod config;
mod crypto;
mod dice;
@@ -52,6 +53,7 @@
use libfdt::Fdt;
use log::{debug, error, info, trace};
use pvmfw_avb::verify_payload;
+use pvmfw_avb::DebugLevel;
use pvmfw_embedded_key::PUBLIC_KEY;
const NEXT_BCC_SIZE: usize = GUEST_PAGE_SIZE;
@@ -119,10 +121,12 @@
flush(next_bcc);
let strict_boot = true;
- modify_for_next_stage(fdt, next_bcc, new_instance, strict_boot, debug_policy).map_err(|e| {
- error!("Failed to configure device tree: {e}");
- RebootReason::InternalError
- })?;
+ let debuggable = verified_boot_data.debug_level != DebugLevel::None;
+ modify_for_next_stage(fdt, next_bcc, new_instance, strict_boot, debug_policy, debuggable)
+ .map_err(|e| {
+ error!("Failed to configure device tree: {e}");
+ RebootReason::InternalError
+ })?;
info!("Starting payload...");
Ok(())