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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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

~Andrew