kernel/task: Move ELF parsing into context of the new task

When loading a user ELF binary for execution the COCONUT kernel created a new
Task object and mapped the ELF sections into the new tasks user-VMR. This all
happened from the context of the old task.

This behavior is problematic as it creates unnecessary complexity for cases
when the kernel needs to write values into the user address space. Fix this
behavior by moving the actual ELF loading into the context of the new task.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>

Author: joergroedel

kernel/src/cpu/idt/svsm_entry.S

@@ -378,6 +378,11 @@ thread_entry_asm:
 	movq %r15, %rsi
 	movq %r14, %rdi
 
+	// Move pointer to X86ExceptionContext to %rdx - Only present and used
+	// for user-tasks
+	movq %rsp, %rdx
+	addq $16, %rdx
+
 	// Fetch thread entry function pointer - do not touch stack pointer to
 	// keep stack alignment
 	movq (%rsp), %rax

kernel/src/task/exec.rs

@@ -10,16 +10,30 @@ use super::TaskPointer;
 use crate::address::{Address, VirtAddr};
 use crate::error::SvsmError;
 use crate::fs::{Directory, open_read};
-use crate::mm::USER_MEM_END;
 use crate::mm::vm::VMFileMappingFlags;
+use crate::mm::{USER_MEM_END, mmap_user};
 use crate::task::{create_user_task, current_task, finish_user_task, schedule};
 use crate::types::PAGE_SIZE;
 use crate::utils::align_up;
+use alloc::boxed::Box;
 use alloc::sync::Arc;
 use elf::{Elf64File, Elf64PhdrFlags};
 
 use alloc::string::String;
 
+#[derive(Debug)]
+pub struct UserExecInfo {
+    binary: String,
+}
+
+impl UserExecInfo {
+    pub fn new(b: &str) -> Self {
+        Self {
+            binary: String::from(b),
+        }
+    }
+}
+
 fn convert_elf_phdr_flags(flags: Elf64PhdrFlags) -> VMFileMappingFlags {
     let mut vm_flags = VMFileMappingFlags::Fixed;
 
@@ -44,27 +58,20 @@ fn task_name(binary: &str) -> String {
     }
 }
 
