Re: [PATCH] tools/ocaml/xenctrl: OCaml 5 support, fix use-after-free

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 24/11/2022 14:03, Edwin Torok wrote:
>
>> On 24 Nov 2022, at 13:43, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> wrote:
>>
>> On 24/11/2022 09:03, Edwin Torok wrote:
>>>> On 23 Nov 2022, at 22:25, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>
>>>> The binding for xc_interface_close() free the underlying handle while 
>>>> leaving
>>>> the Ocaml object still in scope and usable.  This would make it easy to 
>>>> suffer
>>>> a use-after-free, if it weren't for the fact that the typical usage is as a
>>>> singleton that lives for the lifetime of the program.
>>>>
>>>> Ocaml 5 no longer permits storing a naked C pointer in an Ocaml value.
>>>>
>>>> Therefore, use a Custom block.  This allows us to use the finaliser 
>>>> callback
>>>> to call xc_interface_close(), if the Ocaml object goes out of scope.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>> ---
>>>> CC: Christian Lindig <christian.lindig@xxxxxxxxxx>
>>>> CC: David Scott <dave@xxxxxxxxxx>
>>>> CC: Edwin Torok <edvin.torok@xxxxxxxxxx>
>>>> CC: Rob Hoes <Rob.Hoes@xxxxxxxxxx>
>>>>
>>>> I've confirmed that Xenctrl.close_handle does cause the finaliser to be
>>>> called, simply by dropping the handle reference.
>>> Thanks, a good way to test this is with OCAMLRUNPARAM=c, possible under 
>>> valgrind, which causes all finalisers to be called on exit
>>> (normally they are not because the program is exiting anyway)
>> I do that anyway, but it's not relevant here.
>>
>> What matters is checking that calling close_handle releases the object
>> (albeit with a forced GC sweep) before the program ends.
>>
>>>> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c 
>>>> b/tools/ocaml/libs/xc/xenctrl_stubs.c
>>>> index f37848ae0bb3..4e1204085422 100644
>>>> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
>>>> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
>>>> @@ -37,13 +37,28 @@
>>>>
>>>> #include "mmap_stubs.h"
>>>>
>>>> -#define _H(__h) ((xc_interface *)(__h))
>>>> +#define _H(__h) (*((xc_interface **)Data_custom_val(__h)))
>>>> #define _D(__d) ((uint32_t)Int_val(__d))
>>> I think this requires an update in xenopsd too to match, otherwise it'll 
>>> crash:
>>> https://github.com/xapi-project/xenopsd/blob/master/c_stubs/xenctrlext_stubs.c#L32
>> Urgh.  I'll take a note to do that when bringing in the change.
>>
>>> This wasn't an issue with the original patch which used Data_abstract_val 
>>> here, because
>>> that (currently) happens to boil down to just a cast (with some GC metadata 
>>> *before* it),
>>> so the old way of just casting OCaml value to C pointer still worked.
>>>
>>> However Data_custom_val boils down to accessing a value at +sizeof(value) 
>>> offset,
>>> so xenopsd would now read the wrong pointer.
>>> Perhaps it would've been better to have this _H defined in some header, 
>>> otherwise extending Xenctrl the way xenopsd does it is quite brittle.
>> Exporting _H won't help because everything is statically built. 
>
> As long as you don't rebuilt xenopsd you will keep using the old C stubs that 
> xenopsd got compiled with, the change in Xen will have no effect until 
> xenopsd is recompiled,
> at which point it could pick up the new _H if available, but I agree with 
> your point below.
>
>
>> It's
>> brittle because xenopsd has got a local piece of C playing around with
>> the internals of someone else's library.  This violates more rules than
>> I care to list.
>>
>> We (XenServer) should definitely work to improve things, but this is
>> entirely a xenopsd problem, not an upstream Xen problem.
>
> It is a lot easier to add new xenctrl bindings and test them out in xenopsd 
> than it is to add them to Xen.
> We should try to either upstream all xenopsd xenctrl bindings to Xen, and 
> make it easier to add them to Xen going forward;
> or move all the Xenctrl bindings to xenopsd, then at least you only need to 
> rebuild the one piece you are changing.
>
> But to do the latter we first need to get everything that relies on xenctrl 
> to move to more stable interfaces (including oxenstored).
> There are some Mirage libraries as well that use xenctrl, when a more 
> suitable stable interface exists in newer versions of Xen that they should 
> use instead.
>
> Perhaps a compromise between the 2 extremes would be for xenopsd to open and 
> have its own xenctrl handle, even if that leads to some code duplication, it 
> would at least not rely on an undocumented and unstable internal detail of an 
> already unstable ABI. And that would still allow xenopsd to extend xenctrl 
> with bindings that are not (yet) present in Xen.
> What do you think?

Many of these problems will disappear with a stable tools interface. 
But yes, in the short term, xcext opening its own handle would
definitely improve things by keeping the two sets of bindings separate.

~Andrew