Currently, the kernel panics if it runs out of schedulable tasks, meaning we have to remember to include an idle task in every image. This burns text and RAM -- perhaps we should let the scheduler idle internally?

This code executes undefined behavior:

The std::ptr module documents:

A null pointer is never valid, not even for accesses of size zero.

And ptr::read_volatile repeats this:

Note that even if T has size 0, the pointer must be non-null and properly aligned.

I think this line should be replaced with inline assembly that does the faulting read.

The stm32h7 spi server supports a Lock operation that causes it to listen to only one client. If that client crashes before releasing the lock, the server gets woken with a dead code.

It is not immediately clear to me that the server does something reasonable when that happens -- should verify.

The current configure_timing routine in stm32h7-i2c assumes that any board with a given SoC model has the same clock configuration.

Knowledge of clocks should probably move out of this crate to the caller, who should decide it based on target_board instead.

FYI @bcantrill

We have discussed but not implemented several kinds of integrity checks on task interactions. I'm filing this issue to keep track of them and record progress.

The cases I'm aware of are as follows.

Uphill send

Check that SEND is always used from low to high priority. This is one of our mechanisms for avoiding deadlock. Validating this would be relatively cheap (a compare on the send path).

However, there is at least one piece of software that's currently violating the uphill send rule deliberately, and that's the test suite -- specifically, the way the suite and assistant task send back and forth. We could refactor it to fix this, potentially.

However, note that this winds up being a subset of the next thing:

SEND MAC

Impose Mandatory Access Control (MAC) on the SEND operation, limiting the set of tasks that a given task can SEND to. The set would be defined by the application depending on its needs. Tasks outside the set would appear not to exist to the sender, for all intents and purposes -- that is, attempting to access one would be a fault, though it might not specifically be a TaskIndexOutOfRange fault.

Having this can potentially subsume uphill send validation, by ensuring at build time that each task can only send to higher-priority tasks.

Closed RECV reverse MAC validation

A task should only be able to enter closed RECV to listen to a task that could potentially send something. This means we should probably check the SEND MAC information on closed RECV.

REPLY reverse MAC validation

If task A replies to task B, but static MAC rules indicate that B could never have sent a message to A, A is probably malfunctioning. We can detect this case if we have MAC rules. I am inclined to try to detect it, in the interest of catching every programming error as early as possible, but because we must trust servers more than clients in general, I feel like it's slightly less important than (say) validating on SEND.

Post filters

The POST operation does not currently exist, but I'm adding it in #224. This will let any task poke any other task's notifications, simulating interrupts and just generally being a nuisance. (This is why the guidance on interrupt handling is to check the source of truth for the interrupt events instead of simply trusting the notification bits.)

I could imagine a couple of levels of filters here.

1. We could restrict which tasks can access the POST operation in the first place. POST is usually used by specialized servers that simulate interrupts -- the set I'm aware of is the supervisor, the network stack, and the STM32 external interrupt multiplexing driver that @labbott is writing.
2. We could restrict which bits can be posted, either separately or in addition to (1). For instance, we could use the interrupt routing information to generate a "not a hardware interrupt" mask for each task's notification bits, and only allow those to be posted. Or in the extreme we could have ACLs per bit, but that feels too heavy to me.

I currently feel like (1) is the sweet spot there.

Cc @plainspace

Might be fun! I bet @cbiffle has ideas, maybe something fun to brainstorm!

On this page

https://github.com/oxidecomputer/hubris

The "getting OpenOCD" link is broken and generates a 404 error.

The link is to

http://openocd.org/getting-openocd/

But should be to

https://openocd.org/pages/getting-openocd.html

I think that the OpenOCD site/pages were reorganized recently which probably caused this mismatch.

Per #209 I'm working on having the kernel stack grow away from kernel data, which on stm32 has it growing toward an unmapped section of address space that will bus-fault. This is likely to remain true on stm32, as stm32 does not need a hypovisor, only a bootloader -- so the bootloader should have no RAM needs after kernel boot.

On the lpc55, the hypovisor appears to reserve 16kiB of RAM permanently, and puts it at the base of the SRAM. This means the kernel stack will instead grow toward hypovisor space. If the kernel stack were to enter hypovisor space, we need a reliable trap in order to detect overflow.

Hypovisor runs in Secure, so the easiest way to achieve this may be to mark that section of SRAM as Secure-Only if we haven't already. This bug is open to remind us to check this.

As of #76. when a task doesn't match the intended generation, the new generation will be returned in the failure message. This is fine in principle, but we currently have zero tasks that actually deal with this properly: they will see an error code that they don't recognize, and they will (generally) panic!(). Most problematically. on restart, they will do this again: they will still use Generation::default, which again will not match the current generation, they will again make a call that fails with an error that they don't recognize, and they will (again) fail and (again) panic. On the one hand, we could modify every task to deal with this error properly, but this can be pretty thorny -- and for most tasks, the right answer is to reset anyway. It seems that we (still) need an easy way for tasks to query as to the current generation of a task they wish to communicate with. The change that would involve the least delta would be to modify TaskId::for_index_and_gen such that Generation::default denotes that the kernel should be queried for the current task generation. Regardless of the approach we talk, we definitely need to make some change here: as it stands, we are a series of infinite loops, waiting to happen.

Add the following to (say) the venerable pong task:

diff --git a/task-pong/src/main.rs b/task-pong/src/main.rs
index 9d06ed8..b938d08 100644
--- a/task-pong/src/main.rs
+++ b/task-pong/src/main.rs
@@ -3,11 +3,16 @@
 
 use userlib::*;
 
