Merge "[avb] Refactor the error thrown by payload verification API"
diff --git a/TEST_MAPPING b/TEST_MAPPING
index c37fd19..4f879b4 100644
--- a/TEST_MAPPING
+++ b/TEST_MAPPING
@@ -43,6 +43,9 @@
"path": "packages/modules/Virtualization/avmd"
},
{
+ "path": "packages/modules/Virtualization/encryptedstore"
+ },
+ {
"path": "packages/modules/Virtualization/virtualizationservice"
},
{
diff --git a/apkdmverity/src/main.rs b/apkdmverity/src/main.rs
index 6e12e38..50b6069 100644
--- a/apkdmverity/src/main.rs
+++ b/apkdmverity/src/main.rs
@@ -23,7 +23,7 @@
use anyhow::{bail, Context, Result};
use apkverify::{HashAlgorithm, V4Signature};
-use clap::{App, Arg};
+use clap::{arg, Arg, ArgAction, Command};
use dm::loopdevice;
use dm::util;
use dm::verity::{DmVerityHashAlgorithm, DmVerityTargetBuilder};
@@ -34,22 +34,12 @@
use std::path::{Path, PathBuf};
fn main() -> Result<()> {
- let matches = App::new("apkdmverity")
- .about("Creates a dm-verity block device out of APK signed with APK signature scheme V4.")
- .arg(Arg::from_usage(
- "--apk... <apk_path> <idsig_path> <name> <root_hash> \
- 'Input APK file, idsig file, name of the block device, and root hash. \
- The APK file must be signed using the APK signature scheme 4. The \
- block device is created at \"/dev/mapper/<name>\".' root_hash is \
- optional; idsig file's root hash will be used if specified as \"none\"."
- ))
- .arg(Arg::with_name("verbose").short('v').long("verbose").help("Shows verbose output"))
- .get_matches();
+ let matches = clap_command().get_matches();
- let apks = matches.values_of("apk").unwrap();
+ let apks = matches.get_many::<String>("apk").unwrap();
assert!(apks.len() % 4 == 0);
- let verbose = matches.is_present("verbose");
+ let verbose = matches.get_flag("verbose");
for (apk, idsig, name, roothash) in apks.tuples() {
let roothash = if roothash != "none" {
@@ -68,6 +58,28 @@
Ok(())
}
+fn clap_command() -> Command {
+ Command::new("apkdmverity")
+ .about("Creates a dm-verity block device out of APK signed with APK signature scheme V4.")
+ .arg(
+ arg!(--apk ...
+ "Input APK file, idsig file, name of the block device, and root hash. \
+ The APK file must be signed using the APK signature scheme 4. The \
+ block device is created at \"/dev/mapper/<name>\".' root_hash is \
+ optional; idsig file's root hash will be used if specified as \"none\"."
+ )
+ .action(ArgAction::Append)
+ .value_names(&["apk_path", "idsig_path", "name", "root_hash"]),
+ )
+ .arg(
+ Arg::new("verbose")
+ .short('v')
+ .long("verbose")
+ .action(ArgAction::SetTrue)
+ .help("Shows verbose output"),
+ )
+}
+
struct VerityResult {
data_device: PathBuf,
hash_device: PathBuf,
@@ -380,4 +392,10 @@
},
);
}
+
+ #[test]
+ fn verify_command() {
+ // Check that the command parsing has been configured in a valid way.
+ clap_command().debug_assert();
+ }
}
diff --git a/authfs/TEST_MAPPING b/authfs/TEST_MAPPING
index ab6111b..5e144c7 100644
--- a/authfs/TEST_MAPPING
+++ b/authfs/TEST_MAPPING
@@ -4,6 +4,9 @@
"name": "authfs_device_test_src_lib"
},
{
+ "name": "open_then_run.test"
+ },
+ {
"name": "AuthFsHostTest"
}
],
diff --git a/authfs/tests/common/Android.bp b/authfs/tests/common/Android.bp
index ec426c7..01ebcfd 100644
--- a/authfs/tests/common/Android.bp
+++ b/authfs/tests/common/Android.bp
@@ -31,3 +31,19 @@
test_suites: ["general-tests"],
test_harness: false,
}
+
+rust_test {
+ name: "open_then_run.test",
+ crate_name: "open_then_run",
+ srcs: ["src/open_then_run.rs"],
+ edition: "2021",
+ rustlibs: [
+ "libandroid_logger",
+ "libanyhow",
+ "libclap",
+ "libcommand_fds",
+ "liblibc",
+ "liblog_rust",
+ ],
+ test_suites: ["general-tests"],
+}
diff --git a/authfs/tests/common/src/open_then_run.rs b/authfs/tests/common/src/open_then_run.rs
index 110d838..6d828e4 100644
--- a/authfs/tests/common/src/open_then_run.rs
+++ b/authfs/tests/common/src/open_then_run.rs
@@ -19,7 +19,7 @@
//! specified numbers in the child process.
use anyhow::{bail, Context, Result};
-use clap::{App, Arg, Values};
+use clap::{parser::ValuesRef, Arg, ArgAction};
use command_fds::{CommandFdExt, FdMapping};
use log::{debug, error};
use std::fs::OpenOptions;
@@ -51,7 +51,7 @@
}
fn parse_and_create_file_mapping<F>(
- values: Option<Values<'_>>,
+ values: Option<ValuesRef<'_, String>>,
opener: F,
) -> Result<Vec<OwnedFdMapping>>
where
@@ -75,35 +75,35 @@
}
}
-fn parse_args() -> Result<Args> {
- #[rustfmt::skip]
- let matches = App::new("open_then_run")
- .arg(Arg::with_name("open-ro")
+#[rustfmt::skip]
+fn args_command() -> clap::Command {
+ clap::Command::new("open_then_run")
+ .arg(Arg::new("open-ro")
.long("open-ro")
.value_name("FD:PATH")
.help("Open <PATH> read-only to pass as fd <FD>")
- .multiple(true)
- .number_of_values(1))
- .arg(Arg::with_name("open-rw")
+ .action(ArgAction::Append))
+ .arg(Arg::new("open-rw")
.long("open-rw")
.value_name("FD:PATH")
.help("Open/create <PATH> read-write to pass as fd <FD>")
- .multiple(true)
- .number_of_values(1))
- .arg(Arg::with_name("open-dir")
+ .action(ArgAction::Append))
+ .arg(Arg::new("open-dir")
.long("open-dir")
.value_name("FD:DIR")
.help("Open <DIR> to pass as fd <FD>")
- .multiple(true)
- .number_of_values(1))
- .arg(Arg::with_name("args")
+ .action(ArgAction::Append))
+ .arg(Arg::new("args")
.help("Command line to execute with pre-opened FD inherited")
.last(true)
.required(true)
- .multiple(true))
- .get_matches();
+ .num_args(0..))
+}
- let ro_file_fds = parse_and_create_file_mapping(matches.values_of("open-ro"), |path| {
+fn parse_args() -> Result<Args> {
+ let matches = args_command().get_matches();
+
+ let ro_file_fds = parse_and_create_file_mapping(matches.get_many("open-ro"), |path| {
Ok(OwnedFd::from(
OpenOptions::new()
.read(true)
@@ -112,7 +112,7 @@
))
})?;
- let rw_file_fds = parse_and_create_file_mapping(matches.values_of("open-rw"), |path| {
+ let rw_file_fds = parse_and_create_file_mapping(matches.get_many("open-rw"), |path| {
Ok(OwnedFd::from(
OpenOptions::new()
.read(true)
@@ -123,7 +123,7 @@
))
})?;
- let dir_fds = parse_and_create_file_mapping(matches.values_of("open-dir"), |path| {
+ let dir_fds = parse_and_create_file_mapping(matches.get_many("open-dir"), |path| {
Ok(OwnedFd::from(
OpenOptions::new()
.custom_flags(libc::O_DIRECTORY)
@@ -133,7 +133,8 @@
))
})?;
- let cmdline_args: Vec<_> = matches.values_of("args").unwrap().map(|s| s.to_string()).collect();
+ let cmdline_args: Vec<_> =
+ matches.get_many::<String>("args").unwrap().map(|s| s.to_string()).collect();
Ok(Args { ro_file_fds, rw_file_fds, dir_fds, cmdline_args })
}
@@ -168,3 +169,14 @@
std::process::exit(1);
}
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ #[test]
+ fn verify_command() {
+ // Check that the command parsing has been configured in a valid way.
+ args_command().debug_assert();
+ }
+}
diff --git a/avmd/Android.bp b/avmd/Android.bp
index 0b87a7b..e5e0553 100644
--- a/avmd/Android.bp
+++ b/avmd/Android.bp
@@ -20,8 +20,8 @@
defaults: ["libavmd_defaults"],
}
-rust_binary {
- name: "avmdtool",
+rust_defaults {
+ name: "avmdtool.defaults",
srcs: ["src/main.rs"],
host_supported: true,
prefer_rlib: true,
@@ -37,6 +37,17 @@
],
}
+rust_binary {
+ name: "avmdtool",
+ defaults: ["avmdtool.defaults"],
+}
+
+rust_test {
+ name: "avmdtool.test",
+ defaults: ["avmdtool.defaults"],
+ test_suites: ["general-tests"],
+}
+
rust_test {
name: "avmdtool_tests",
srcs: ["tests/*_test.rs"],
diff --git a/avmd/TEST_MAPPING b/avmd/TEST_MAPPING
index ea58edb..892eb2c 100644
--- a/avmd/TEST_MAPPING
+++ b/avmd/TEST_MAPPING
@@ -1,7 +1,10 @@
{
- "avf-presubmit" : [
+ "avf-presubmit": [
{
- "name" : "avmdtool_tests"
+ "name": "avmdtool.test"
+ },
+ {
+ "name": "avmdtool_tests"
}
]
}
diff --git a/avmd/src/main.rs b/avmd/src/main.rs
index fc18225..740e9aa 100644
--- a/avmd/src/main.rs
+++ b/avmd/src/main.rs
@@ -18,9 +18,13 @@
use apexutil::get_payload_vbmeta_image_hash;
use apkverify::get_apk_digest;
use avmd::{ApkDescriptor, Avmd, Descriptor, ResourceIdentifier, VbMetaDescriptor};
-use clap::{App, AppSettings, Arg, ArgMatches, SubCommand};
+use clap::{
+ builder::ValueParser,
+ parser::{Indices, ValuesRef},
+ Arg, ArgAction, ArgMatches, Command,
+};
use serde::ser::Serialize;
-use std::fs::File;
+use std::{fs::File, path::PathBuf};
use vbmeta::VbMetaImage;
fn get_vbmeta_image_hash(file: &str) -> Result<Vec<u8>> {
@@ -31,13 +35,13 @@
/// Iterate over a set of argument values, that could be empty or come in
/// (<index>, <namespace>, <name>, <file>) tuple.
struct NamespaceNameFileIterator<'a> {
- indices: Option<clap::Indices<'a>>,
- values: Option<clap::Values<'a>>,
+ indices: Option<Indices<'a>>,
+ values: Option<ValuesRef<'a, String>>,
}
impl<'a> NamespaceNameFileIterator<'a> {
fn new(args: &'a ArgMatches, name: &'a str) -> Self {
- NamespaceNameFileIterator { indices: args.indices_of(name), values: args.values_of(name) }
+ NamespaceNameFileIterator { indices: args.indices_of(name), values: args.get_many(name) }
}
}
@@ -100,54 +104,58 @@
.packed_format()
.legacy_enums(),
)?;
- std::fs::write(args.value_of("file").unwrap(), &bytes)?;
+ std::fs::write(args.get_one::<PathBuf>("file").unwrap(), &bytes)?;
Ok(())
}
fn dump(args: &ArgMatches) -> Result<()> {
- let file = std::fs::read(args.value_of("file").unwrap())?;
+ let file = std::fs::read(args.get_one::<PathBuf>("file").unwrap())?;
let avmd: Avmd = serde_cbor::from_slice(&file)?;
println!("{}", avmd);
Ok(())
}
-fn main() -> Result<()> {
+fn clap_command() -> Command {
let namespace_name_file = ["namespace", "name", "file"];
- let app = App::new("avmdtool")
- .setting(AppSettings::SubcommandRequiredElseHelp)
+
+ Command::new("avmdtool")
+ .subcommand_required(true)
+ .arg_required_else_help(true)
.subcommand(
- SubCommand::with_name("create")
- .setting(AppSettings::ArgRequiredElseHelp)
- .arg(Arg::with_name("file").required(true).takes_value(true))
+ Command::new("create")
+ .arg_required_else_help(true)
+ .arg(Arg::new("file").value_parser(ValueParser::path_buf()).required(true))
.arg(
- Arg::with_name("vbmeta")
+ Arg::new("vbmeta")
.long("vbmeta")
- .takes_value(true)
.value_names(&namespace_name_file)
- .multiple(true),
+ .num_args(3)
+ .action(ArgAction::Append),
)
.arg(
- Arg::with_name("apk")
+ Arg::new("apk")
.long("apk")
- .takes_value(true)
.value_names(&namespace_name_file)
- .multiple(true),
+ .num_args(3)
+ .action(ArgAction::Append),
)
.arg(
- Arg::with_name("apex-payload")
+ Arg::new("apex-payload")
.long("apex-payload")
- .takes_value(true)
.value_names(&namespace_name_file)
- .multiple(true),
+ .num_args(3)
+ .action(ArgAction::Append),
),
)
.subcommand(
- SubCommand::with_name("dump")
- .setting(AppSettings::ArgRequiredElseHelp)
- .arg(Arg::with_name("file").required(true).takes_value(true)),
- );
+ Command::new("dump")
+ .arg_required_else_help(true)
+ .arg(Arg::new("file").value_parser(ValueParser::path_buf()).required(true)),
+ )
+}
- let args = app.get_matches();
+fn main() -> Result<()> {
+ let args = clap_command().get_matches();
match args.subcommand() {
Some(("create", sub_args)) => create(sub_args)?,
Some(("dump", sub_args)) => dump(sub_args)?,
@@ -155,3 +163,14 @@
}
Ok(())
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ #[test]
+ fn verify_command() {
+ // Check that the command parsing has been configured in a valid way.
+ clap_command().debug_assert();
+ }
+}
diff --git a/compos/common/compos_client.rs b/compos/common/compos_client.rs
index 34be1d5..f6811cb 100644
--- a/compos/common/compos_client.rs
+++ b/compos/common/compos_client.rs
@@ -222,10 +222,10 @@
bail!("Protected VM not supported, unable to start VM");
}
- let have_unprotected_vm =
+ let have_non_protected_vm =
system_properties::read_bool("ro.boot.hypervisor.vm.supported", false)?;
- if have_unprotected_vm {
- warn!("Protected VM not supported, falling back to unprotected on debuggable build");
+ if have_non_protected_vm {
+ warn!("Protected VM not supported, falling back to non-protected on debuggable build");
return Ok(false);
}
diff --git a/encryptedstore/Android.bp b/encryptedstore/Android.bp
index 13ef1b9..94ebcfc 100644
--- a/encryptedstore/Android.bp
+++ b/encryptedstore/Android.bp
@@ -29,3 +29,9 @@
defaults: ["encryptedstore.defaults"],
bootstrap: true,
}
+
+rust_test {
+ name: "encryptedstore.test",
+ defaults: ["encryptedstore.defaults"],
+ test_suites: ["general-tests"],
+}
diff --git a/encryptedstore/TEST_MAPPING b/encryptedstore/TEST_MAPPING
new file mode 100644
index 0000000..a9e1d87
--- /dev/null
+++ b/encryptedstore/TEST_MAPPING
@@ -0,0 +1,7 @@
+{
+ "avf-presubmit": [
+ {
+ "name": "encryptedstore.test"
+ }
+ ]
+}
diff --git a/encryptedstore/src/main.rs b/encryptedstore/src/main.rs
index 7140ae2..888485b 100644
--- a/encryptedstore/src/main.rs
+++ b/encryptedstore/src/main.rs
@@ -19,9 +19,8 @@
//! It uses dm_rust lib.
use anyhow::{ensure, Context, Result};
-use clap::{arg, App};
-use dm::crypt::CipherType;
-use dm::util;
+use clap::arg;
+use dm::{crypt::CipherType, util};
use log::info;
use std::ffi::CString;
use std::fs::{create_dir_all, OpenOptions};
@@ -42,18 +41,11 @@
);
info!("Starting encryptedstore binary");
- let matches = App::new("encryptedstore")
- .args(&[
- arg!(--blkdevice <FILE> "the block device backing the encrypted storage")
- .required(true),
- arg!(--key <KEY> "key (in hex) equivalent to 32 bytes)").required(true),
- arg!(--mountpoint <MOUNTPOINT> "mount point for the storage").required(true),
- ])
- .get_matches();
+ let matches = clap_command().get_matches();
- let blkdevice = Path::new(matches.value_of("blkdevice").unwrap());
- let key = matches.value_of("key").unwrap();
- let mountpoint = Path::new(matches.value_of("mountpoint").unwrap());
+ let blkdevice = Path::new(matches.get_one::<String>("blkdevice").unwrap());
+ let key = matches.get_one::<String>("key").unwrap();
+ let mountpoint = Path::new(matches.get_one::<String>("mountpoint").unwrap());
encryptedstore_init(blkdevice, key, mountpoint).context(format!(
"Unable to initialize encryptedstore on {:?} & mount at {:?}",
blkdevice, mountpoint
@@ -61,6 +53,14 @@
Ok(())
}
+fn clap_command() -> clap::Command {
+ clap::Command::new("encryptedstore").args(&[
+ arg!(--blkdevice <FILE> "the block device backing the encrypted storage").required(true),
+ arg!(--key <KEY> "key (in hex) equivalent to 32 bytes)").required(true),
+ arg!(--mountpoint <MOUNTPOINT> "mount point for the storage").required(true),
+ ])
+}
+
fn encryptedstore_init(blkdevice: &Path, key: &str, mountpoint: &Path) -> Result<()> {
ensure!(
std::fs::metadata(&blkdevice)
@@ -160,3 +160,14 @@
Ok(())
}
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ #[test]
+ fn verify_command() {
+ // Check that the command parsing has been configured in a valid way.
+ clap_command().debug_assert();
+ }
+}
diff --git a/javalib/src/android/system/virtualmachine/VirtualMachineConfig.java b/javalib/src/android/system/virtualmachine/VirtualMachineConfig.java
index 0e9e86b..d9fc70c 100644
--- a/javalib/src/android/system/virtualmachine/VirtualMachineConfig.java
+++ b/javalib/src/android/system/virtualmachine/VirtualMachineConfig.java
@@ -352,6 +352,7 @@
* that would alter the identity of the VM (e.g. using a different payload or changing the debug
* mode) are considered incompatible.
*
+ * @see VirtualMachine#setConfig
* @hide
*/
@SystemApi
@@ -536,6 +537,14 @@
/**
* Sets the debug level. Defaults to {@link #DEBUG_LEVEL_NONE}.
*
+ * <p>If {@link #DEBUG_LEVEL_FULL} is set then logs from inside the VM are exported to the
+ * host and adb connections from the host are possible. This is convenient for debugging but
+ * may compromise the integrity of the VM - including bypassing the protections offered by a
+ * {@linkplain #setProtectedVm protected VM}.
+ *
+ * <p>Note that it isn't possible to {@linkplain #isCompatibleWith change} the debug level
+ * of a VM instance; debug and non-debug VMs always have different secrets.
+ *
* @hide
*/
@SystemApi
@@ -552,6 +561,13 @@
* Sets whether to protect the VM memory from the host. No default is provided, this must be
* set explicitly.
*
+ * <p>Note that if debugging is {@linkplain #setDebugLevel enabled} for a protected VM, the
+ * VM is not truly protected - direct memory access by the host is prevented, but e.g. the
+ * debugger can be used to access the VM's internals.
+ *
+ * <p>It isn't possible to {@linkplain #isCompatibleWith change} the protected status of a
+ * VM instance; protected and non-protected VMs always have different secrets.
+ *
* @see VirtualMachineManager#getCapabilities
* @hide
*/
@@ -566,7 +582,7 @@
} else {
if (!HypervisorProperties.hypervisor_vm_supported().orElse(false)) {
throw new UnsupportedOperationException(
- "Unprotected VMs are not supported on this device.");
+ "Non-protected VMs are not supported on this device.");
}
}
mProtectedVm = protectedVm;
diff --git a/microdroid/Android.bp b/microdroid/Android.bp
index b46f112..1fc44a2 100644
--- a/microdroid/Android.bp
+++ b/microdroid/Android.bp
@@ -544,7 +544,7 @@
avb_gen_vbmeta_image {
name: "microdroid_initrd_normal_hashdesc",
src: ":microdroid_initrd_normal",
- partition_name: "vbmeta_initrd_normal",
+ partition_name: "initrd_normal",
salt: initrd_normal_salt,
enabled: false,
arch: {
@@ -564,7 +564,7 @@
avb_gen_vbmeta_image {
name: "microdroid_initrd_debug_hashdesc",
src: ":microdroid_initrd_debuggable",
- partition_name: "vbmeta_initrd_debug",
+ partition_name: "initrd_debug",
salt: initrd_debug_salt,
enabled: false,
arch: {
@@ -582,7 +582,7 @@
name: "microdroid_kernel_signed",
src: "empty_kernel",
filename: "microdroid_kernel",
- partition_name: "bootloader",
+ partition_name: "boot",
private_key: ":microdroid_sign_key",
salt: bootloader_salt,
enabled: false,
diff --git a/microdroid/init.rc b/microdroid/init.rc
index 7402481..bc42791 100644
--- a/microdroid/init.rc
+++ b/microdroid/init.rc
@@ -165,7 +165,6 @@
class core
critical
seclabel u:r:ueventd:s0
- shutdown critical
capabilities CHOWN DAC_OVERRIDE DAC_READ_SEARCH FOWNER FSETID MKNOD NET_ADMIN SETGID SETUID SYS_MODULE SYS_RAWIO
service console /system/bin/sh
diff --git a/microdroid/initrd/src/main.rs b/microdroid/initrd/src/main.rs
index 69c6ae4..3f1fab5 100644
--- a/microdroid/initrd/src/main.rs
+++ b/microdroid/initrd/src/main.rs
@@ -54,7 +54,8 @@
checksum += get_checksum(&bootconfig)?;
}
- let padding_size: usize = FOOTER_ALIGNMENT - (initrd_size + bootconfig_size) % FOOTER_ALIGNMENT;
+ let padding_size: usize =
+ (FOOTER_ALIGNMENT - (initrd_size + bootconfig_size) % FOOTER_ALIGNMENT) % FOOTER_ALIGNMENT;
output_file.write_all(&ZEROS[..padding_size])?;
output_file.write_all(&((padding_size + bootconfig_size) as u32).to_le_bytes())?;
output_file.write_all(&checksum.to_le_bytes())?;
diff --git a/pvmfw/avb/Android.bp b/pvmfw/avb/Android.bp
index cbec235..d3a5e4e 100644
--- a/pvmfw/avb/Android.bp
+++ b/pvmfw/avb/Android.bp
@@ -62,7 +62,7 @@
avb_add_hash_footer {
name: "test_image_with_one_hashdesc",
src: ":unsigned_test_image",
- partition_name: "bootloader",
+ partition_name: "boot",
private_key: ":pvmfw_sign_key",
salt: "1111",
}
diff --git a/pvmfw/avb/src/verify.rs b/pvmfw/avb/src/verify.rs
index 3e8c71b..fb18626 100644
--- a/pvmfw/avb/src/verify.rs
+++ b/pvmfw/avb/src/verify.rs
@@ -101,7 +101,7 @@
out_pointer: *mut *mut u8,
out_num_bytes_preloaded: *mut usize,
) -> Result<(), AvbIOError> {
- let ops = as_avbops_ref(ops)?;
+ let ops = as_ref(ops)?;
let partition = ops.as_ref().get_partition(partition)?;
let out_pointer = to_nonnull(out_pointer)?;
// SAFETY: It is safe as the raw pointer `out_pointer` is a nonnull pointer.
@@ -143,7 +143,7 @@
buffer: *mut c_void,
out_num_read: *mut usize,
) -> Result<(), AvbIOError> {
- let ops = as_avbops_ref(ops)?;
+ let ops = as_ref(ops)?;
let partition = ops.as_ref().get_partition(partition)?;
let buffer = to_nonnull(buffer)?;
// SAFETY: It is safe to copy the requested number of bytes to `buffer` as `buffer`
@@ -185,7 +185,7 @@
partition: *const c_char,
out_size_num_bytes: *mut u64,
) -> Result<(), AvbIOError> {
- let ops = as_avbops_ref(ops)?;
+ let ops = as_ref(ops)?;
let partition = ops.as_ref().get_partition(partition)?;
let partition_size =
u64::try_from(partition.len()).map_err(|_| AvbIOError::InvalidValueSize)?;
@@ -256,7 +256,7 @@
// `public_key_data` is a valid pointer and it points to an array of length
// `public_key_length`.
let public_key = unsafe { slice::from_raw_parts(public_key_data, public_key_length) };
- let ops = as_avbops_ref(ops)?;
+ let ops = as_ref(ops)?;
// Verifies the public key for the known partitions only.
ops.as_ref().get_partition(partition)?;
let trusted_public_key = ops.as_ref().trusted_public_key;
@@ -268,14 +268,14 @@
Ok(())
}
-fn as_avbops_ref<'a>(ops: *mut AvbOps) -> Result<&'a AvbOps, AvbIOError> {
- let ops = to_nonnull(ops)?;
- // SAFETY: It is safe as the raw pointer `ops` is a nonnull pointer.
- unsafe { Ok(ops.as_ref()) }
+fn as_ref<'a, T>(ptr: *mut T) -> Result<&'a T, AvbIOError> {
+ let ptr = to_nonnull(ptr)?;
+ // SAFETY: It is safe as the raw pointer `ptr` is a nonnull pointer.
+ unsafe { Ok(ptr.as_ref()) }
}
-fn to_nonnull<T>(p: *mut T) -> Result<NonNull<T>, AvbIOError> {
- NonNull::new(p).ok_or(AvbIOError::NoSuchValue)
+fn to_nonnull<T>(ptr: *mut T) -> Result<NonNull<T>, AvbIOError> {
+ NonNull::new(ptr).ok_or(AvbIOError::NoSuchValue)
}
fn is_not_null<T>(ptr: *const T) -> Result<(), AvbIOError> {
@@ -286,6 +286,41 @@
}
}
+#[derive(Clone, Debug, PartialEq, Eq)]
+enum PartitionName {
+ Kernel,
+ InitrdNormal,
+ InitrdDebug,
+}
+
+impl PartitionName {
+ const KERNEL_PARTITION_NAME: &[u8] = b"boot\0";
+ const INITRD_NORMAL_PARTITION_NAME: &[u8] = b"initrd_normal\0";
+ const INITRD_DEBUG_PARTITION_NAME: &[u8] = b"initrd_debug\0";
+
+ fn as_cstr(&self) -> &CStr {
+ let partition_name = match self {
+ Self::Kernel => Self::KERNEL_PARTITION_NAME,
+ Self::InitrdNormal => Self::INITRD_NORMAL_PARTITION_NAME,
+ Self::InitrdDebug => Self::INITRD_DEBUG_PARTITION_NAME,
+ };
+ CStr::from_bytes_with_nul(partition_name).unwrap()
+ }
+}
+
+impl TryFrom<&CStr> for PartitionName {
+ type Error = AvbIOError;
+
+ fn try_from(partition_name: &CStr) -> Result<Self, Self::Error> {
+ match partition_name.to_bytes_with_nul() {
+ Self::KERNEL_PARTITION_NAME => Ok(Self::Kernel),
+ Self::INITRD_NORMAL_PARTITION_NAME => Ok(Self::InitrdNormal),
+ Self::INITRD_DEBUG_PARTITION_NAME => Ok(Self::InitrdDebug),
+ _ => Err(AvbIOError::NoSuchPartition),
+ }
+ }
+}
+
struct Payload<'a> {
kernel: &'a [u8],
initrd: Option<&'a [u8]>,
@@ -304,22 +339,17 @@
}
impl<'a> Payload<'a> {
- const KERNEL_PARTITION_NAME: &[u8] = b"bootloader\0";
- const INITRD_NORMAL_PARTITION_NAME: &[u8] = b"initrd_normal\0";
- const INITRD_DEBUG_PARTITION_NAME: &[u8] = b"initrd_debug\0";
-
const MAX_NUM_OF_HASH_DESCRIPTORS: usize = 3;
fn get_partition(&self, partition_name: *const c_char) -> Result<&[u8], AvbIOError> {
is_not_null(partition_name)?;
// SAFETY: It is safe as the raw pointer `partition_name` is a nonnull pointer.
let partition_name = unsafe { CStr::from_ptr(partition_name) };
- match partition_name.to_bytes_with_nul() {
- Self::KERNEL_PARTITION_NAME => Ok(self.kernel),
- Self::INITRD_NORMAL_PARTITION_NAME | Self::INITRD_DEBUG_PARTITION_NAME => {
+ match partition_name.try_into()? {
+ PartitionName::Kernel => Ok(self.kernel),
+ PartitionName::InitrdNormal | PartitionName::InitrdDebug => {
self.initrd.ok_or(AvbIOError::NoSuchPartition)
}
- _ => Err(AvbIOError::NoSuchPartition),
}
}
@@ -377,8 +407,7 @@
trusted_public_key: &[u8],
) -> Result<(), AvbSlotVerifyError> {
let mut payload = Payload { kernel, initrd, trusted_public_key };
- let kernel = CStr::from_bytes_with_nul(Payload::KERNEL_PARTITION_NAME).unwrap();
- let requested_partitions = [kernel];
+ let requested_partitions = [PartitionName::Kernel.as_cstr()];
payload.verify_partitions(&requested_partitions)
}
diff --git a/rialto/tests/test.rs b/rialto/tests/test.rs
index 0447cd3..b25034f 100644
--- a/rialto/tests/test.rs
+++ b/rialto/tests/test.rs
@@ -33,7 +33,7 @@
const RIALTO_PATH: &str = "/data/local/tmp/rialto_test/arm64/rialto.bin";
-/// Runs the Rialto VM as an unprotected VM via VirtualizationService.
+/// Runs the Rialto VM as a non-protected VM via VirtualizationService.
#[test]
fn test_boots() -> Result<(), Error> {
android_logger::init_once(
diff --git a/virtualizationservice/src/aidl.rs b/virtualizationservice/src/aidl.rs
index 0859a76..f110af7 100644
--- a/virtualizationservice/src/aidl.rs
+++ b/virtualizationservice/src/aidl.rs
@@ -664,6 +664,16 @@
.try_for_each(check_label_for_partition)
.map_err(|e| Status::new_service_specific_error_str(-1, Some(format!("{:?}", e))))?;
+ let kernel = maybe_clone_file(&config.kernel)?;
+ let initrd = maybe_clone_file(&config.initrd)?;
+
+ // In a protected VM, we require custom kernels to come from a trusted source (b/237054515).
+ if config.protectedVm {
+ check_label_for_kernel_files(&kernel, &initrd).map_err(|e| {
+ Status::new_service_specific_error_str(-1, Some(format!("{:?}", e)))
+ })?;
+ }
+
let zero_filler_path = temporary_directory.join("zero.img");
write_zero_filler(&zero_filler_path).map_err(|e| {
error!("Failed to make composite image: {:?}", e);
@@ -706,8 +716,8 @@
cid,
name: config.name.clone(),
bootloader: maybe_clone_file(&config.bootloader)?,
- kernel: maybe_clone_file(&config.kernel)?,
- initrd: maybe_clone_file(&config.initrd)?,
+ kernel,
+ initrd,
disks,
params: config.params.to_owned(),
protected: *is_protected,
@@ -971,14 +981,8 @@
check_permission("android.permission.USE_CUSTOM_VIRTUAL_MACHINE")
}
-/// Check if a partition has selinux labels that are not allowed
-fn check_label_for_partition(partition: &Partition) -> Result<()> {
- let ctx = getfilecon(partition.image.as_ref().unwrap().as_ref())?;
- check_label_is_allowed(&ctx).with_context(|| format!("Partition {} invalid", &partition.label))
-}
-
-// Return whether a partition is exempt from selinux label checks, because we know that it does
-// not contain code and is likely to be generated in an app-writable directory.
+/// Return whether a partition is exempt from selinux label checks, because we know that it does
+/// not contain code and is likely to be generated in an app-writable directory.
fn is_safe_app_partition(label: &str) -> bool {
// See add_microdroid_system_images & add_microdroid_payload_images in payload.rs.
label == "vm-instance"
@@ -988,23 +992,46 @@
|| label.starts_with("extra-idsig-")
}
-fn check_label_is_allowed(ctx: &SeContext) -> Result<()> {
- // We only want to allow code in a VM payload to be sourced from places that apps, and the
- // system, do not have write access to.
- // (Note that sepolicy must also grant read access for these types to both virtualization
- // service and crosvm.)
- // App private data files are deliberately excluded, to avoid arbitrary payloads being run on
- // user devices (W^X).
- match ctx.selinux_type()? {
+/// Check that a file SELinux label is acceptable.
+///
+/// We only want to allow code in a VM to be sourced from places that apps, and the
+/// system, do not have write access to.
+///
+/// Note that sepolicy must also grant read access for these types to both virtualization
+/// service and crosvm.
+///
+/// App private data files are deliberately excluded, to avoid arbitrary payloads being run on
+/// user devices (W^X).
+fn check_label_is_allowed(context: &SeContext) -> Result<()> {
+ match context.selinux_type()? {
| "system_file" // immutable dm-verity protected partition
| "apk_data_file" // APKs of an installed app
| "staging_data_file" // updated/staged APEX images
| "shell_data_file" // test files created via adb shell
=> Ok(()),
- _ => bail!("Label {} is not allowed", ctx),
+ _ => bail!("Label {} is not allowed", context),
}
}
+fn check_label_for_partition(partition: &Partition) -> Result<()> {
+ let file = partition.image.as_ref().unwrap().as_ref();
+ check_label_is_allowed(&getfilecon(file)?)
+ .with_context(|| format!("Partition {} invalid", &partition.label))
+}
+
+fn check_label_for_kernel_files(kernel: &Option<File>, initrd: &Option<File>) -> Result<()> {
+ if let Some(f) = kernel {
+ check_label_for_file(f, "kernel")?;
+ }
+ if let Some(f) = initrd {
+ check_label_for_file(f, "initrd")?;
+ }
+ Ok(())
+}
+fn check_label_for_file(file: &File, name: &str) -> Result<()> {
+ check_label_is_allowed(&getfilecon(file)?).with_context(|| format!("{} file invalid", name))
+}
+
/// Implementation of the AIDL `IVirtualMachine` interface. Used as a handle to a VM.
#[derive(Debug)]
struct VirtualMachine {
diff --git a/virtualizationservice/src/virtmgr.rs b/virtualizationservice/src/virtmgr.rs
index 5616097..dca64cb 100644
--- a/virtualizationservice/src/virtmgr.rs
+++ b/virtualizationservice/src/virtmgr.rs
@@ -33,6 +33,7 @@
use nix::fcntl::{fcntl, F_GETFD, F_SETFD, FdFlag};
use nix::unistd::{Pid, Uid};
use std::os::unix::raw::{pid_t, uid_t};
+use rustutils::system_properties;
const LOG_TAG: &str = "virtmgr";
@@ -91,6 +92,11 @@
Ok(unsafe { OwnedFd::from_raw_fd(raw_fd) })
}
+fn is_property_set(name: &str) -> bool {
+ system_properties::read_bool(name, false)
+ .unwrap_or_else(|e| panic!("Failed to read {name}: {e:?}"))
+}
+
fn main() {
android_logger::init_once(
android_logger::Config::default()
@@ -99,6 +105,15 @@
.with_log_id(android_logger::LogId::System),
);
+ let non_protected_vm_supported = is_property_set("ro.boot.hypervisor.vm.supported");
+ let protected_vm_supported = is_property_set("ro.boot.hypervisor.protected_vm.supported");
+ if !non_protected_vm_supported && !protected_vm_supported {
+ // This should never happen, it indicates a misconfigured device where the virt APEX
+ // is present but VMs are not supported. If it does happen, fail fast to avoid wasting
+ // resources trying.
+ panic!("Device doesn't support protected or unprotected VMs");
+ }
+
let args = Args::parse();
let mut owned_fds = vec![];
diff --git a/vm/TEST_MAPPING b/vm/TEST_MAPPING
index a8d1fa6..485c3af 100644
--- a/vm/TEST_MAPPING
+++ b/vm/TEST_MAPPING
@@ -1,7 +1,7 @@
{
- "avf-presubmit" : [
+ "avf-presubmit": [
{
- "name" : "vm.test"
+ "name": "vm.test"
}
]
}
diff --git a/vm/src/main.rs b/vm/src/main.rs
index 9fa805e..bfc7920 100644
--- a/vm/src/main.rs
+++ b/vm/src/main.rs
@@ -358,15 +358,15 @@
/// Print information about supported VM types.
fn command_info() -> Result<(), Error> {
- let unprotected_vm_supported =
+ let non_protected_vm_supported =
system_properties::read_bool("ro.boot.hypervisor.vm.supported", false)?;
let protected_vm_supported =
system_properties::read_bool("ro.boot.hypervisor.protected_vm.supported", false)?;
- match (unprotected_vm_supported, protected_vm_supported) {
+ match (non_protected_vm_supported, protected_vm_supported) {
(false, false) => println!("VMs are not supported."),
(false, true) => println!("Only protected VMs are supported."),
- (true, false) => println!("Only unprotected VMs are supported."),
- (true, true) => println!("Both protected and unprotected VMs are supported."),
+ (true, false) => println!("Only non-protected VMs are supported."),
+ (true, true) => println!("Both protected and non-protected VMs are supported."),
}
if let Some(version) = system_properties::read("ro.boot.hypervisor.version")? {
@@ -387,10 +387,11 @@
#[cfg(test)]
mod tests {
use super::*;
- use clap::IntoApp;
+ use clap::CommandFactory;
#[test]
fn verify_app() {
- Opt::into_app().debug_assert();
+ // Check that the command parsing has been configured in a valid way.
+ Opt::command().debug_assert();
}
}
diff --git a/zipfuse/src/main.rs b/zipfuse/src/main.rs
index 365d236..5e9e160 100644
--- a/zipfuse/src/main.rs
+++ b/zipfuse/src/main.rs
@@ -21,7 +21,7 @@
mod inode;
use anyhow::{Context as AnyhowContext, Result};
-use clap::{App, Arg};
+use clap::{builder::ValueParser, Arg, ArgAction, Command};
use fuse::filesystem::*;
use fuse::mount::*;
use rustutils::system_properties;
@@ -34,65 +34,58 @@
use std::mem::size_of;
use std::os::unix::io::AsRawFd;
use std::path::Path;
+use std::path::PathBuf;
use std::sync::Mutex;
use crate::inode::{DirectoryEntry, Inode, InodeData, InodeKind, InodeTable};
fn main() -> Result<()> {
- let matches = App::new("zipfuse")
+ let matches = clap_command().get_matches();
+
+ let zip_file = matches.get_one::<PathBuf>("ZIPFILE").unwrap();
+ let mount_point = matches.get_one::<PathBuf>("MOUNTPOINT").unwrap();
+ let options = matches.get_one::<String>("options");
+ let noexec = matches.get_flag("noexec");
+ let ready_prop = matches.get_one::<String>("readyprop");
+ let uid: u32 = matches.get_one::<String>("uid").map_or(0, |s| s.parse().unwrap());
+ let gid: u32 = matches.get_one::<String>("gid").map_or(0, |s| s.parse().unwrap());
+ run_fuse(zip_file, mount_point, options, noexec, ready_prop, uid, gid)?;
+
+ Ok(())
+}
+
+fn clap_command() -> Command {
+ Command::new("zipfuse")
.arg(
- Arg::with_name("options")
+ Arg::new("options")
.short('o')
- .takes_value(true)
.required(false)
.help("Comma separated list of mount options"),
)
.arg(
- Arg::with_name("noexec")
+ Arg::new("noexec")
.long("noexec")
- .takes_value(false)
+ .action(ArgAction::SetTrue)
.help("Disallow the execution of binary files"),
)
.arg(
- Arg::with_name("readyprop")
+ Arg::new("readyprop")
.short('p')
- .takes_value(true)
.help("Specify a property to be set when mount is ready"),
)
- .arg(
- Arg::with_name("uid")
- .short('u')
- .takes_value(true)
- .help("numeric UID who's the owner of the files"),
- )
- .arg(
- Arg::with_name("gid")
- .short('g')
- .takes_value(true)
- .help("numeric GID who's the group of the files"),
- )
- .arg(Arg::with_name("ZIPFILE").required(true))
- .arg(Arg::with_name("MOUNTPOINT").required(true))
- .get_matches();
-
- let zip_file = matches.value_of("ZIPFILE").unwrap().as_ref();
- let mount_point = matches.value_of("MOUNTPOINT").unwrap().as_ref();
- let options = matches.value_of("options");
- let noexec = matches.is_present("noexec");
- let ready_prop = matches.value_of("readyprop");
- let uid: u32 = matches.value_of("uid").map_or(0, |s| s.parse().unwrap());
- let gid: u32 = matches.value_of("gid").map_or(0, |s| s.parse().unwrap());
- run_fuse(zip_file, mount_point, options, noexec, ready_prop, uid, gid)?;
- Ok(())
+ .arg(Arg::new("uid").short('u').help("numeric UID who's the owner of the files"))
+ .arg(Arg::new("gid").short('g').help("numeric GID who's the group of the files"))
+ .arg(Arg::new("ZIPFILE").value_parser(ValueParser::path_buf()).required(true))
+ .arg(Arg::new("MOUNTPOINT").value_parser(ValueParser::path_buf()).required(true))
}
/// Runs a fuse filesystem by mounting `zip_file` on `mount_point`.
pub fn run_fuse(
zip_file: &Path,
mount_point: &Path,
- extra_options: Option<&str>,
+ extra_options: Option<&String>,
noexec: bool,
- ready_prop: Option<&str>,
+ ready_prop: Option<&String>,
uid: u32,
gid: u32,
) -> Result<()> {
@@ -471,11 +464,11 @@
#[cfg(test)]
mod tests {
- use anyhow::{bail, Result};
+ use super::*;
+ use anyhow::bail;
use nix::sys::statfs::{statfs, FsType};
use std::collections::BTreeSet;
use std::fs;
- use std::fs::File;
use std::io::Write;
use std::os::unix::fs::MetadataExt;
use std::path::{Path, PathBuf};
@@ -873,4 +866,10 @@
// Start zipfuse over to the loop device (not the zip file)
run_fuse_and_check_test_zip(&test_dir.path(), &ld.path().unwrap());
}
+
+ #[test]
+ fn verify_command() {
+ // Check that the command parsing has been configured in a valid way.
+ clap_command().debug_assert();
+ }
}