betrusted-io / xous-core

The Xous microkernel
Apache License 2.0
532 stars 85 forks source link

update heapless::Vec to v0.7 (now: migrate to libstd no heapless) #30

Closed bunnie closed 3 years ago

bunnie commented 3 years ago

A problem was found where code of a form similar to the below would leak data, presumably from stack:

let mut keys: Vec<(usize, usize), U16> = Vec::new();

let r = 2;
let c = 0;
keys.push( (r, c) ).unwrap();
for &(r, c) in keys.iter() {
   print!("r{} c{}", r, c);
}

The contents returned in the (r, c) tuple would have r containing the value of c, and c would always be 15.

This commit allows one to reproduce the issue: https://github.com/betrusted-io/xous-core/commit/6a3833ebc797f8bda5dedc2cb3c1f2b63ee9fafe

Permalink to the lines of interest: https://github.com/betrusted-io/xous-core/blob/6a3833ebc797f8bda5dedc2cb3c1f2b63ee9fafe/services/keyboard/src/main.rs#L284-L305

A work-around was tried by replacing the tuple of (usize, usize) with a Struct of two usizes, and that had the same problem; the size of the Struct elements was reduced to u8, and then the code behaved properly.

The working version is here: https://github.com/betrusted-io/xous-core/commit/5cad6315693657cbd3253020737a7e337e2c198c

An attempt was made to pull in the heapless crate and make a test that runs on x86 to reproduce the issue, but of course, it passes on an x86 target.

Thus the problem seems specific to the RISC-V target at hand.

The issue is particularly scary because it doesn't involve any unsafe code. It's noted here as probably something that should be dug into later on, or at the very least, as heapless gets updated newer versions should be pulled in and tested by adjusting the type of the RowCol struct to usize again (the code has been written to make that easier).

bunnie commented 3 years ago

To be clear, the "fix" was to use u8 in the struct, which causes the problem to go away -- but the root cause was neither found nor addressed, hence this bug was opened with a security label because presumably this is a window into a deeper issue somewhere in the system that could be abused....

bunnie commented 3 years ago

If someone wanted to tackle this, I'd recommend trying to get the code to run in renode. You may have to do some massaging because renode doesn't emulate the keyboard, but with any luck a simple synthetic test case may trigger the bug anyways, and then you can pause and dig through stack/code to try and figure out why this happens.

bunnie commented 3 years ago

Heapless has had some major updates in the past couple months, including the introduction of const_generics. Changed the subject of this bug to update to the new crate, I bet this probably fixes this problem, too.

bunnie commented 3 years ago

Heapless was upgraded to 0.7, and the latest Rust edition was pulled too.

A new problem has appeared in 391122823d62279f4c32880f76e4b6bb0231ad0d, it appears that in xous-names entries are being pulled out of the FnvIndexMap that contain data from adjacent records. This is really unfortunate.

I poked into the heapless implementation and it is riddled with unsafe operations. It looks like, from the comments, the emphasis of the crate's implementation is on speed, not safety.

We are going to have to find a different solution for things like priority queues, vectors, etc. At the moment, the leading candidate is to get libstd finally ported over, so we can benefit from Rust's well-vetted std library, but this is a heavy lift.

bunnie commented 3 years ago

Tagging @xobs on this for now as this is blocked on getting libstd implemented. Any reccos for alternative, safe crates that can provide Vectors, Queues, HashMaps, and sortable lists would also be a viable option.

bunnie commented 3 years ago

we have libstd. heapless is no more. long live libstd! closing this bug.

thanks @xobs