+#[no_mangle]
+pub static mut KABOOM: [u32; 1024] = [0xdefec8d; 1024];
+
 #[export_name = "main"]
 pub fn main() -> ! {
     const TIMER_NOTIFICATION: u32 = 1;
     const INTERVAL: u64 = 500;
 
+    unsafe { KABOOM[0] = 1; }
+
     let mut response: u32 = 0;
 
     let user_leds = get_user_leds();

And adjust app.toml to add (only) the necessary DRAM:

diff --git a/gemini-bu/app.toml b/gemini-bu/app.toml
index ad3b570..c46de87 100644
--- a/gemini-bu/app.toml
+++ b/gemini-bu/app.toml
@@ -91,7 +91,7 @@ start = true
 path = "../task-pong"
 name = "task-pong"
 priority = 3
-requires = {flash = 8192, ram = 1024}
+requires = {flash = 8192, ram = 8192}
 start = true
 
 [tasks.idle]

The resulting app.toml will now fail to flash:

$ cargo xtask flash ./gemini-bu/app.toml 
...
** Programming Started **
Info : Device: STM32H74x/75x
Info : flash size probed value 2048
Info : STM32H7 flash has dual banks
Info : Bank (0) size is 1024 kb, base address is 0x8000000
Info : Padding image section 0 at 0x08004c74 with 13196 bytes
Info : Padding image section 1 at 0x0800a65c with 6564 bytes
Info : Padding image section 2 at 0x0800d488 with 2936 bytes
Info : Padding image section 3 at 0x0800f628 with 2520 bytes
Info : Padding image section 4 at 0x0801190c with 9972 bytes
Info : Padding image section 5 at 0x08015c7c with 9092 bytes
Info : Padding image section 6 at 0x08019534 with 2764 bytes
Error: Section at 0x0801c000 overlaps section ending at 0x0801c33c
Error: Flash write aborted.

If one should have the misfortune of having this task be the last in the app.toml, it will successfully flash -- but pong will be stuck in a fault loop:

$ cargo xtask humility ./gemini-bu/app.toml tasks
...
humility: attached via ST-Link
ID ADDR     TASK               GEN STATE    
 0 20000128 jefe                 0 Healthy(Runnable)          <-
 1 20000198 rcc_driver           0 Healthy(InRecv(None))     
 2 20000208 gpio_driver          0 Healthy(InRecv(None))     
 3 20000278 usart_driver         0 Healthy(InRecv(None))     
 4 200002e8 i2c_driver           0 Healthy(InRecv(None))     
 5 20000358 user_leds            0 Healthy(InRecv(None))     
 6 200003c8 idle                 0 Healthy(Runnable)         
 7 20000438 pong                30 Faulted { fault: MemoryAccess { address: Some(0x801e000), source: User }, original_state: Runnable } 

The fundamental problem is that we're creating initialized RW data -- it is in neither .rodata nor .bss. This data must be allocated twice: it must exist in flash (where it is initialized), but must be copied into RAM. But because the allocation is only checked against RAM, failure to allocate sufficient flash results in either a corrupt image (the first case), or an image that walks off the MPU-protected region when initializing data in userlib::_start (the second case). There are presumably many ways to fix this, but a straightforward one is to have the build process check that a task's allocated flash does not exceed its requested flash. Diff for this fix:

diff --git a/xtask/src/dist.rs b/xtask/src/dist.rs
index ed1c18c..7ed2542 100644
--- a/xtask/src/dist.rs
+++ b/xtask/src/dist.rs
@@ -141,7 +141,18 @@ pub fn package(verbose: bool, cfg: &Path) -> Result<()> {
             &task_names,
             &toml.secure,
         )?;
-        let ep = load_elf(&out.join(name), &mut all_output_sections)?;
+
+        let (ep, flash) = load_elf(&out.join(name), &mut all_output_sections)?;
+
+        if flash > task_toml.requires["flash"] as usize {
+            bail!(
+                "{} has insufficient flash: specified {} bytes, needs {}",
+                task_toml.name,
+                task_toml.requires["flash"],
+                flash
+            );
+        }
+
         entry_points.insert(name.clone(), ep);
     }
 
@@ -177,7 +188,7 @@ pub fn package(verbose: bool, cfg: &Path) -> Result<()> {
         "",
         &toml.secure,
     )?;
-    let kentry = load_elf(&out.join("kernel"), &mut all_output_sections)?;
+    let (kentry, _) = load_elf(&out.join("kernel"), &mut all_output_sections)?;
 
     // Write a map file, because that seems nice.
     let mut mapfile = File::create(&out.join("map.txt"))?;
@@ -779,7 +790,7 @@ fn load_srec(
 fn load_elf(
     input: &Path,
     output: &mut BTreeMap<u32, LoadSegment>,
-) -> Result<u32> {
+) -> Result<(u32, usize)> {
     use goblin::container::Container;
     use goblin::elf::program_header::PT_LOAD;
 
@@ -793,6 +804,8 @@ fn load_elf(
         bail!("this is not an ARM file");
     }
 
+    let mut flash = 0;
+
     // Good enough.
     for phdr in &elf.program_headers {
         // Skip sections that aren't intended to be loaded.
@@ -806,6 +819,8 @@ fn load_elf(
         // is loaded in flash but expected to be copied to RAM.
         let addr = phdr.p_paddr as u32;
 
+        flash += size;
+
         // Check for address overlap
         let range = addr..addr + size as u32;
         if let Some(overlap) = output.range(range.clone()).next() {
@@ -824,7 +839,11 @@ fn load_elf(
             },
         );
     }
-    Ok(elf.header.e_entry as u32)
+
+    // Return both our entry and the total allocated flash, allowing the
+    // caller to assure that the allocated flash does not exceed the task's
+    // required flash
+    Ok((elf.header.e_entry as u32, flash))
 }
 
 /// Keeps track of a build archive being constructed.

Running with this fix yields:

$ cargo xtask flash ./gemini-bu/app.toml 
...
Error: task-pong has insufficient flash: specified 8192 bytes, needs 9020

We currently used closed receive against the kernel's virtual TaskId in cases where we only wish to receive notifications, not messages. It's currently possible to call this with a notification mask with all bits clear. In this case, the task will never receive notifications. This is okay-ish in our intended applications because an application-level watchdog will eventually restart the dead task, but, it is not ideal.

We could detect this immediately on entry to the kernel and turn it into a UsageError fault against the task, since it most likely represents a programming error.

I think we've got a TODO about this somewhere in the code but I don't see an issue filed, so, here's an issue. Thanks to an anonymous friend of @steveklabnik's for reminding me about this.

xtask flash currently has a hardcoded table of board names for finding flashing config files. That's going to become annoying. It should take direction from app.toml instead.

I'm imagining something like

[flashing]
method = "openocd"
config = "../whatever.cfg"

where the paths are relative to the app.toml itself, as usual.

I'm writing this down just so that we don't forget. :-)

The way the ringbuf macro manages its static mut storage is likely unsound and we should take a pass over it.

It's "unsound" in the "could easily break in weird ways later" sense, not in the "currently doesn't work" sense -- it works fine.

So, aspects of the IPC ABI mean that we need to keep the generation sent by callers relatively small (6 bits, currently), because we're real short on registers on ARMv7-M. (Thank goodness we're not trying to support x86.)

