kern: fix client-controlled kernel panic in kipc #1961
Conversation
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!
cbiffle
force-pushed
the
cbiffle/kipc-panic
branch
from
0722a6b to
d858fef
Compare
| // 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(); |
There was a problem hiding this comment.
I know you prefer this over let _ because the latter is a bit of a footgun in other contexts, but there is a part of me that kind of wonders if
fn_returning_result().ok();optimizes worse than
let _ = fn_returning_result();because it's a function call, though hopefully LLVM can eliminate it...I might have to go check.
EDIT: After spending some time in Compiler Explorer, I have been proven definitively wrong about this, in fact, the opt-level=2 compiler is so smart about this that it correctly realizes that these are actually the same function and doesn't even generate two separate functions for them:
#[no_mangle]
#[inline(never)]
pub extern fn ignore_err_ok() {
returns_result().ok();
// pretend the function then does other stuff
core::hint::black_box(1);
}
#[no_mangle]
#[inline(never)]
pub extern fn ignore_err_underscore() {
let _ = returns_result();
// pretend the function then does other stuff
core::hint::black_box(1);
}So, I have seen the error of my ways and should not have doubted the use of .ok() to ignore Results!.
| irqs.iter().try_fold(IrqStatus::empty(), |status, irq| { | ||
| crate::arch::irq_status(irq.0).map(|n| status | n) | ||
| })?; |
There was a problem hiding this comment.
it's nice how this was so trivially converted to fallible with try_fold! :)
| pub fn pend_software_irq(InterruptNum(n): InterruptNum) { | ||
| pub fn pend_software_irq( | ||
| InterruptNum(n): InterruptNum, | ||
| ) -> Result<(), UsageError> { |
There was a problem hiding this comment.
I was about to say "hmm, shouldn't we update the documentation for the software_irq KIPC to reflect that it will fault you if you ask for a nonexistent IRQ...but then, I realized that the Hubris reference docs for KIPCs doesn't even mention software_irq, and instead suggests that find_faulted_task is KIPC 8, which it isn't (it's KIPC 9, and software_irq is 8). This is almost certainly my fault back when I added the software IRQ KIPC, so I should probably fix that...
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
if we wanted to be really fastidious about code size, we could potentially do a single check that the IRQ number is in range, and then make both of the actual accessess unchecked; if rustc isn't smart enough to know that if it's in range for ICER, it's also in range for ICPR? but, I dunno if it's worth the more manual checking to save 40 bytes of kernel --- feels sketchier for sure.
| // 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(); |
There was a problem hiding this comment.
Hmm --- this is the only call site of pend_software_irq, right? So, if we're going to unwrap here, we could just leave that function using infallible array indexing and let it panic, instead. I dunno if that has a meaningful impact on binary size, though, and this is maybe nicer in the event that we add other uses of pend_software_irq later, which don't want to panic. I think having it be the caller's decision whether or not to panic is almost certainly the ideologically nicer factoring...
|
Incidentally --- and this is extremely pedantic of me --- I don't think the title/commit message message for this PR is quite right. The client-controlled panic is actually in the syscalls, rather than the |
I [recently discovered][1] that the `software_irq` KIPC call is missing reference documentation. In fact, the KIPC documentation doesn't even mention it (in contrast to other KIPC calls that have no documentation but are still listed in the reference). Instead, the reference documentation currently incorrectly states that `find_faulted_task` is KIPC number 8, when it's actually KIPC 9 and `software_irq` is number 8. Not adding this KIPC to the docs was an oversight on my part in PR #1661 --- my bad, sorry! This commit adds documentation for the `software_irq` KIPC to the reference. [1]: #1961 (comment)
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!