From b00456dbb9ec33be924778afc2e04e3d732fc212 Mon Sep 17 00:00:00 2001 From: jD91mZM2 Date: Wed, 15 Jul 2020 13:04:33 +0200 Subject: [PATCH] WIP: Continue trying to fix clone_ret --- src/arch/x86_64/interrupt/handler.rs | 76 ++++++++++++++++------------ src/arch/x86_64/interrupt/irq.rs | 4 +- src/arch/x86_64/interrupt/syscall.rs | 66 ++++++++++-------------- src/syscall/process.rs | 12 ++--- 4 files changed, 80 insertions(+), 78 deletions(-) diff --git a/src/arch/x86_64/interrupt/handler.rs b/src/arch/x86_64/interrupt/handler.rs index 70f3b13..070454b 100644 --- a/src/arch/x86_64/interrupt/handler.rs +++ b/src/arch/x86_64/interrupt/handler.rs @@ -233,33 +233,6 @@ macro_rules! push_scratch { push r11 " }; } -#[macro_export] -macro_rules! push_preserved { - () => { " - // Push preserved registers - push rbx - push rbp - push r12 - push r13 - push r14 - push r15 - " }; -} -#[macro_export] -macro_rules! push_fs { - () => { " - // Push fs - push fs - - // Load kernel tls - // We can't load the value directly into `fs`. We also can't use `rax` - // as the temporary value, as during errors that's already used for the - // error code. - mov rbx, 0x18 - mov fs, bx - " }; -} - #[macro_export] macro_rules! pop_scratch { () => { " @@ -275,6 +248,19 @@ macro_rules! pop_scratch { pop rax " }; } + +#[macro_export] +macro_rules! push_preserved { + () => { " + // Push preserved registers + push rbx + push rbp + push r12 + push r13 + push r14 + push r15 + " }; +} #[macro_export] macro_rules! pop_preserved { () => { " @@ -287,6 +273,21 @@ macro_rules! pop_preserved { pop rbx " }; } + +#[macro_export] +macro_rules! push_fs { + () => { " + // Push fs + push fs + + // Load kernel tls + // We can't load the value directly into `fs`. We also can't use `rax` + // as the temporary value, as during errors that's already used for the + // error code. + mov rbx, 0x18 + mov fs, bx + " }; +} #[macro_export] macro_rules! pop_fs { () => { " @@ -300,8 +301,15 @@ macro_rules! interrupt_stack { ($name:ident, |$stack:ident| $code:block) => { paste::item! { #[no_mangle] - unsafe extern "C" fn [<__interrupt_ $name>]($stack: &mut $crate::arch::x86_64::interrupt::InterruptStack) { - $code + unsafe extern "C" fn [<__interrupt_ $name>](stack: *mut $crate::arch::x86_64::interrupt::InterruptStack) { + // This inner function is needed because macros are buggy: + // https://github.com/dtolnay/paste/issues/7 + #[inline(always)] + unsafe fn inner($stack: &mut $crate::arch::x86_64::interrupt::InterruptStack) { + $code + } + let _guard = $crate::ptrace::set_process_regs(stack); + inner(&mut *stack); } function!($name => { @@ -371,8 +379,14 @@ macro_rules! interrupt_error { ($name:ident, |$stack:ident| $code:block) => { paste::item! { #[no_mangle] - unsafe extern "C" fn [<__interrupt_ $name>]($stack: &mut $crate::arch::x86_64::interrupt::handler::InterruptErrorStack) { - $code + unsafe extern "C" fn [<__interrupt_ $name>](stack: *mut $crate::arch::x86_64::interrupt::handler::InterruptErrorStack) { + // This inner function is needed because macros are buggy: + // https://github.com/dtolnay/paste/issues/7 + unsafe fn inner($stack: &mut $crate::arch::x86_64::interrupt::handler::InterruptErrorStack) { + $code + } + let _guard = $crate::ptrace::set_process_regs(&mut (*stack).inner); + inner(&mut *stack); } function!($name => { diff --git a/src/arch/x86_64/interrupt/irq.rs b/src/arch/x86_64/interrupt/irq.rs index cdac53d..a1e2f20 100644 --- a/src/arch/x86_64/interrupt/irq.rs +++ b/src/arch/x86_64/interrupt/irq.rs @@ -8,7 +8,7 @@ use crate::device::{local_apic, ioapic, pic}; use crate::device::serial::{COM1, COM2}; use crate::ipi::{ipi, IpiKind, IpiTarget}; use crate::scheme::debug::debug_input; -use crate::{context, ptrace, time}; +use crate::{context, time}; //resets to 0 in context::switch() #[thread_local] @@ -169,7 +169,7 @@ unsafe fn ioapic_unmask(irq: usize) { ioapic::unmask(irq as u8); } -interrupt_stack!(pit_stack, |stack| { +interrupt_stack!(pit_stack, |_stack| { // Saves CPU time by not sending IRQ event irq_trigger(0); const PIT_RATE: u64 = 2_250_286; diff --git a/src/arch/x86_64/interrupt/syscall.rs b/src/arch/x86_64/interrupt/syscall.rs index 4ec6b9d..fc08614 100644 --- a/src/arch/x86_64/interrupt/syscall.rs +++ b/src/arch/x86_64/interrupt/syscall.rs @@ -15,34 +15,17 @@ pub unsafe fn init() { } macro_rules! with_interrupt_stack { - ($name:ident |$stack:ident| $code:block) => {{ - let _guard = ptrace::set_process_regs($stack); - + (|$stack:ident| $code:block) => {{ let allowed = ptrace::breakpoint_callback(PTRACE_STOP_PRE_SYSCALL, None) .and_then(|_| ptrace::next_breakpoint().map(|f| !f.contains(PTRACE_FLAG_IGNORE))); if allowed.unwrap_or(true) { - paste::expr! { - #[no_mangle] - unsafe extern "C" fn [<__inner_stack_ $name>]($stack: &mut InterruptStack) -> usize { - $code - } - - // A normal function call like inner(stack) is sadly not guaranteed - // to map to a `call` instruction - it can also be a `jmp`. We want - // this to definitely be its own call stack, to make `clone_ret` - // work as expected. - let ret; - asm!( - concat!("call __inner_stack_", stringify!($name)) - : "={rax}"(ret) - : "{rdi}"(&mut *$stack) - : /* no clobbers */ - : "volatile", "intel" - ); - - (*$stack).scratch.rax = ret; - } + // If the syscall is `clone`, the clone won't return here. Instead, + // it'll return early and leave any undropped values. This is + // actually GOOD, because any references are at that point UB + // anyway, because they are based on the wrong stack. + let $stack = &mut *$stack; + (*$stack).scratch.rax = $code; } ptrace::breakpoint_callback(PTRACE_STOP_POST_SYSCALL, None); @@ -51,7 +34,9 @@ macro_rules! with_interrupt_stack { #[no_mangle] pub unsafe extern "C" fn __inner_syscall_instruction(stack: *mut InterruptStack) { - with_interrupt_stack!(syscall_instruction |stack| { + let _guard = ptrace::set_process_regs(stack); + with_interrupt_stack!(|stack| { + // Set a restore point for clone let rbp; asm!("" : "={rbp}"(rbp) : : : "intel", "volatile"); @@ -67,8 +52,8 @@ function!(syscall_instruction => { mov gs:[28], rsp // Save userspace rsp mov rsp, gs:[4] // Load kernel rsp push 5 * 8 + 3 // Push userspace data segment - push qword ptr gs:[28] // Push userspace rsp - mov qword ptr gs:[28], 0 // Clear userspace rsp + push QWORD PTR gs:[28] // Push userspace rsp + mov QWORD PTR gs:[28], 0 // Clear userspace rsp push r11 // Push rflags push 4 * 8 + 3 // Push userspace code segment push rcx // Push userspace return pointer @@ -101,7 +86,8 @@ function!(syscall_instruction => { }); interrupt_stack!(syscall, |stack| { - with_interrupt_stack!(syscall |stack| { + with_interrupt_stack!(|stack| { + // Set a restore point for clone let rbp; asm!("" : "={rbp}"(rbp) : : : "intel", "volatile"); @@ -111,16 +97,18 @@ interrupt_stack!(syscall, |stack| { }); function!(clone_ret => { - // The C x86_64 ABI specifies that rbp is pushed to save the old call frame. - // Popping rbp means we're using the parent's call frame and thus will not - // only return from this function but also from the function above this one. - - // When this is called, the stack should have been - // interrupt->inner->syscall->clone->clone_ret - // then popped to - // interrupt->inner->syscall - // so this will return from "syscall". - "pop rbx\n", - "xor rax, rax\n", + // The address of this instruction is injected by `clone` in process.rs, on + // top of the stack syscall->inner in this file, which is done using the rbp + // register we save there. + // + // The top of our stack here is the address pointed to by rbp, which is: + // + // - the previous rbp + // - the return location + // + // Our goal is to return from the parent function, inner, so we restore + // rbp... + "pop rbp\n", + // ...and we return to the address at the top of the stack "ret\n", }); diff --git a/src/syscall/process.rs b/src/syscall/process.rs index b0f3f38..0dc381d 100644 --- a/src/syscall/process.rs +++ b/src/syscall/process.rs @@ -131,18 +131,18 @@ pub fn clone(flags: CloneFlags, stack_base: usize) -> Result { } if let Some(ref stack) = context.kstack { - // Get the relative offset to the return address of this function + // Get the relative offset to the return address of the function + // obtaining `stack_base`. + // // (base pointer - start of stack) - one offset = stack_base - stack.as_ptr() as usize - mem::size_of::(); // Add clone ret let mut new_stack = stack.clone(); unsafe { + // Set clone's return value to zero. This is done because + // the clone won't return like normal, which means the value + // would otherwise never get set. if let Some(regs) = ptrace::rebase_regs_ptr_mut(context.regs, Some(&mut new_stack)) { - // We'll need to tell the clone that it should return 0, - // but that's it. We don't actually clone the registers - // and put them on the child, because it will then - // instead become None and be exempt from all kinds of - // ptracing until the current syscall has completed. (*regs).scratch.rax = 0; }