This is probably fine for the original purpose of generations, which is to provide best-effort assurance that the task you're talking to has not restarted since you last spoke to it.

However, generations are also finding use as a "crash counter," in which case having them wrap around can be deceptive. We had a case where we observed a crashing task having GEN 0, which sure looks like it hasn't ever crashed.

We can fix this pretty easily by expanding the generation field in the task record to something like a u32, and only comparing the bottom N bits for checking IPC generation.

Current status summary

Most of this is now done, we just need to detect the DMA attribute in Task::can_access.

Original description follows

In the kernel, we use the USlice type to model unprivileged memory regions. To access the contents of those regions from the kernel, we normally use can_read to verify access for a specific task, followed by assume_readable to turn the abstract memory region into a kernel-native &[T].

The reason why we can ever do this is that the execution of the kernel is mutually exclusive with all tasks, by design. While the kernel is running, all tasks are frozen. (Importantly, this is why all of our discussions around multi-core have assumed a kernel per core.) Because the USlice and Task implementations ensure the basic properties like correct alignment and access permissions, we can get a valid Rust slice that we can access without racing unprivileged task code.

However -- while this means it's correct sometimes, it is not sufficient to ensure that it's always correct. Because we use a Rust shared reference to access the task memory, we are expressing to the compiler that the contents of that memory can't change, and that assumption does not hold in two cases:

1. The memory is RAM but the task has arranged for DMA to be writing into it.
2. The memory is not RAM and is in fact memory-mapped peripheral registers.

As a concrete example, consider the panic path, which was noted by @iljavs2 in our IOActive review. It checks that the alleged message is valid ASCII and then does from_utf8_unchecked (to avoid the enormous text cost of the checked version of from_utf8). If the memory is changing, we have a TOCTOU issue that could produce a non-UTF8 str in the kernel. (This specific issue can be eliminated by not klog-ing the panic message, since the supervisor is responsible for that anyway, but the point remains.)

One possible fix

In order to fix this, we likely need to have the kernel distinguish between RAM and registers, and DMA-capable RAM vs not. We already have the latter attribute, but we do not currently record the RAM-register distinction in the image, though we do have it at build time.

The kernel could then decline to access either class of region.

This fix might be heavy-handed: it would prevent a task from putting a lease table or panic message anywhere in DMA-capable RAM. Nothing prevents a task from marking all of its RAM as DMA-capable, today -- it produces a performance hit on Cortex-M7 by bypassing cache, but is otherwise fine.

The fix is also technically insufficient, as a misbehaving task could set up DMA to non-DMA-marked RAM.

Another possible fix

We may want to eliminate the assume_readable / assume_writable operations that produce Rust references, and always access unprivileged memory using explicit pointer accesses. My use of kernel-native slices for accessing unprivileged memory was a convenience that simply be incorrect. More analysis needed.

Build, say, demo, and observe resulting app table:

$ cargo xtask dist demo/app.toml
...
$ humility apptable ./target/demo/dist/kernel | more
App = {
        magic: 0x1defa7a1,
        task_count: 0x7,
        region_count: 0x13,
        irq_count: 0x1,
        fault_notification: 0x1,
        zeroed_expansion_space: [
            0x0,
            0x0,
            0x0,
            0x0,
            0x0,
            0x0,
            0x0,
            0x0,
            0x0,
            0x0,
            0x0,
            0x0
        ]
    }
...

Now change the app.toml to (say) send notifications to task 2:

diff --git a/demo/app.toml b/demo/app.toml
index f5af15f..de0d5b7 100644
--- a/demo/app.toml
+++ b/demo/app.toml
@@ -19,7 +19,7 @@ requires = {flash = 65536, ram = 4096}
 features = ["itm"]
 
 [supervisor]
-notification = 1
+notification = 2
 
 [outputs.flash]
 address = 0x08000000

Thanks to @steveklabnik's work in #87, it correctly realizes that each package should be cargo clean'd:

$ cargo xtask dist demo/app.toml
    Finished dev [optimized + debuginfo] target(s) in 0.21s
     Running `target/debug/xtask dist demo/app.toml`
flash = 8000000..8040000
ram = 20000000..2001c000
kernel: {"flash": 8000000..8010000, "ram": 20000000..20001000}
app.toml has changed; rebuilding all tasks
cleaning demo
cleaning task-jefe
cleaning drv-stm32f4-rcc
cleaning drv-stm32f4-usart
cleaning drv-user-leds
cleaning task-ping
cleaning task-pong
cleaning task-idle
building path demo/../task-jefe
    Finished release [optimized + debuginfo] target(s) in 0.14s