-/// Loads and executes an ELF binary in user-mode.
-///
-/// # Arguments
-///
-/// * binary: Path to file in the file-system
-///
-/// # Returns
-///
-/// [`Ok(TaskPointer)`] on success, [`Err(SvsmError)`] on failure.
-pub fn exec_user(binary: &str, root: Arc<dyn Directory>) -> Result<TaskPointer, SvsmError> {
-    let fh = open_read(binary)?;
+pub fn exec(info: UserExecInfo) -> Result<u64, SvsmError> {
+    let fh = open_read(&info.binary)?;
     let file_size = fh.size();
 
     let current_task = current_task();
+
     let vstart = current_task.mmap_kernel_guard(
         VirtAddr::new(0),
         Some(&fh),
         0,
         file_size,
         VMFileMappingFlags::Read,
     )?;
+
     // SAFETY: `vstart` has just been mapped using `file_size` as the size,
     // so it is safe to create a slice of the same size.
     let buf = unsafe { vstart.to_slice::<u8>(file_size) };
@@ -74,8 +81,6 @@ pub fn exec_user(binary: &str, root: Arc<dyn Directory>) -> Result<TaskPointer,
     let virt_base = alloc_info.range.vaddr_begin;
     let entry = elf_bin.get_entry(virt_base);
 
-    let new_task = create_user_task(entry.try_into().unwrap(), root, task_name(binary))?;
-
     for seg in elf_bin.image_load_segment_iter(virt_base) {
         let virt_start = VirtAddr::from(seg.vaddr_range.vaddr_begin);
         let virt_end = VirtAddr::from(seg.vaddr_range.vaddr_end).align_up(PAGE_SIZE);
@@ -93,16 +98,16 @@ pub fn exec_user(binary: &str, root: Arc<dyn Directory>) -> Result<TaskPointer,
             let start_aligned = virt_start.page_align();
             let offset = file_offset - virt_start.page_offset();
             let size = file_size + virt_start.page_offset();
-            new_task.mmap_user(start_aligned, Some(&fh), offset, size, flags)?;
+            mmap_user(start_aligned, Some(&fh), offset, size, flags)?;
 
             let size_aligned = align_up(size, PAGE_SIZE);
             if size_aligned < len {
                 let start_anon = start_aligned.const_add(size_aligned);
                 let remaining_len = len - size_aligned;
-                new_task.mmap_user(start_anon, None, 0, remaining_len, flags)?;
+                mmap_user(start_anon, None, 0, remaining_len, flags)?;
             }
         } else {
-            new_task.mmap_user(virt_start, None, 0, len, flags)?;
+            mmap_user(virt_start, None, 0, len, flags)?;
         }
     }
 
@@ -113,7 +118,23 @@ pub fn exec_user(binary: &str, root: Arc<dyn Directory>) -> Result<TaskPointer,
     let user_stack_size: usize = 64 * 1024;
     let stack_flags: VMFileMappingFlags = VMFileMappingFlags::Fixed | VMFileMappingFlags::Write;
     let stack_addr = USER_MEM_END - user_stack_size;
-    new_task.mmap_user(stack_addr, None, 0, user_stack_size, stack_flags)?;
+    mmap_user(stack_addr, None, 0, user_stack_size, stack_flags)?;
+
+    Ok(entry)
+}
+
+/// Loads and executes an ELF binary in user-mode.
+///
+/// # Arguments
+///
+/// * binary: Path to file in the file-system
+///
+/// # Returns
+///
+/// [`Ok(TaskPointer)`] on success, [`Err(SvsmError)`] on failure.
+pub fn exec_user(binary: &str, root: Arc<dyn Directory>) -> Result<TaskPointer, SvsmError> {
+    let info = Box::new(UserExecInfo::new(binary));
+    let new_task = create_user_task(info, root, task_name(binary))?;
 
     finish_user_task(new_task.clone());
     schedule();

kernel/src/task/mod.rs

@@ -21,5 +21,5 @@ pub use tasks::{
     TaskListAdapter, TaskPointer, TaskRunListAdapter, TaskState, is_task_fault,
 };
 
-pub use exec::exec_user;
+pub use exec::{UserExecInfo, exec_user};
 pub use waiting::WaitQueue;

kernel/src/task/schedule.rs

@@ -38,6 +38,7 @@ use super::tasks::TASK_ACTIVE_OFFSET;
 use super::tasks::TASK_CUR_CPU_OFFSET;
 use super::{
     INITIAL_TASK_ID, KernelThreadStartInfo, Task, TaskListAdapter, TaskPointer, TaskRunListAdapter,
+    UserExecInfo,
 };
 use crate::address::{Address, VirtAddr};
 use crate::cpu::IrqGuard;
@@ -59,6 +60,7 @@ use crate::fs::Directory;
 use crate::locking::SpinLock;
 use crate::mm::SVSM_CONTEXT_SWITCH_SHADOW_STACK;
 use crate::platform::SVSM_PLATFORM;
+use alloc::boxed::Box;
 use alloc::string::String;
 use alloc::sync::Arc;
 use core::arch::global_asm;
@@ -365,12 +367,12 @@ pub fn start_kernel_thread(start_info: KernelThreadStartInfo) -> Result<TaskPoin
 ///
 /// A new instance of [`TaskPointer`] on success, [`SvsmError`] on failure.
 pub fn create_user_task(
-    user_entry: usize,
+    info: Box<UserExecInfo>,
     root: Arc<dyn Directory>,
     name: String,
 ) -> Result<TaskPointer, SvsmError> {
     let cpu = this_cpu();
-    Task::create_user(cpu, user_entry, root, name)
+    Task::create_user(cpu, info, root, name)
 }
 
 /// Finished user-space task creation by putting the task on the global

kernel/src/task/tasks.rs

@@ -6,6 +6,7 @@
 
 extern crate alloc;
 
+use alloc::boxed::Box;
 use alloc::collections::btree_map::BTreeMap;
 use alloc::string::String;
 use alloc::sync::Arc;
@@ -50,10 +51,11 @@ use crate::types::{SVSM_USER_CS, SVSM_USER_DS};
 use crate::utils::{MemoryRegion, is_aligned};
 use intrusive_collections::{LinkedListAtomicLink, intrusive_adapter};
 
-use super::WaitQueue;
+use super::exec::exec;
 use super::schedule::complete_task_switch;
 use super::schedule::terminate;
 use super::task_mm::{TaskKernelMapping, TaskMM};
+use super::{UserExecInfo, WaitQueue};
 
 pub const INITIAL_TASK_ID: u32 = 1;
 
@@ -129,7 +131,7 @@ impl KernelThreadStartInfo {
 #[derive(Debug)]
 enum ThreadStartInfo {
     Kernel(KernelThreadStartInfo),
-    User(usize),
+    User(*const UserExecInfo),
 }
 
 #[derive(PartialEq, Debug, Copy, Clone, Default)]
@@ -491,7 +493,7 @@ impl Task {
 
         // Call the correct stack creation routine for this task.
         let (stack, raw_bounds, rsp_offset) = match args.start_info {
-            ThreadStartInfo::User(entry) => Self::allocate_utask_stack(cpu, entry, xsa_addr)?,
+            ThreadStartInfo::User(info_ptr) => Self::allocate_utask_stack(cpu, info_ptr, xsa_addr)?,
             ThreadStartInfo::Kernel(ref info) => Self::allocate_ktask_stack(
                 cpu,
                 info.entry,
@@ -552,7 +554,7 @@ impl Task {
 
     pub fn create_user(
         cpu: &PerCpu,
-        user_entry: usize,
+        info: Box<UserExecInfo>,
         root: Arc<dyn Directory>,
         name: String,
     ) -> Result<TaskPointer, SvsmError> {
@@ -562,14 +564,23 @@ impl Task {
         unsafe {
             vm_user_range.initialize_lazy()?;
         }
+
+        // Destroy the Box and get the pointer to the raw data
+        let info_ptr = Box::into_raw(info);
+
         let create_args = CreateTaskArguments {
-            start_info: ThreadStartInfo::User(user_entry),
+            start_info: ThreadStartInfo::User(info_ptr),
             name,
             vm_user_range: Some(vm_user_range),
             rootdir: root,
             thread_of: None,
         };
-        Self::create_common(cpu, create_args)
+        Self::create_common(cpu, create_args).inspect_err(|_| {
+            // In error case, make sure info gets freed
+            // SAFETY: info_ptr was created above from a Box and not
+            // dropped in the create_common call-path.
+            drop(unsafe { Box::from_raw(info_ptr) });
+        })
     }
 
     /// Create a new thread for an existing task.
@@ -737,20 +748,14 @@ impl Task {
 
     fn allocate_utask_stack(
         cpu: &PerCpu,
-        user_entry: usize,
+        info_ptr: *const UserExecInfo,
         xsa_addr: usize,
     ) -> Result<(Mapping, MemoryRegion<VirtAddr>, usize), SvsmError> {
-        // Do not run user-mode with IRQs enabled on platforms which are not
-        // ready for it.
-        let iret_rflags: usize = if SVSM_PLATFORM.use_interrupts() {
-            2 | EFLAGS_IF
-        } else {
-            2
-        };
         let task_context = UserTaskContext {
             task_context: TaskContext {
                 regs: X86TaskSwitchRegs {
-                    rsi: xsa_addr, // XSAVE area addr
+                    rsi: xsa_addr,          // XSAVE area addr
+                    r14: info_ptr as usize, // New task launch info
                     ..Default::default()
                 },
                 ret_addr: thread_entry_asm as *const () as u64,
@@ -762,20 +767,9 @@ impl Task {
                 .unwrap(),
             // Setup IRQ return frame.  User-mode tasks always run with
             // interrupts enabled.
-            excp_context: X86ExceptionContext {
-                frame: X86InterruptFrame {
-                    rip: user_entry,
-                    cs: (SVSM_USER_CS | 3).into(),
-                    flags: iret_rflags,
-                    rsp: (USER_MEM_END - 8).into(),
-                    ss: (SVSM_USER_DS | 3).into(),
-                },
-                ..Default::default()
-            },
+            excp_context: X86ExceptionContext::default(),
         };
 
-        debug_assert!(is_aligned(task_context.excp_context.frame.rsp + 8, 16));
-
         Self::allocate_stack_common(cpu, &task_context)
     }
 
@@ -1038,7 +1032,41 @@ unsafe fn complete_new_thread(prev: usize, xsa_addr: u64) {
 /// This is called from assembly code. The caller needs to ensure the
 /// parameters are passed correctly.
 #[unsafe(no_mangle)]
-unsafe fn run_user_task() {
+unsafe fn run_user_task(
+    info_ptr: *mut UserExecInfo,
+    _unsused: u64,
+    ctxt: &mut X86ExceptionContext,
+) {
+    // SAFETY: The raw pointer was extracted from a Box in
+    // allocate_utask_stack() to move ownership to a new process.
+    let info = unsafe { Box::from_raw(info_ptr) };
+
+    let entry = match exec(*info) {
+        Ok(e) => e,
+        Err(e) => {
+            log::error!("Failed to load ELF binary: {e:?}");
+            terminate();
+        }
+    };
+
+    // Do not run user-mode with IRQs enabled on platforms which are not
+    // ready for it.
+    let iret_rflags: usize = if SVSM_PLATFORM.use_interrupts() {
+        2 | EFLAGS_IF
+    } else {
+        2
+    };
+
+    ctxt.frame = X86InterruptFrame {
+        rip: entry as usize,
+        cs: (SVSM_USER_CS | 3).into(),
+        flags: iret_rflags,
+        rsp: (USER_MEM_END - 8).into(),
+        ss: (SVSM_USER_DS | 3).into(),
+    };
+
+    debug_assert!(is_aligned(ctxt.frame.rsp + 8, 16));
+
     task_attach_console();
 }