Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

PVF: Move landlock out of thread into process; add landlock exceptions #7580

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions node/core/pvf/common/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ sp-tracing = { git = "https://github.com/paritytech/substrate", branch = "master

[target.'cfg(target_os = "linux")'.dependencies]
landlock = "0.2.0"
rand = "0.8.5"

[dev-dependencies]
assert_matches = "1.4.0"
Expand Down
2 changes: 2 additions & 0 deletions node/core/pvf/common/src/execute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ use std::time::Duration;
pub struct Handshake {
/// The executor parameters.
pub executor_params: ExecutorParams,
/// Whether the host has determined that landlock is enabled.
pub landlock_enabled: bool,
}

/// The response from an execution job on the worker.
Expand Down
8 changes: 8 additions & 0 deletions node/core/pvf/common/src/prepare.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,11 @@ pub enum PrepareJobKind {
/// A prechecking job.
Prechecking,
}

/// The payload of the one-time handshake that is done when a worker process is created. Carries
/// data from the host to the worker.
#[derive(Encode, Decode)]
pub struct Handshake {
/// Whether the host has determined that landlock is enabled.
pub landlock_enabled: bool,
}
119 changes: 114 additions & 5 deletions node/core/pvf/common/src/worker/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use cpu_time::ProcessTime;
use futures::never::Never;
use std::{
any::Any,
path::PathBuf,
path::{Path, PathBuf},
sync::mpsc::{Receiver, RecvTimeoutError},
time::Duration,
};
Expand Down Expand Up @@ -70,17 +70,23 @@ macro_rules! decl_worker_main {
}

let mut node_version = None;
let mut socket_path: &str = "";
let mut socket_path = None;
let mut cache_path = None;

for i in (2..args.len()).step_by(2) {
match args[i].as_ref() {
"--socket-path" => socket_path = args[i + 1].as_str(),
"--socket-path" => socket_path = Some(args[i + 1].as_str()),
"--node-impl-version" => node_version = Some(args[i + 1].as_str()),
"--cache-path" => cache_path = Some(args[i + 1].as_str()),
arg => panic!("Unexpected argument found: {}", arg),
}
}
let socket_path = socket_path.expect("the --socket-path argument is required");
let cache_path = cache_path.expect("the --cache-path argument is required");

$entrypoint(&socket_path, node_version, Some($worker_version));
let cache_path = &std::path::Path::new(cache_path);

$entrypoint(&socket_path, node_version, Some($worker_version), cache_path);
}
};
}
Expand All @@ -102,6 +108,7 @@ pub fn worker_event_loop<F, Fut>(
socket_path: &str,
node_version: Option<&str>,
worker_version: Option<&str>,
cache_path: &Path,
mut event_loop: F,
) where
F: FnMut(UnixStream) -> Fut,
Expand All @@ -115,6 +122,7 @@ pub fn worker_event_loop<F, Fut>(
if node_version != worker_version {
gum::error!(
target: LOG_TARGET,
%debug_id,
%worker_pid,
%node_version,
%worker_version,
Expand All @@ -127,8 +135,28 @@ pub fn worker_event_loop<F, Fut>(
}
}

#[cfg(target_os = "linux")]
{
if let Err(err_ctx) = change_root(cache_path) {
let err = io::Error::last_os_error();
gum::error!(
target: LOG_TARGET,
%debug_id,
%worker_pid,
%err_ctx,
?cache_path,
"Could not change root to be the cache path: {}",
err
);
worker_shutdown_message(debug_id, worker_pid, err);
return
}
}

remove_env_vars(debug_id);

gum::info!(target: LOG_TARGET, "5. {:?}", std::fs::read_dir(".").unwrap().map(|entry| entry.unwrap().path()).collect::<Vec<PathBuf>>());

// Run the main worker loop.
let rt = Runtime::new().expect("Creates tokio runtime. If this panics the worker will die and the host will detect that and deal with it.");
let err = rt
Expand All @@ -151,6 +179,87 @@ pub fn worker_event_loop<F, Fut>(
rt.shutdown_background();
}

/// Change root to be the artifact directory.
#[cfg(target_os = "linux")]
fn change_root(cache_path: &Path) -> Result<(), &'static str> {
use rand::{distributions::Alphanumeric, Rng};
use std::{ffi::CString, os::unix::ffi::OsStrExt, ptr};

const RANDOM_LEN: usize = 10;
let mut buf = Vec::with_capacity(RANDOM_LEN);
buf.extend(rand::thread_rng().sample_iter(&Alphanumeric).take(RANDOM_LEN));
Comment on lines +189 to +190
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to do a .collect here directly into the buffer.

let s = std::str::from_utf8(&buf)
.expect("the string is collected from a valid utf-8 sequence; qed");

let cache_path_str = match cache_path.to_str() {
Some(s) => s,
None => return Err("cache path is not valid UTF-8")
};
mrcnski marked this conversation as resolved.
Show resolved Hide resolved
let cache_path_c = CString::new(cache_path.as_os_str().as_bytes()).unwrap();
let root_absolute_c = CString::new("/").unwrap();
// Append a random string to prevent races and to avoid dealing with the dir already existing.
let oldroot_relative_c = CString::new(format!("{}/oldroot-{}", cache_path_str, s)).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use join or push instead of manually merging these with /.

let oldroot_absolute_c = CString::new(format!("/oldroot-{}", s)).unwrap();

// SAFETY: TODO
unsafe {
// 1. `unshare` the user and the mount namespaces.
if libc::unshare(libc::CLONE_NEWUSER) < 0 {
return Err("unshare user namespace")
}
if libc::unshare(libc::CLONE_NEWNS) < 0 {
return Err("unshare mount namespace")
}
Comment on lines +207 to +212
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. These are flags so this can be done in a single unshare call.
  2. Since this is also switching to a new user namespace this function should be renamed as it's not strictly only changing the root.
  3. On some distros this could actually fail (due to user namespaces being disabled). In this case we'd probably want to abort execution unless a flag is passed on the command-line?


// 2. `pivot_root` to the artifact directory.
gum::info!(target: LOG_TARGET, "1. {:?}", std::env::current_dir());
gum::info!(target: LOG_TARGET, "1.5. {:?}", std::fs::read_dir(".").unwrap().map(|entry| entry.unwrap().path()).collect::<Vec<PathBuf>>());
// TODO: Ensure that 'new_root' and its parent mount don't have shared propagation.
if libc::mount(ptr::null(), root_absolute_c.as_ptr(), ptr::null(), libc::MS_REC | libc::MS_PRIVATE, ptr::null()) < 0 {
return Err("mount MS_PRIVATE")
}
if libc::mount(
cache_path_c.as_ptr(),
cache_path_c.as_ptr(),
ptr::null(), // ignored when MS_BIND is used
libc::MS_BIND | libc::MS_REC | libc::MS_NOEXEC,
ptr::null(), // ignored when MS_BIND is used
) < 0
{
return Err("mount MS_BIND")
}
if libc::mkdir(oldroot_relative_c.as_ptr(), 0755) < 0 {
return Err("mkdir oldroot")
}
if libc::syscall(
libc::SYS_pivot_root,
cache_path_c.as_ptr(),
oldroot_relative_c.as_ptr(),
) < 0
{
return Err("pivot_root")
}
Comment on lines +231 to +241
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you call the pivot root syscall with both paths being "." then this mkdir should be unnecessary.


// 3. Change to the new root, `unmount2` and remove the old root.
if libc::chdir(root_absolute_c.as_ptr()) < 0 {
return Err("chdir to new root")
}
gum::info!(target: LOG_TARGET, "2. {:?}", std::env::current_dir());
gum::info!(target: LOG_TARGET, "3. {:?}", std::fs::read_dir(".").unwrap().map(|entry| entry.unwrap().path()).collect::<Vec<PathBuf>>());
if libc::umount2(oldroot_absolute_c.as_ptr(), libc::MNT_DETACH) < 0 {
return Err("umount2 the oldroot")
}
if libc::rmdir(oldroot_absolute_c.as_ptr()) < 0 {
return Err("rmdir the oldroot")
}
gum::info!(target: LOG_TARGET, "4. {:?}", std::fs::read_dir(".").unwrap().map(|entry| entry.unwrap().path()).collect::<Vec<PathBuf>>());

// TODO: do some assertions
}

Ok(())
}

/// Delete all env vars to prevent malicious code from accessing them.
fn remove_env_vars(debug_id: &'static str) {
for (key, value) in std::env::vars_os() {
Expand Down Expand Up @@ -299,7 +408,7 @@ pub mod thread {
Arc::new((Mutex::new(WaitOutcome::Pending), Condvar::new()))
}

/// Runs a worker thread. Will first enable security features, and afterwards notify the threads
/// Runs a worker thread. Will run the requested function, and afterwards notify the threads
/// waiting on the condvar. Catches panics during execution and resumes the panics after
/// triggering the condvar, so that the waiting thread is notified on panics.
///
Expand Down
Loading
Loading