/home/bmc/hubris/target/thumbv7em-none-eabihf/release/task-jefe -> target/demo/dist/jefe
building path demo/../drv/stm32f4-rcc
    Finished release [optimized + debuginfo] target(s) in 0.15s
/home/bmc/hubris/target/thumbv7em-none-eabihf/release/drv-stm32f4-rcc -> target/demo/dist/rcc_driver
building path demo/../drv/stm32f4-usart
    Finished release [optimized + debuginfo] target(s) in 0.14s
/home/bmc/hubris/target/thumbv7em-none-eabihf/release/drv-stm32f4-usart -> target/demo/dist/usart_driver
building path demo/../drv/user-leds
    Finished release [optimized + debuginfo] target(s) in 0.14s
/home/bmc/hubris/target/thumbv7em-none-eabihf/release/drv-user-leds -> target/demo/dist/user_leds
building path demo/../task-ping
    Finished release [optimized + debuginfo] target(s) in 0.14s
/home/bmc/hubris/target/thumbv7em-none-eabihf/release/task-ping -> target/demo/dist/ping
building path demo/../task-pong
    Finished release [optimized + debuginfo] target(s) in 0.13s
/home/bmc/hubris/target/thumbv7em-none-eabihf/release/task-pong -> target/demo/dist/pong
building path demo/../task-idle
    Finished release [optimized + debuginfo] target(s) in 0.14s
/home/bmc/hubris/target/thumbv7em-none-eabihf/release/task-idle -> target/demo/dist/idle
building path demo/../demo
    Finished release [optimized + debuginfo] target(s) in 0.15s
/home/bmc/hubris/target/thumbv7em-none-eabihf/release/demo -> target/demo/dist/kernel

But note that the kernel has not in fact been rebuilt, and contained the old app table:

$ humility apptable ./target/demo/dist/kernel | grep fault_notification
        fault_notification: 0x1,

If, say, a kernel source file is touched, the kernel will be rebuilt:

$ touch ./kern/src/arch/arm_m.rs
$ cargo xtask dist demo/app.toml
    Finished dev [optimized + debuginfo] target(s) in 0.19s
     Running `target/debug/xtask dist demo/app.toml`
flash = 8000000..8040000
ram = 20000000..2001c000
kernel: {"flash": 8000000..8010000, "ram": 20000000..20001000}
building path demo/../task-jefe
    Finished release [optimized + debuginfo] target(s) in 0.13s
/home/bmc/hubris/target/thumbv7em-none-eabihf/release/task-jefe -> target/demo/dist/jefe
building path demo/../drv/stm32f4-rcc
    Finished release [optimized + debuginfo] target(s) in 0.14s
/home/bmc/hubris/target/thumbv7em-none-eabihf/release/drv-stm32f4-rcc -> target/demo/dist/rcc_driver
building path demo/../drv/stm32f4-usart
    Finished release [optimized + debuginfo] target(s) in 0.13s
/home/bmc/hubris/target/thumbv7em-none-eabihf/release/drv-stm32f4-usart -> target/demo/dist/usart_driver
building path demo/../drv/user-leds
    Finished release [optimized + debuginfo] target(s) in 0.14s
/home/bmc/hubris/target/thumbv7em-none-eabihf/release/drv-user-leds -> target/demo/dist/user_leds
building path demo/../task-ping
    Finished release [optimized + debuginfo] target(s) in 0.13s
/home/bmc/hubris/target/thumbv7em-none-eabihf/release/task-ping -> target/demo/dist/ping
building path demo/../task-pong
    Finished release [optimized + debuginfo] target(s) in 0.13s
/home/bmc/hubris/target/thumbv7em-none-eabihf/release/task-pong -> target/demo/dist/pong
building path demo/../task-idle
    Finished release [optimized + debuginfo] target(s) in 0.13s
/home/bmc/hubris/target/thumbv7em-none-eabihf/release/task-idle -> target/demo/dist/idle
building path demo/../demo
   Compiling kern v0.1.0 (/home/bmc/hubris/kern)
   Compiling demo v0.1.0 (/home/bmc/hubris/demo)
    Finished release [optimized + debuginfo] target(s) in 1.67s
/home/bmc/hubris/target/thumbv7em-none-eabihf/release/demo -> target/demo/dist/kernel

And the app table is now what we would expect:

$ humility apptable ./target/demo/dist/kernel | grep fault_notification
        fault_notification: 0x2,

(It should go without saying that one should not attempt to run such a kernel!)

The problem appears to be that cargo clean is not in fact blowing away the built kernel artifact:

$ find target -name demo-\*
target/thumbv7em-none-eabihf/release/.fingerprint/demo-e7eb269c08982ab0
target/thumbv7em-none-eabihf/release/deps/demo-e7eb269c08982ab0
target/thumbv7em-none-eabihf/release/deps/demo-e7eb269c08982ab0.d
$ cargo clean -p demo
$ find target -name demo-\*
target/thumbv7em-none-eabihf/release/.fingerprint/demo-e7eb269c08982ab0
target/thumbv7em-none-eabihf/release/deps/demo-e7eb269c08982ab0
target/thumbv7em-none-eabihf/release/deps/demo-e7eb269c08982ab0.d

If the build artifact is manually blown away, the kernel is properly rebuilt -- but I couldn't seem to find the incantation for cargo clean to do this...

Currently we're doing stuff like

[tasks.spi_driver]
# irrelevant bits omitted
interrupts = {51 = 1}

Good old interrupt 51. ...

Better would be

interrupts = {spi3 = 1}

The SVD files consumed by the PAC crates during their build process contain enough information to do this, but the PAC crates don't produce any kind of reflective interface we could use to reason about interrupts on x86. Too bad, that. We may have to build our own thing.

Due to some recent changes, HIFFY_DATA is being optimized away, leading to the following failure mode in Humility:

