From 42abf362e4348dfb35fac8b62a01294911b5e1c3 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Wed, 18 Dec 2024 10:19:53 -0800 Subject: [PATCH] kern: fix client-controlled kernel panic in kipc It turns out it's possible to trigger a bounds check in kipc handling code by submitting an invalid IRQ number. This fixes that, causing the bounds check to be optimized out. In several syscall paths (and one kipc) we manipulate interrupts without accepting an interrupt number from the client. In these cases, a bad interrupt number would indicate a problem in our dispatch table, so I've made them explicit panics. (They were panics before, just implicit ones in the array access.) This causes a small increase in kernel text size (40-64 bytes), but kipc should not be able to crash the kernel! --- sys/kern/src/arch/arm_m.rs | 48 +++++++++++++++++++++++++++----------- sys/kern/src/kipc.rs | 7 ++++-- sys/kern/src/syscalls.rs | 11 +++++---- 3 files changed, 47 insertions(+), 19 deletions(-) diff --git a/sys/kern/src/arch/arm_m.rs b/sys/kern/src/arch/arm_m.rs index 1168d4df0..aa2547ce2 100644 --- a/sys/kern/src/arch/arm_m.rs +++ b/sys/kern/src/arch/arm_m.rs @@ -83,7 +83,7 @@ use crate::time::Timestamp; use crate::umem::USlice; #[cfg(any(armv7m, armv8m))] use abi::FaultSource; -use abi::{FaultInfo, InterruptNum}; +use abi::{FaultInfo, InterruptNum, UsageError}; #[cfg(armv8m)] use armv8_m_mpu::{disable_mpu, enable_mpu}; use unwrap_lite::UnwrapLite; @@ -1233,7 +1233,10 @@ pub unsafe extern "C" fn DefaultHandler() { .unwrap_or_else(|| panic!("unhandled IRQ {irq_num}")); let switch = with_task_table(|tasks| { - disable_irq(irq_num, false); + // This can only fail if the IRQ number is out of range, which + // in this case would mean the hardware is conspiring against + // us. So ignore it to ensure we don't generate a bogus check. + disable_irq(irq_num, false).ok(); // Now, post the notification and return the // scheduling hint. @@ -1250,22 +1253,29 @@ pub unsafe extern "C" fn DefaultHandler() { crate::profiling::event_isr_exit(); } -pub fn disable_irq(n: u32, also_clear_pending: bool) { +pub fn disable_irq(n: u32, also_clear_pending: bool) -> Result<(), UsageError> { // Disable the interrupt by poking the Interrupt Clear Enable Register. let nvic = unsafe { &*cortex_m::peripheral::NVIC::PTR }; let reg_num = (n / 32) as usize; let bit_mask = 1 << (n % 32); unsafe { - nvic.icer[reg_num].write(bit_mask); + nvic.icer + .get(reg_num) + .ok_or(UsageError::NoIrq)? + .write(bit_mask); } if also_clear_pending { unsafe { - nvic.icpr[reg_num].write(bit_mask); + nvic.icpr + .get(reg_num) + .ok_or(UsageError::NoIrq)? + .write(bit_mask); } } + Ok(()) } -pub fn enable_irq(n: u32, also_clear_pending: bool) { +pub fn enable_irq(n: u32, also_clear_pending: bool) -> Result<(), UsageError> { // Enable the interrupt by poking the Interrupt Set Enable Register. let nvic = unsafe { &*cortex_m::peripheral::NVIC::PTR }; let reg_num = (n / 32) as usize; @@ -1273,17 +1283,24 @@ pub fn enable_irq(n: u32, also_clear_pending: bool) { if also_clear_pending { // Do this _before_ enabling. unsafe { - nvic.icpr[reg_num].write(bit_mask); + nvic.icpr + .get(reg_num) + .ok_or(UsageError::NoIrq)? + .write(bit_mask); } } unsafe { - nvic.iser[reg_num].write(bit_mask); + nvic.iser + .get(reg_num) + .ok_or(UsageError::NoIrq)? + .write(bit_mask); } + Ok(()) } /// Looks up an interrupt in the NVIC and returns a cross-platform /// representation of that interrupt's status. -pub fn irq_status(n: u32) -> abi::IrqStatus { +pub fn irq_status(n: u32) -> Result { let mut status = abi::IrqStatus::empty(); let nvic = unsafe { &*cortex_m::peripheral::NVIC::PTR }; @@ -1292,7 +1309,8 @@ pub fn irq_status(n: u32) -> abi::IrqStatus { // See if the interrupt is enabled by checking the bit in the Interrupt Set // Enable Register. - let enabled = nvic.iser[reg_num].read() & bit_mask == bit_mask; + let iser_reg = nvic.iser.get(reg_num).ok_or(UsageError::NoIrq)?; + let enabled = iser_reg.read() & bit_mask == bit_mask; status.set(abi::IrqStatus::ENABLED, enabled); // See if the interrupt is pending by checking the bit in the Interrupt @@ -1300,17 +1318,21 @@ pub fn irq_status(n: u32) -> abi::IrqStatus { let pending = nvic.ispr[reg_num].read() & bit_mask == bit_mask; status.set(abi::IrqStatus::PENDING, pending); - status + Ok(status) } -pub fn pend_software_irq(InterruptNum(n): InterruptNum) { +pub fn pend_software_irq( + InterruptNum(n): InterruptNum, +) -> Result<(), UsageError> { let nvic = unsafe { &*cortex_m::peripheral::NVIC::PTR }; let reg_num = (n / 32) as usize; let bit_mask = 1 << (n % 32); // Pend the IRQ by poking the corresponding bit in the Interrupt Set Pending // Register (ISPR). - unsafe { nvic.ispr[reg_num].write(bit_mask) }; + let ispr_reg = nvic.ispr.get(reg_num).ok_or(UsageError::NoIrq)?; + unsafe { ispr_reg.write(bit_mask) }; + Ok(()) } #[repr(u8)] diff --git a/sys/kern/src/kipc.rs b/sys/kern/src/kipc.rs index 2d6bac6cf..8073a9150 100644 --- a/sys/kern/src/kipc.rs +++ b/sys/kern/src/kipc.rs @@ -5,12 +5,13 @@ //! Implementation of IPC operations on the virtual kernel task. use abi::{FaultInfo, Kipcnum, SchedState, TaskState, UsageError}; +use core::mem::size_of; +use unwrap_lite::UnwrapLite; use crate::arch; use crate::err::UserError; use crate::task::{current_id, ArchState, NextTask, Task}; use crate::umem::USlice; -use core::mem::size_of; /// Message dispatcher. pub fn handle_kernel_message( @@ -462,7 +463,9 @@ fn software_irq( )))?; for &irq in irqs.iter() { - crate::arch::pend_software_irq(irq); + // Any error here would be a problem in our dispatch table, not the + // caller, so we panic because we want to hear about it. + crate::arch::pend_software_irq(irq).unwrap_lite(); } tasks[caller].save_mut().set_send_response_and_length(0, 0); diff --git a/sys/kern/src/syscalls.rs b/sys/kern/src/syscalls.rs index 10e4646ab..0e84579cc 100644 --- a/sys/kern/src/syscalls.rs +++ b/sys/kern/src/syscalls.rs @@ -783,7 +783,9 @@ fn irq_control( UsageError::NoIrq, )))?; for i in irqs.iter() { - operation(i.0, also_clear_pending); + // Because the IRQ numbers are coming from our own table, any error here + // would indicate a kernel bug, _not_ bad syscall arguments. + operation(i.0, also_clear_pending).unwrap_lite(); } Ok(NextTask::Same) } @@ -923,9 +925,10 @@ fn irq_status( )))?; // Combine the platform-level status of all the IRQs in the notification set. - let mut status = irqs.iter().fold(IrqStatus::empty(), |status, irq| { - status | crate::arch::irq_status(irq.0) - }); + let mut status = + irqs.iter().try_fold(IrqStatus::empty(), |status, irq| { + crate::arch::irq_status(irq.0).map(|n| status | n) + })?; // If any bits in the notification mask are set in the caller's notification // set, then a notification has been posted to the task and not yet consumed.