Re: [MirageOS-devel] Unmapping grants is unsafe

[Thomas Leonard <talex5@xxxxxxxxx>]

On 15 January 2016 at 16:15, Anil Madhavapeddy <anil@xxxxxxxxxx> wrote:
>
>> On 4 Jan 2016, at 14:26, Thomas Leonard <talex5@xxxxxxxxx> wrote:
>>
>> On 3 January 2016 at 10:38, Thomas Leonard <talex5@xxxxxxxxx> wrote:
>>> On 2 January 2016 at 14:35, Mindy <mindy@xxxxxxxxxxxxxxxxxxx> wrote:
>>>> On 01/02/2016 12:14 PM, Anil Madhavapeddy wrote:
>>>>>
>>>>> On 2 Jan 2016, at 12:07, Thomas Leonard <talex5@xxxxxxxxx> wrote:
>>>>>>
>>>>>> I have a Mirage firewall running now under Qubes:
>>>>>>
>>>>>>
>>>>>> http://roscidus.com/blog/blog/2016/01/01/a-unikernel-firewall-for-qubesos/
>> [...]
>>>>>> - Mirage allows you to access memory after unmapping it. This isn't
>>>>>> safe (may page-fault or give access to someone else's data). I've
>>>>>> added checks in mirage-net-xen to make sure we don't try to access a
>>>>>> ring after unmapping it [1], but I think this should be fixed at the
>>>>>> Bigarray level [2].
>>>>>>
>>>>> Did that Bigarray patch get committed into OCaml 4.03dev?  I dont
>>>>> have a checkout handy here and it's hard to map a commit to the
>>>>> associated PRs in the web UI.
>>>
>>> I haven't submitted a PR yet. Need to update it to master and test.
>>
>> https://github.com/ocaml/ocaml/pull/389
>>
>> However, they're not keen on merging it, for performance reasons.
>> Also, it would mean changing Cstruct, which currently says:
>>
>>> We try to maintain the property that no constructed [t] can ever point out 
>>> of
>>> its underlying buffer. This property is guarded by all of the constructing
>>> functions and the fact that the type is private, and used by various
>>> functions that would otherwise be completely unsafe.
>>
>> I wonder if we should instead change Gnt.Gnttab.Local_mapping.to_buf
>> so that instead of returning an Io_page it returns something else,
>> which provides similar operations but:
>>
>> a) checks every access to ensure it hasn't been unmapped, and
>> b) doesn't allow you to get a Cstruct or Bigarray from it.
>>
>> However, this would mean a largish API change.
>
> Before going through the epic amount of work that an API change would make in 
> upstream libraries, perhaps we should build a small unit test that rapidly 
> does a simple data copy across domains and benchmark it?
>
> I think any new API should do very explicit lifetimes for the grant buffers 
> (i.e. not depend on the GC to cleanup), but also minimise copying the data if 
> at all possible.  While it's dangerous to read from a shared memory buffer if 
> you don't trust the other side, an enforced copy does make it hard to build 
> efficient unikernel proxies that are just transferring data across shared 
> rings.

A simpler solution might be to rename [to_buf] to [to_buf_unsafe] and
provide a safe [blit] operation (that fails if the buffer is no longer
mapped). The block and net drivers already copy, so they could use the
safe interface.

shared-memory-ring currently reads directly, but maybe it shouldn't
anyway (whether frontend or backend) to avoid TOCTOU issues.


-- 
Dr Thomas Leonard        http://roscidus.com/blog/
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

_______________________________________________
MirageOS-devel mailing list
MirageOS-devel@xxxxxxxxxxxxxxxxxxxx
http://lists.xenproject.org/cgi-bin/mailman/listinfo/mirageos-devel