humility: hiffy failed: expected hiffy interface not found: variable HIFFY_DATA not found

We need a better long-term fix for this, but right now the fix ("fix") is to trick the compiler into thinking HIFFY_DATA is being used.

Because we don't use cargo build to build a task, cargo-expand doesn't work for them. It would be nice to gain support for this somehow, possibly by integrating it into xtask somehow?

Currently, peripheral sizes must be powers of two -- and if they aren't, they are (silently) undersized; from apply_memory_protection:

        // This is a bit of a hack; it works if the size is a power of two, but
        // will undersize the region if it isn't. We really need to validate the
        // regions at boot time with architecture-specific logic....
        let l2size = 30 - region.size.leading_zeros();
        let rasr = (xn as u32) << 28
            | ap << 24
            | tex << 19
            | scb << 16
            | l2size << 1
            | (1 << 0); // enable
        unsafe {
            mpu.rbar.write(rbar);
            mpu.rasr.write(rasr);
        }

This behavior is currently silent (and was found due to the GPIO sizing on STM32H7 of 0x2c00 and the manipulation of a port that maps to the upper 0xc00 bytes of that range inducing a fault); it would be great to enforce this at several levels -- presumably starting with the build, which should reject sizes that aren't powers of two.

The code inside covers compares the self_end with the slice base address and the slice end with self.base. This may or may not be correct, however, it's not self explanatory (as you'd expect a self_end to be compared with a slice end and self.base to be compared with the slice base). Either a comment explaining this would make sense, or, in case it is wrong, a fix for this issue.

hubris-master\kern\src\app.rs

impl RegionDescExt for abi::RegionDesc {
    /// Tests whether `slice` is fully enclosed by this region.
    fn covers<T>(&self, slice: &crate::umem::USlice<T>) -> bool {
        let self_end =
            self.base.wrapping_add(self.size).wrapping_sub(1) as usize;
        if let Some(slice_end) = slice.last_byte_addr() {
            // Slice is not empty
            self_end >= slice.base_addr() && slice_end >= self.base as usize   <-- should this not be reversed? self_end >= slice_end && slice.base_addr() >= self.base
        } else {
            self_end >= slice.base_addr()
                && slice.base_addr() >= self.base as usize
        }
    }
}

Describing what "defected" means in the documentation would be helpful.

slice::from_raw_parts_mut documents that

data must point to len consecutive properly initialized values of type T.

I see that M::Response requires zerocopy::FromBytes. I think this trait bound means that the pointed-to data for the slice could be initialized with core::mem::zeroed(). But it's totally unclear to me if this initialization imposes too much overhead, and the interface for sys_send should be to pass through the MaybeUninit-ness of the data by using a &[MaybeUninit<u8>] instead of &[u8].

Currently, if a task that one needs dies, subsequent calls will fail. While it is true that a savvy (or resigned) client could simply increment the generation number looking for the current generation of its server, there should really be a way of getting this. @cbiffle posited that a Dead response could return the current generation in its payload, which seems like it would solve the problem rather nicely.

Does the kernel switch between tasks or is it the supervisor task that does it? Is it preemptive or cooperative?

I'm not sure we've done anything to trap if the kernel dereferences NULL. We should consider using the MPU to achieve this, perhaps by dedicating a region slot to an inaccessible region positioned around NULL, or perhaps by swapping one in when we enter the kernel.

On STM32 we can just revoke access to the bottom X bytes of address space (where X is large, about 128 MiB), because the vector table that initially lives there is aliased from its "real" address up higher.

On LPC55 I'm less confident. @labbott ?

The ARM ecosystem is complex enough that I'm sure there's some micro out there where the kernel needs access to zero, but let's try to avoid buying it. :-)

Noticed this in passing: one of the side effects of the borrow_lease operation in the kernel is that, if applied to a task that is not waiting in reply, it assumes that task has defected (claimed to loan things and not actually loaning them) and kills it.

I believe that this currently means you can use any of the borrow syscalls to kill any task you like. That's bad. We should alter this logic.

tl;dr: we are passing arguments to the kernel / returning values from the kernel in registers r6 and r7. There's a reason for this. However, LLVM wants to use those for the base and frame pointer, respectively, and there is no way to tell it not to. This is complicating our syscall stubs and we might want to revisit the ABI design.

While working on #70, I have run into multiple stack overflows. These were entirely my fault, but it took me far too long to realize what was going on, diagnose and fix it. Some of that is my relative inexperience, of course, but... yeah. Basically, when this happens, you get a silent failure.

A screenshot of the first one, since the branch is likely to get deleted:

Second, my tasks were hitting a panic, and then blowing up, which was expected, but silently, which was not. This is because I had set the ram for my task too low, because I copy/pasted the config from task-idle, thinking that a smaller task is a good idea to start with.

It would be nice if it was much more obvious that this kind of failure was causing these problems, though I won't be making this mistake again (I hope...).

can_access is used in the kernel to validate incoming USlices, to make sure they're sane. There is some special casing for 0-sized slices, essentially, they're not validated. There is a comment that explains this, it is to account for
literals like &[] which apparently produce a base address of 0 + sizeof::<T>(), however, it might make sense to special case only that and still validate other 0-sized USlices.

In general it sounds reasonable not to validate 0-sized slices, since the slice is 0 bytes long. However, functionally similar APIs like can_access on different platforms (e.g. ProbeForRead, ProbeForWrite on windows) have the same behavior and it has lead to numerous bugs being exploitable. While, yes, technically it's not can_access' fault that someone else does a non 0-sized read or write on a user provided USlice of 0-size, the point is that this behavior exacerbates the problem. It should also be noted that due to the nature of rust, it's significantly less likely that issues like these would come up. However, given the history of behavior like this, it would probably make sense to try and validate a USlice of 0 size, and simply special case &[].

