libmicrovmi icon indicating copy to clipboard operation
libmicrovmi copied to clipboard

Memory as file descriptor

Open Wenzel opened this issue 3 years ago • 5 comments

This PR implements the memory as a file descriptor, using Rust standard traits

  • Read
  • Write
  • Seek

A second memory object is implemented to handle "padded" memory reads, useful for Volatility.

Changes

To introduce this memory abstraction, I had to update the API. The library doesn't return a dyn Introspectable anymore, but a Microvmi struct, which contains the driver and both memory objects (padded and non padded).

On this Microvmi struct, the methods are simply forwarding to the driver implementation. Regarding the memory objects, I had to use a Rc<Refcell<Box<dyn introspectable>>> to share the driver between all the structs, allowing a mutable borrow checked at runtime.

I tested against master while dumping the memory and the overhead is invisible.

Some unit tests have been implemented for the seek implementation.

I also changed how the drivers should implement reading physical memory.

The new API is:

    fn read_frame(&self, _frame: PageFrame, _buf: &mut [u8]) -> Result<(), IoError> {
        unimplemented!();
    }

Which allows us to remove complexity from the driver implementation to the Read trait implementation (especially for Xen), and share this for all drivers.

Wenzel avatar Apr 23 '21 09:04 Wenzel

Codecov Report

Merging #195 (9fef107) into master (acf7b73) will increase coverage by 4.43%. The diff coverage is 19.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #195      +/-   ##
==========================================
+ Coverage   14.19%   18.62%   +4.43%     
==========================================
  Files           5        7       +2     
  Lines         472      671     +199     
  Branches       88      108      +20     
==========================================
+ Hits           67      125      +58     
- Misses        383      513     +130     
- Partials       22       33      +11     
Flag Coverage Δ
unittests 18.62% <19.82%> (+4.43%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/api.rs 1.73% <0.00%> (+1.73%) :arrow_up:
src/capi.rs 0.00% <0.00%> (ø)
src/driver/kvm.rs 26.73% <0.00%> (+4.21%) :arrow_up:
src/errors.rs 0.00% <ø> (ø)
src/lib.rs 100.00% <ø> (+95.65%) :arrow_up:
src/microvmi.rs 0.00% <0.00%> (ø)
src/memory.rs 37.50% <37.50%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update acf7b73...9fef107. Read the comment docs.

codecov-commenter avatar Apr 30 '21 08:04 codecov-commenter

This PR is ready for review. @rageagainsthepc , if you want to have a quick look.

Wenzel avatar Apr 30 '21 08:04 Wenzel

@Wenzel Will do 👍

rageagainsthepc avatar Apr 30 '21 15:04 rageagainsthepc

Imho the microvmi struct looks a bit overly complicated. I would prefer a design where you (as a library user) initialize the driver like we used to and then pass it to one of the memory abstraction classes:

let drv = driver_init();
let memory = Memory::new(drv);
let padded_memory = Paddedmemory::new(drv);

Not every user needs memory abstractions, but those who do could easily initialize them. But it's just a thought, haven't looked at the PR in detail yet. Maybe I'm not seeing the bigger picture.

Edit: Right now this implementation would force the library user to write single threaded applications (because Rc does not implement Send). It would be better if the user could choose between Rc or Arc for example. The idea above would allow this behavior with minimal adjustments to the current implementation I think. Edit2: If you make your memory abstraction classes generic over the trait bound BorrowMut<Box<dyn Introspectable>> that should do the trick.

rageagainsthepc avatar May 08 '21 10:05 rageagainsthepc

Hi @rageagainsthepc and thank you for your feedback.

I should have explained a bit more why I decided this implementation and design when opening the PR. I would have also have prefered a simpler design, where the library would return a trait object (Box<dyn Introspectable), and provide wrapper on top of this, (Memory::new(drv)).

The issues comes from exposing this API in C and Python. because how do you keep the context ?

The C API exposes a void* drv pointer, to be casted into our box<dyn Introspectable>, however, this wouldn't retain the Memory object context as well, since it's a separate object.

That's the main reason why I choose to go this way, keep a single struct Microvmi to be casted as void* into the C world, back and forth, and able to offer the read/write/seek abstraction underneath.

A second reason is that we are able to simplify our read implementations in the drivers, mainly the Xen driver, since the main algorithm to iterate over each frame is implemented in the layer above.

And another reason is to be able to implement batch read operation later on, hidden in the Memory object implementation. Memflow has support for this since some of its targets have a very high latency, so have to batch you read operations, otherwise the whole read will become slow and uneffective.

Edit: as a bonus, I just noticed that with the struct Microvmi we can keep the DriverType after initialization, and therefore remove get_driver_type() from the driver's implementation

Wenzel avatar May 17 '21 09:05 Wenzel