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

[Edwin Torok <edvin.torok@xxxxxxxxxx>]

> 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?

Best regards,
--Edwin