From 7cfc233cdc6e6c709594ff26640a350c2e6fb6a8 Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@realemail.net>
Date: Tue, 14 Nov 2023 15:03:19 +0100
Subject: [PATCH] PVF: fix detection of unshare-and-change-root security
capability (#2304)
---
Cargo.lock | 1 +
.../node/core/candidate-validation/src/lib.rs | 2 +-
polkadot/node/core/pvf/Cargo.toml | 1 +
.../benches/host_prepare_rococo_runtime.rs | 2 +-
.../node/core/pvf/common/src/worker/mod.rs | 6 +++---
polkadot/node/core/pvf/src/host.rs | 8 ++++++--
polkadot/node/core/pvf/src/security.rs | 20 +++++++++++++++++--
polkadot/node/core/pvf/tests/it/main.rs | 2 +-
8 files changed, 32 insertions(+), 10 deletions(-)
diff --git a/Cargo.lock b/Cargo.lock
index 32d9099b386..a9b6c68b50f 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -12453,6 +12453,7 @@ dependencies = [
"polkadot-node-core-pvf-prepare-worker",
"polkadot-node-metrics",
"polkadot-node-primitives",
+ "polkadot-node-subsystem",
"polkadot-parachain-primitives",
"polkadot-primitives",
"procfs",
diff --git a/polkadot/node/core/candidate-validation/src/lib.rs b/polkadot/node/core/candidate-validation/src/lib.rs
index 89ea0272884..4232e5f1cdd 100644
--- a/polkadot/node/core/candidate-validation/src/lib.rs
+++ b/polkadot/node/core/candidate-validation/src/lib.rs
@@ -150,7 +150,7 @@ async fn run<Context>(
),
pvf_metrics,
)
- .await;
+ .await?;
ctx.spawn_blocking("pvf-validation-host", task.boxed())?;
loop {
diff --git a/polkadot/node/core/pvf/Cargo.toml b/polkadot/node/core/pvf/Cargo.toml
index 430f7cd5e8e..3e72ca9e532 100644
--- a/polkadot/node/core/pvf/Cargo.toml
+++ b/polkadot/node/core/pvf/Cargo.toml
@@ -27,6 +27,7 @@ polkadot-core-primitives = { path = "../../../core-primitives" }
polkadot-node-core-pvf-common = { path = "common" }
polkadot-node-metrics = { path = "../../metrics" }
polkadot-node-primitives = { path = "../../primitives" }
+polkadot-node-subsystem = { path = "../../subsystem" }
polkadot-primitives = { path = "../../../primitives" }
sp-core = { path = "../../../../substrate/primitives/core" }
diff --git a/polkadot/node/core/pvf/benches/host_prepare_rococo_runtime.rs b/polkadot/node/core/pvf/benches/host_prepare_rococo_runtime.rs
index acd80526262..d0cefae6cdb 100644
--- a/polkadot/node/core/pvf/benches/host_prepare_rococo_runtime.rs
+++ b/polkadot/node/core/pvf/benches/host_prepare_rococo_runtime.rs
@@ -47,7 +47,7 @@ impl TestHost {
execute_worker_path,
);
f(&mut config);
- let (host, task) = start(config, Metrics::default()).await;
+ let (host, task) = start(config, Metrics::default()).await.unwrap();
let _ = handle.spawn(task);
Self { host: Mutex::new(host) }
}
diff --git a/polkadot/node/core/pvf/common/src/worker/mod.rs b/polkadot/node/core/pvf/common/src/worker/mod.rs
index 274a2fc8039..f6a67b98321 100644
--- a/polkadot/node/core/pvf/common/src/worker/mod.rs
+++ b/polkadot/node/core/pvf/common/src/worker/mod.rs
@@ -92,13 +92,13 @@ macro_rules! decl_worker_main {
std::process::exit(status)
},
"--check-can-unshare-user-namespace-and-change-root" => {
+ #[cfg(target_os = "linux")]
+ let cache_path_tempdir = std::path::Path::new(&args[2]);
#[cfg(target_os = "linux")]
let status = if let Err(err) = security::unshare_user_namespace_and_change_root(
$crate::worker::WorkerKind::CheckPivotRoot,
worker_pid,
- // We're not accessing any files, so we can try to pivot_root in the temp
- // dir without conflicts with other processes.
- &std::env::temp_dir(),
+ &cache_path_tempdir,
) {
// Write the error to stderr, log it on the host-side.
eprintln!("{}", err);
diff --git a/polkadot/node/core/pvf/src/host.rs b/polkadot/node/core/pvf/src/host.rs
index 7b383e8034a..5919b9ba32c 100644
--- a/polkadot/node/core/pvf/src/host.rs
+++ b/polkadot/node/core/pvf/src/host.rs
@@ -35,6 +35,7 @@ use polkadot_node_core_pvf_common::{
error::{PrepareError, PrepareResult},
pvf::PvfPrepData,
};
+use polkadot_node_subsystem::SubsystemResult;
use polkadot_parachain_primitives::primitives::ValidationResult;
use std::{
collections::HashMap,
@@ -203,7 +204,10 @@ impl Config {
/// The future should not return normally but if it does then that indicates an unrecoverable error.
/// In that case all pending requests will be canceled, dropping the result senders and new ones
/// will be rejected.
-pub async fn start(config: Config, metrics: Metrics) -> (ValidationHost, impl Future<Output = ()>) {
+pub async fn start(
+ config: Config,
+ metrics: Metrics,
+) -> SubsystemResult<(ValidationHost, impl Future<Output = ()>)> {
gum::debug!(target: LOG_TARGET, ?config, "starting PVF validation host");
// Run checks for supported security features once per host startup. Warn here if not enabled.
@@ -273,7 +277,7 @@ pub async fn start(config: Config, metrics: Metrics) -> (ValidationHost, impl Fu
};
};
- (validation_host, task)
+ Ok((validation_host, task))
}
/// A mapping from an artifact ID which is in preparation state to the list of pending execution
diff --git a/polkadot/node/core/pvf/src/security.rs b/polkadot/node/core/pvf/src/security.rs
index 295dd7df94d..0c0c5f40166 100644
--- a/polkadot/node/core/pvf/src/security.rs
+++ b/polkadot/node/core/pvf/src/security.rs
@@ -27,14 +27,19 @@ const SECURE_MODE_ANNOUNCEMENT: &'static str =
\nMore information: https://wiki.polkadot.network/docs/maintain-guides-secure-validator#secure-validator-mode";
/// Run checks for supported security features.
+///
+/// # Return
+///
+/// Returns the set of security features that we were able to enable. If an error occurs while
+/// enabling a security feature we set the corresponding status to `false`.
pub async fn check_security_status(config: &Config) -> SecurityStatus {
- let Config { prepare_worker_program_path, .. } = config;
+ let Config { prepare_worker_program_path, cache_path, .. } = config;
// TODO: add check that syslog is available and that seccomp violations are logged?
let (landlock, seccomp, change_root) = join!(
check_landlock(prepare_worker_program_path),
check_seccomp(prepare_worker_program_path),
- check_can_unshare_user_namespace_and_change_root(prepare_worker_program_path)
+ check_can_unshare_user_namespace_and_change_root(prepare_worker_program_path, cache_path)
);
let security_status = SecurityStatus {
@@ -149,11 +154,22 @@ fn print_secure_mode_message(errs: Vec<SecureModeError>) -> bool {
async fn check_can_unshare_user_namespace_and_change_root(
#[cfg_attr(not(target_os = "linux"), allow(unused_variables))]
prepare_worker_program_path: &Path,
+ #[cfg_attr(not(target_os = "linux"), allow(unused_variables))] cache_path: &Path,
) -> SecureModeResult {
cfg_if::cfg_if! {
if #[cfg(target_os = "linux")] {
+ let cache_dir_tempdir =
+ crate::worker_intf::tmppath_in("check-can-unshare", cache_path)
+ .await
+ .map_err(
+ |err|
+ SecureModeError::CannotUnshareUserNamespaceAndChangeRoot(
+ format!("could not create a temporary directory in {:?}: {}", cache_path, err)
+ )
+ )?;
match tokio::process::Command::new(prepare_worker_program_path)
.arg("--check-can-unshare-user-namespace-and-change-root")
+ .arg(cache_dir_tempdir)
.output()
.await
{
diff --git a/polkadot/node/core/pvf/tests/it/main.rs b/polkadot/node/core/pvf/tests/it/main.rs
index f4fd7f802f5..801b60884fa 100644
--- a/polkadot/node/core/pvf/tests/it/main.rs
+++ b/polkadot/node/core/pvf/tests/it/main.rs
@@ -61,7 +61,7 @@ impl TestHost {
execute_worker_path,
);
f(&mut config);
- let (host, task) = start(config, Metrics::default()).await;
+ let (host, task) = start(config, Metrics::default()).await.unwrap();
let _ = tokio::task::spawn(task);
Self { cache_dir, host: Mutex::new(host) }
}
--
GitLab