hubris-master\kern\src\task.rs

    pub fn can_access<T>(
        &self,
        slice: &USlice<T>,
        atts: RegionAttributes,
    ) -> bool {
        if slice.is_empty() { // <-- this might not be a good design decision. not checking for 0-sized buffers will make certains bugs exploitable. 
            // We deliberately omit tests for empty slices, as they confer no
            // authority as far as the kernel is concerned. This is pretty
            // important because a literal like `&[]` tends to produce a base
            // address of `0 + sizeof::<T>()`, which is almost certainly invalid
            // according to the task's region map... but fine with us.
            return true;
        }
        self.region_table.iter().any(|region| {
            region.covers(slice)
                && region.attributes.contains(atts)
                && !region.attributes.contains(RegionAttributes::DEVICE)
        })
    } ```

The kernel currently uses a bump-pointer allocator to allocate task structures at startup, from its free RAM section. When the free RAM section is not long enough for this, it will panic, and this panic is difficult to diagnose if you're not watching over ITM at the time. We should fix this. Potential approaches off the top of my head include

• Figure out how to allocate that stuff statically, converting this into a build failure.
• Make the panic behavior better
• Add kernel panic diagnosis to humilty

I'm seeing rebuilds of a lot of crates I wouldn't expect, particularly dependencies of xtask. We should measure and investigate.

libudev-dev was missing from the instructions; create an easy copy-paste to install all system dependencies.

I'm a hobbyist tinkerer who likes to play with microcontrollers and small computers like arduinos, raspis and feathers. I began reading the documentation for hubris hoping to get a taste for what running a rust-based microkernel on something like the new rp2040 feather would feel like and I'll admit that I'm overwhelmed by all the low level systems details and lack of hand-holding! I'm sure the choice to eschew virtual addressing - for instance - was an important one architecturally, but I'm at the stage where I want to flash my feather and blink an LED and I'm not clear on where to go for that.

I think what I would like to see is a concrete tutorial based on something you could buy from adafruit or sparkfun and something simple like blinking an LED, detecting a button press or buzzing a peizo. I would be tempted to copy something else structurally, or at least drawing inspo from the arduino and circuitpython projects.

I get if this is a low priority - oxide's goals don't seem to hinge on the hobbyist market - and I certainly don't want to be demanding, but I think it would make it a lot more approachable for me specifically ;) and hopefully I'm not the only one.

Thanks! Hope this is helpful!

In writing the fault injection operation, I observed that both kipc::read_task_status and kipc::restart_task have an off-by-one error in that they only check if the target exceeds the task table length. If one specifies a target that equals NUM_TASKS, the kernel will panic, e.g.:

panicked at 'index out of bounds: the len is 4 but the index is 4', kern/src/kipc.rs:94:9

This will be fixed (and tests will be added) as part of the jefe external control and fault injection work.

Should a task always start with interrupts disabled, or not? Investigate. (This is a placeholder so I don't forget.)

So, we've kind of avoided dealing with the timebase problem. (Well, okay, mostly me.)

Currently we express time in all APIs as "system ticks," where the system tick is set for the application (board+image) to whatever you want .... but in practice is milliseconds. And basically all code assumes it's milliseconds.

There are two problems with this.

1. We often need timing more precise than milliseconds, but changing the tick frequency, while easy, would break basically all drivers.
2. The userlib time-related APIs don't actually do what you'd expect.

On that second point -- if you call sleep_for(1), you will (in the absence of higher priority tasks keeping you from being scheduled at all) sleep for somewhere between 0ms and 1ms. In other words, it's really sleep_for_at_most_ms. Often, we actually want at least.

The actual kernel time API is based on absolute deadlines rather than intervals (basically to keep the kernel out of this argument) so I think we could fix this part in userlib.

Currently, xtask dist runs around cleaning things whenever app.toml changes. This was originally done so that switching between apps didn't accidentally link in each others' objects. It's kind of annoying, however, and shouldn't be strictly necessary ... if we've got all the various build dependencies right. It may be masking cases where we forgot to (say) emit a cargo:rebuild-if-whatever line from the build script.

It would be great for one of us to investigate and make this more precise, as it should dramatically reduce build times for people working on more than one app.

It's also going to become increasingly important as we move more config information into the toml, since right now, changing (say) the proposed i2c config tree will force a rebuild of all tasks, which is overkill.

Currently,

\Users\steve\scoop\apps\gcc-arm-none-eabi\current\bin\arm-none-eabi-gdb.exe: warning: Couldn't determine a path for the index cache directory. 
                                                                                                                      
 Reading symbols from target/dist/combined.elf...                                                                                    
(No debugging symbols found in target/dist/combined.elf)                                                                            
add symbol table from file "target/distkernel" 
                                                                                     
target/dist/script.gdb:1: Error in sourced command file:                                                                            
target/distkernel: No such file or directory.   

this is because script.gdb looks like this:

add-symbol-file target/dist\kernel
add-symbol-file target/dist\jefe
add-symbol-file target/dist\rcc_driver
add-symbol-file target/dist\usart_driver
add-symbol-file target/dist\ping
add-symbol-file target/dist\pong
add-symbol-file target/dist\idle

and gdb does not like the \. Changing them to / works just fine, but we shouldn't require manual intervention to get things going, of course

The task's name is set by the key in the tasks map, not the name attribute of the task entry. The name attribute is the name of the bin crate to be built.

We do not currently have a way to poke notification bits from taskyland, because we haven't needed it. The original design proposal included a post operation for doing this, which would let a task -- subject to MAC eventually -- OR a word into another task's notification bits, possibly waking it.

We're approaching some use cases where we'll need to have this implemented.

Kernel depends on the contents of the app table at runtime but only the location of the table at build time. Since the app table includes the entry point of each task, it necessarily depends on having built all of the tasks. If we use tricks similar to how peer tasks are resolved, the compilation of the kernel will no longer be dependent on the build output of the tasks.

It looks like the build time changes in #73 aren't applying to flash/ram on the H743, at least as of CI runs on pull #81. We should figure out why.

Right now, we can't do multiple SPI transactions in a single HIF program, because spi_read/write always use data[0..].

We should parameterize the offset within the data array to make this possible. This probably means adding another argument, which would be a breaking change for Humility as well.

Currently, tasks request amounts of flash and RAM. The RAM is used for both their stack, and any statics (including, potentially, the RAM handed to an allocator). As on lots of early machines, the stack grows down toward the statics. As on those early machines, the reason I did this initially was to be squidgy with resource allocation: if you need 700 bytes of stack and 200 bytes of static, sneaking those into the same 1024-byte region saves memory compared to dedicating 1024 and 256 bytes, respectively, as required by the ARMv7-M MPU model.

This creates the potential for the stack to grow into the statics and begin randomly corrupting things. This can happen without an explicit stack overflow trap, because we only detect overflow when the stack reaches the bottom of RAM -- which is by definition too late.

The least intrusive way to fix this is to swap the two areas of RAM, so that the stack grows away from statics toward the bottom of the RAM region (and thus toward an unmapped and reliably trapping section of address space). This hack works because stack overflows happen in compiled structured programming languages, but stack underflows do not, in practice, as we are not writing Forth. To do this, we need to know at link time the maximum size of the stack for each task, which is information we don't currently have.

I suggest that we add this information to each task's entry in the app.toml. I've been concerned that it would be yet another fiddly parameter to tweak during development, but the operating system's entire userbase (namely @bcantrill @labbott @steveklabnik and @kc8apf) appear to agree that this is fine in exchange for finer-grained failure detection, proving once again that I chose to work with the correct people. ;-)

Having the stack size information enables another option on ARMv8-M: continue to have the two areas grow toward each other, but use the newfangled PSPLIM register to impose a hardware-checked growth limit on the stack. This would require changes to the linker script and context switch code. I think it's interesting, but we'd need to be able to articulate why it is better than having the stack grow toward unmapped address space. Perhaps it allows for more precise error reporting?

I'm assuming that I'll be doing this work, but just in case, I expect the task to involve the following:

1. Add a stack size parameter to the task TOML record.
2. Assign appropriate stack sizes to each task by trial and error. (Note: we can do this because we don't support debug builds, which have wildly variable stack usage.)
3. Add an additional environment variable hack to forward this information through the packager xtask to the userlib build.rs, or funnel it through one of the existing ones.
4. Output an additional linker script fragment to communicate the stack size to the linker.
5. Alter the existing user linker script to swap stack and data/bss by reserving N bytes and then defining the _initial_stack symbol or whatever (currently set to ORIGIN(RAM) + LENGTH(RAM))

No changes should be required in context switching or fault handling, unless we decide that using PSPLIM would be nice.

Task restarts are implemented as of yesterday, which got me thinking through the corner cases in restarts and IPC. The current implementation has quite a few corner cases that are poorly handled.

Current situation

Each task has a generation number, which distinguishes successive incarnations of a task (mod small number). This is used to alert peers interacting with a task if that task falls over during interactions: they start getting errors (DEAD) until they update their target generation number. The restart_task algorithm increments the generation number to facilitate this.

This is a good start, but is not sufficient to cause bewildering and error-prone interactions between tasks. Let's consider a few cases.

The reply that never comes. Task A sends to task B, which receives the message and then crashes. B is restarted. A is still blocked in InReply(B), but B has no idea there is a client waiting. A is now dead.

Reply after reincarnation. Same scenario, except -- for whatever reason -- B decides to reply to A. This should not work, but (on current master) does, because currently task state is recorded without peer generation numbers.

Misdirected receive. "Directed receive" is a receive operation from a specific peer only. It isn't exposed in the syscall interface, but is implemented in the kernel. Were we to expose it, task A could wait for messages from task B, which crashes. Task A's receive either (1) can never be fulfilled, or (2) can be fulfilled by any random message from a future incarnation of B. This is bad, because directed receives are expected to be used as part of back-and-forth messaging protocols, which is exactly where you want to detect a crashing peer.

The send race. Task A sends to B while B is doing something else -- A is blocked in InSend(B). B crashes and is restarted. When B goes to receive, it will receive the message from A (because, again, task states do not currently involve generation numbers), and A will get its message processed without receiving the DEAD response code that it would have received had the send not blocked. This is a system-load-dependent behavior that could produce nasty, difficult-to-reproduce bugs.

Faulty fault recovery. In a system where a supervisor attempts to recover some tasks from fault conditions -- like, hypothetically, a system that implemented virtual memory -- a task might sit in a fault condition for an arbitrary period of time before returning to Healthy status. Because IPC operations can trigger faults, the Healthy status that gets restored may very well be an IPC-related status naming a peer B. If the peer B has restarted while the task was waiting in fault, the resumed task will now be in one of the previously described conditions through no fault of its own. (While I do not intend to implement virtual memory / demand paging in any of our currently planned systems, there are some cases where fault recovery might make sense, such as emulating access to privileged memory-mapped peripherals, or debug trap handling.)

Fixing this

If A uses an IPC operation at B, and B gets restarted before feedback is returned to A, the feedback returned to A should indicate DEAD.

• If A is blocked InSend(B) or InReply(B) when B is restarted, A should be unblocked with a DEAD response code. (Note that B's generation number is checked before blocking A, so the case where B restarts before the operation is issued has already been covered.)
• If A is in directed receive (InRecv(Some(B))) when B is restarted, A should be unblocked with DEAD.
• Task SchedState values should include generation number in addition to task index, so that we can notice a generation mismatch when resuming a task from a faulted state. (This also makes any bugs in the previous two bullets less likely to produce time-travel effects where a message is received from a "future" task version.)
• Any kernel operations that move tasks between the blocked states must also consider this.

Stated in terms of a kernel invariant, we get:

• Type level: Any Healthy SchedState of any task that names another (peer) task must use a generation number (i.e. use a TaskId combining index and generation).
• Runtime invariant: Any Healthy SchedState that names another (peer) task must refer to the current generation of that task. If this is ever broken, it means we have failed to accurately report a DEAD condition to that task.

I'm going to work through the implementations of the various cases and (1) fix the bogus behavior and (2) see if I can find ways to make the behavior harder to produce under maintenance.

Changes to the system definition in app.toml don't necessarily induce a rebuild/relink of the kernel, resulting in run-time inconsistencies (including but not restricted to panics).

To see this, first build a demo (i.e., STM32F407):

$ cargo xtask dist -v demo/app.toml

Look at the program headers:

$ readelf -l ./target/demo/dist/kernel 

Elf file type is EXEC (Executable file)
Entry point 0x80001a9
There are 4 program headers, starting at offset 52

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x000120 0x08000000 0x08000000 0x048ac 0x048ac R E 0x20
  LOAD           0x0049ec 0x080048ac 0x080048ac 0x00204 0x00204 R E 0x20
  LOAD           0x004c00 0x20000000 0x20000000 0x00000 0x00020 RW  0x20
  GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00000 RW  0

 Section to Segment mapping:
  Segment Sections...
   00     .vector_table .text .rodata 
   01     .hubris_app_table 
   02     .bss 
   03     

Now change (e.g.) the starting address:

diff --git a/demo/app.toml b/demo/app.toml
index f5af15f..f76ae38 100644
--- a/demo/app.toml
+++ b/demo/app.toml
@@ -28,7 +28,7 @@ read = true
 execute = true

 [outputs.ram]
-address = 0x20000000
+address = 0x20001000
 size = 114688
 read = true
 write = true

With that change alone, rebuild:

$ cargo xtask dist -v demo/app.toml

And note that the program header hasn't changed (specifically, the VirtAddr for the RAM-based program header is still 0x20000000):

$ readelf -l ./target/demo/dist/kernel

Elf file type is EXEC (Executable file)
Entry point 0x80001a9
There are 4 program headers, starting at offset 52

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x000120 0x08000000 0x08000000 0x048ac 0x048ac R E 0x20
  LOAD           0x0049ec 0x080048ac 0x080048ac 0x00204 0x00204 R E 0x20
  LOAD           0x004c00 0x20000000 0x20000000 0x00000 0x00020 RW  0x20
  GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00000 RW  0

 Section to Segment mapping:
  Segment Sections...
   00     .vector_table .text .rodata
   01     .hubris_app_table
   02     .bss
   03

Now touch one of the kernel dependencies and rebuild; the kernel is rebuilt and the program header changes as expected (i.e., VirtAddr for the RAM-based program header is now 0x20001000):

$ touch ./kern/src/arch/arm_m.rs
$ cargo xtask dist -v demo/app.toml
...
$ readelf -l ./target/demo/dist/kernel

Elf file type is EXEC (Executable file)
Entry point 0x80001a9
There are 4 program headers, starting at offset 52

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x000120 0x08000000 0x08000000 0x048ac 0x048ac R E 0x20
  LOAD           0x0049ec 0x080048ac 0x080048ac 0x00204 0x00204 R E 0x20
  LOAD           0x004c00 0x20001000 0x20001000 0x00000 0x00020 RW  0x20
  GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00000 RW  0

 Section to Segment mapping:
  Segment Sections...
   00     .vector_table .text .rodata 
   01     .hubris_app_table 
   02     .bss 
   03     

We need to rebuild the kernel when anything that affects the app.toml changes -- and note that in the testing rework, the same kernel can have multiple app.tomls point to it; it may be easier to always relink the kernel?

The AsFooArgs structs in kern/src/task.rs were my first attempt at having an abstraction layer in syscall argument handling, to insulate most of the code from architecture and ABI changes. They work but adding a syscall requires kind of a lot of typing.

I bet we could make this simpler.

Stack overflows in the kernel aren't detected. They're also somewhat unlikely, since the kernel itself is non-preemptible, but, in theory, a stack overflow could corrupt data.

The most general way to fix this is to have the kernel stack grow toward a permanently inaccessible guard region. These are easiest to find at the edge of a memory, where they'll tend to cause a bus error, but that's not always practical. They can be created by the MPU, but they need to be inaccessible to privileged mode, meaning they can't overlap with some task or we won't be able to accept messages from that section of task RAM.

If we use an MPU guard region, it either needs to permanently occupy one of the limited MPU slots, or be swapped in really early on kernel entry and replaced on kernel exit. These both have drawbacks; the first one burns one of eight slots; while the second one increases the cost of syscall entry/exit for calls that don't cause a context switch, which is a lot of calls. (On such calls we are careful not to update the MPU.)

last_byte_addr is called to grab the highest address from a USlice it is called by covers which is used by can_access to validate a USlice. This method contains an integer overflow if a large length is specified when multiplying with the size of the type. This can be problematic when trying to validate an invalid or malicious USlice, as the address would wrap and be shorter than what the USlice really describes.

hubris-master\kern\src\umem.rs

    pub fn last_byte_addr(&self) -> Option<usize> {
        // This implementation would be wrong for ZSTs, but we blocked them at
        // construction.
        let size_in_bytes = self.length * core::mem::size_of::<T>(); <-- this can int overflow if self.length is very large 
        if size_in_bytes == 0 {
            None
        } else {
            Some(
                self.base_address
                    .wrapping_add(size_in_bytes)
                    .wrapping_sub(1),
            )
        }
    }

This seems like a one-off as USlice::from_raw for example does this correctly:

        let size_in_bytes = length
            .checked_mul(core::mem::size_of::<T>())
            .ok_or(UsageError::InvalidSlice)?;