All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Xen-devel <xen-devel@lists.xenproject.org>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Christian Lindig" <christian.lindig@citrix.com>,
	"David Scott" <dave@recoil.org>,
	"Edwin Török" <edwin.torok@cloud.com>,
	"Rob Hoes" <Rob.Hoes@citrix.com>
Subject: [PATCH 6/7] tools/ocaml/xc: Don't reference Abstract_Tag objects with the GC lock released
Date: Tue, 31 Jan 2023 21:29:12 +0000	[thread overview]
Message-ID: <20230131212913.6199-7-andrew.cooper3@citrix.com> (raw)
In-Reply-To: <20230131212913.6199-1-andrew.cooper3@citrix.com>

The intf->{addr,len} references in the xc_map_foreign_range() call are unsafe.
From the manual:

  https://ocaml.org/manual/intfc.html#ss:parallel-execution-long-running-c-code

"After caml_release_runtime_system() was called and until
caml_acquire_runtime_system() is called, the C code must not access any OCaml
data, nor call any function of the run-time system, nor call back into OCaml
code."

More than what the manual says, the intf pointer is (potentially) invalidated
by caml_enter_blocking_section() if another thread happens to perform garbage
collection at just the right (wrong) moment.

Rewrite the logic.  There's no need to stash data in the Ocaml object until
the success path at the very end.

Fixes: 8b7ce06a2d34 ("ocaml: Add XC bindings.")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Christian Lindig <christian.lindig@citrix.com>
CC: David Scott <dave@recoil.org>
CC: Edwin Török <edwin.torok@cloud.com>
CC: Rob Hoes <Rob.Hoes@citrix.com>

Note: the mmap stub has a similar pattern when constructing a mmap_interface,
but, but it's not actually unsafe because it doesn't drop the GC lock.

_H() is buggy too, but this patch needs backporting further than that fix.
---
 tools/ocaml/libs/xc/xenctrl_stubs.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index 291663bb278a..e5277f6f19a2 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -1028,26 +1028,25 @@ CAMLprim value stub_map_foreign_range(value xch, value dom,
 	CAMLparam4(xch, dom, size, mfn);
 	CAMLlocal1(result);
 	struct mmap_interface *intf;
-	uint32_t c_dom;
-	unsigned long c_mfn;
+	unsigned long c_mfn = Nativeint_val(mfn);
+	int len = Int_val(size);
+	void *ptr;
 
 	BUILD_BUG_ON((sizeof(struct mmap_interface) % sizeof(value)) != 0);
 	result = caml_alloc(Wsize_bsize(sizeof(struct mmap_interface)),
 			    Abstract_tag);
 
-	intf = (struct mmap_interface *) result;
-
-	intf->len = Int_val(size);
-
-	c_dom = _D(dom);
-	c_mfn = Nativeint_val(mfn);
 	caml_enter_blocking_section();
-	intf->addr = xc_map_foreign_range(_H(xch), c_dom,
-	                                  intf->len, PROT_READ|PROT_WRITE,
-	                                  c_mfn);
+	ptr = xc_map_foreign_range(_H(xch), _D(dom), len,
+				   PROT_READ|PROT_WRITE, c_mfn);
 	caml_leave_blocking_section();
-	if (!intf->addr)
+
+	if (!ptr)
 		caml_failwith("xc_map_foreign_range error");
+
+	intf = Data_abstract_val(result);
+	*intf = (struct mmap_interface){ ptr, len };
+
 	CAMLreturn(result);
 }
 
-- 
2.11.0



  parent reply	other threads:[~2023-01-31 21:29 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-31 21:29 [PATCH 0/7] tools/ocaml: Memory corruption fixes in bindings Andrew Cooper
2023-01-31 21:29 ` [PATCH 1/7] tools/ocaml/libs: Don't declare stubs as taking void Andrew Cooper
2023-01-31 21:29 ` [PATCH 2/7] tools/ocaml/libs: Allocate the correct amount of memory for Abstract_tag Andrew Cooper
2023-01-31 21:29 ` [PATCH 3/7] tools/ocaml/evtchn: Don't reference Custom objects with the GC lock released Andrew Cooper
2023-01-31 21:29 ` [PATCH 4/7] tools/ocaml/evtchn: Misc cleanup Andrew Cooper
2023-01-31 21:29 ` [PATCH 5/7] tools/ocaml/xc: Fix binding for xc_domain_assign_device() Andrew Cooper
2023-01-31 21:29 ` Andrew Cooper [this message]
2023-01-31 21:29 ` [PATCH 7/7] tools/ocaml/xc: Don't reference Custom objects with the GC lock released Andrew Cooper
2023-02-01  9:04 ` [PATCH 0/7] tools/ocaml: Memory corruption fixes in bindings Christian Lindig

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230131212913.6199-7-andrew.cooper3@citrix.com \
    --to=andrew.cooper3@citrix.com \
    --cc=Rob.Hoes@citrix.com \
    --cc=christian.lindig@citrix.com \
    --cc=dave@recoil.org \
    --cc=edwin.torok@cloud.com \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.