Added capability to load extra blob data in sandbox

[danbugs]

This PR contains the following changes:
(1) Made a non-breaking change to the signature of the UninitializedSandbox::new(...) function. Now, instead of a GuestBinary, it takes a GuestEnvironment, which contains the usual binary + optional blob data.
(2) Created new user memory region to store the extra blob data and added some extra config options for it.
(3) Added info of the new memory region to the PEB.
(4) Added tests verifying added blob data.

Reviewing commit-by-commit might be best 👍

[ludfjig]

The PR comment states that the this is a non-breaking change, but the rename GuestBinary -> GuestBlob is breaking, right? Also I do think the renaming is not intuitive for users who want to load a guest binary, when it's now called GuestBlob

Not strictly related to your PR, but I think we should consider not allowing passing blobs/binaries by "path", which would force the consumers to do their own reading of files, etc, and instead only take slices of u8.

I haven't fully thought about it, but maybe Uninitialized::new should take a trait instead?

[jprendes]

IIRC, @simongdavies had an idea where the host could share host memory with the VM by adding pages to the VM's page table. Would something like that help in this case?

[danbugs]

The PR comment states that the this is a non-breaking change, but the rename GuestBinary -> GuestBlob is breaking, right? Also I do think the renaming is not intuitive for users who want to load a guest binary, when it's now called GuestBlob

On the breaking-change point, you're right. Guess what I meant to say is that you don't necessarily have to pass a GuestEnvironment to create a new uninit sandbox because you can still create it off a binary alone.

Not strictly related to your PR, but I think we should consider not allowing passing blobs/binaries by "path", which would force the consumers to do their own reading of files, etc, and instead only take slices of u8.

If so, I can change GuestBlob to be just bytes and revert the re-name of the GuestBinary. I opted to re-name because they were the same and I felt like GuestBlob also encompassed a binary.

I haven't fully thought about it, but maybe Uninitialized::new should take a trait instead?

It does currently take a trait: env: impl Into<GuestEnvironment<'a>>,

[ludfjig]

I haven't fully thought about it, but maybe Uninitialized::new should take a trait instead?

It does currently take a trait: env: impl Into<GuestEnvironment<'a>>,

What do you think about letting Uninitialized::new take something like a &impl GuestEnvironment

pub trait GuestEnvironment {
    /// Get the guest binary
    fn guest_binary(&self) -> &[u8];

    /// Get optional memory blobs
    fn blobs(&self) -> Option<&[&[u8]]>;
}

Then we could implement it for exisiting GuestBinary, and also a new GuestBinaryWithBlob, etc, etc.

[danbugs]

I haven't fully thought about it, but maybe Uninitialized::new should take a trait instead?

It does currently take a trait: env: impl Into<GuestEnvironment<'a>>,

What do you think about letting Uninitialized::new take something like a &impl GuestEnvironment

pub trait GuestEnvironment {
    /// Get the guest binary as a `GuestBinary`.
    fn guest_binary(&self) -> GuestBinary;

    /// Get optional memory blobs
    fn blob(&self) -> Option<&[&[u8]]>;
}

Then we could implement it for exisiting GuestBinary, and also a new GuestBinaryWithBlob, etc, etc.

I think I like impl Into<GuestEnvironment<'a>> a little better so we don't have to write empty implementations (e.g., fn blob for GuestBinary) and grow the trait and its deps if we want to support extra items in a guest environment, but I'm not strongly opinionated here. Open to change if you think this has other pros I might be missing 🙂

[ludfjig]

I haven't fully thought about it, but maybe Uninitialized::new should take a trait instead?

It does currently take a trait: env: impl Into<GuestEnvironment<'a>>,

What do you think about letting Uninitialized::new take something like a &impl GuestEnvironment

pub trait GuestEnvironment {
    /// Get the guest binary as a `GuestBinary`.
    fn guest_binary(&self) -> GuestBinary;

    /// Get optional memory blobs
    fn blob(&self) -> Option<&[&[u8]]>;
}

Then we could implement it for exisiting GuestBinary, and also a new GuestBinaryWithBlob, etc, etc.

I think I like impl Into<GuestEnvironment<'a>> a little better so we don't have to write empty implementations (e.g., fn blob for GuestBinary) and grow the trait and its deps if we want to support extra items in a guest environment, but I'm not strongly opinionated here. Open to change if you think this has other pros I might be missing 🙂

@simongdavies @jprendes @dblnz @andreiltd @syntactically, any opinions on the API of this PR?

[simongdavies]

Looks good just a few questions

[simongdavies]

IIRC, @simongdavies had an idea where the host could share host memory with the VM by adding pages to the VM's page table. Would something like that help in this case?

I don't recall what that might have been, but I think if @danbugs takes up my feedback about allowing the data to have RWX flags associated with it then if its R we could do an optimisation where its only allocated in the host once, that would require us to change the way we bounds check though so I think out of scope here.

We also talked about allowing a user to pass an vector of (address,len) into a sandbox for mapping, that may be a better way to do something like this, and it may fit better in a configuration than as a argument to the new function

[simongdavies]

Not strictly related to your PR, but I think we should consider not allowing passing blobs/binaries by "path", which would force the consumers to do their own reading of files, etc, and instead only take slices of u8.

I think this all stemmed from us having in-proc and more specifically loadlib support in Windows, I think you are correct, I don't think we need it any longer ,not sure if now is the right time though...

[simongdavies]

any opinions on the API of this PR?

I quite like it as it is but not really that fussed either way, eventually we should bury all this behind a builder, that might be the right time to get rid of the paths

[danbugs]

I don't recall what that might have been, but I think if @danbugs takes up my feedback about allowing the data to have RWX flags associated with it then if its R we could do an optimisation where its only allocated in the host once, that would require us to change the way we bounds check though so I think out of scope here.

Added configurable flags to the new mem region 👍

[danbugs]

I think this all stemmed from us having in-proc and more specifically loadlib support in Windows, I think you are correct, I don't think we need it any longer ,not sure if now is the right time though...

#614 ~ made an issue for this, @ludfjig / @simongdavies