[Xen-devel] [PATCH 0/8] Fix build with using OCaml 4.06.1 and -safe-string

[Xen-devel] [PATCH 0/8] Fix build with using OCaml 4.06.1 and -safe-string

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Hi all,

This series is meant to solve the build issue reported by Dario when
using recent version of OCaml and -safe-string.

I took the opportunity to harden a bit more the code by using const more
often.

This series was only build tested.

Cheers,

Julien Grall (8):
  xen/guest_access: Harden copy_to_guest_offset to prevent const dest
    operand
  xen/public: sysctl: set_parameter.params and debug.keys should be
    const
  tools/libxc: misc: Mark const the parameter 'keys' of
    xc_send_debug_keys()
  tools/libxc: misc: Mark const the parameter 'params' of
    xc_set_parameters()
  tools/ocaml: libxc: Check error return in stub_xc_vcpu_context_get()
  tools/ocaml: libxb: Harden stub_header_of_string()
  tools/ocaml: libxb: Avoid to use String_val() when value is bytes
  tools/ocaml: Fix stubs build when OCaml has been compiled with
    -safe-string

 tools/libxc/include/xenctrl.h       |  4 ++--
 tools/libxc/xc_misc.c               |  8 ++++----
 tools/libxc/xc_private.h            |  8 ++++++++
 tools/ocaml/libs/xb/xenbus_stubs.c  |  6 +++---
 tools/ocaml/libs/xb/xs_ring_stubs.c | 12 ++++++++++--
 tools/ocaml/libs/xc/xenctrl_stubs.c |  6 ++++--
 xen/include/asm-arm/guest_access.h  |  2 +-
 xen/include/asm-x86/guest_access.h  |  2 +-
 xen/include/public/sysctl.h         |  4 ++--
 9 files changed, 35 insertions(+), 17 deletions(-)

-- 
2.17.1

[Xen-devel] [PATCH 1/8] xen/guest_access: Harden copy_to_guest_offset to prevent const dest operand

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

At the moment, copy_to_guest_offset() will allow the hypervisor to copy
data to guest handle marked const.

Thankfully, no users of the helper will do that. Rather than hoping this
can be caught during review, harden copy_to_guest_offset() so the build
will fail if such users are introduced.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/include/asm-arm/guest_access.h | 2 +-
 xen/include/asm-x86/guest_access.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/include/asm-arm/guest_access.h b/xen/include/asm-arm/guest_access.h
index 8997a1cbfe..ff2eec237d 100644
--- a/xen/include/asm-arm/guest_access.h
+++ b/xen/include/asm-arm/guest_access.h
@@ -126,7 +126,7 @@ int access_guest_memory_by_ipa(struct domain *d, paddr_t ipa, void *buf,
 
 #define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
     const typeof(*(ptr)) *_s = (ptr);                   \
-    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
+    typeof(*((hnd).p)) *_d = (hnd).p;                   \
     ((void)((hnd).p == (ptr)));                         \
     __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
 })
diff --git a/xen/include/asm-x86/guest_access.h b/xen/include/asm-x86/guest_access.h
index ca700c959a..2693c6540b 100644
--- a/xen/include/asm-x86/guest_access.h
+++ b/xen/include/asm-x86/guest_access.h
@@ -86,7 +86,7 @@
  */
 #define copy_to_guest_offset(hnd, off, ptr, nr) ({      \
     const typeof(*(ptr)) *_s = (ptr);                   \
-    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
+    typeof(*((hnd).p)) *_d = (hnd).p;                   \
     ((void)((hnd).p == (ptr)));                         \
     raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));  \
 })
-- 
2.17.1

[Xen-devel] [PATCH 2/8] xen/public: sysctl: set_parameter.params and debug.keys should be const

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

The fields set_parameter.params and debug.keys should never be modified
by the hypervisor. So mark them as const.

Signed-off-by: Julien Grall <jgrall@amazon.com>

---

I am not entirely sure whether we should bump the systcl version for
this change. Any thoughts?
---
 xen/include/public/sysctl.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 3d72fab49f..3a08c512e8 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -170,7 +170,7 @@ struct xen_sysctl_getdomaininfolist {
 /* XEN_SYSCTL_debug_keys */
 struct xen_sysctl_debug_keys {
     /* IN variables. */
-    XEN_GUEST_HANDLE_64(char) keys;
+    XEN_GUEST_HANDLE_64(const_char) keys;
     uint32_t nr_keys;
 };
 
@@ -1037,7 +1037,7 @@ struct xen_sysctl_livepatch_op {
  */
 
 struct xen_sysctl_set_parameter {
-    XEN_GUEST_HANDLE_64(char) params;       /* IN: pointer to parameters. */
+    XEN_GUEST_HANDLE_64(const_char) params; /* IN: pointer to parameters. */
     uint16_t size;                          /* IN: size of parameters. */
     uint16_t pad[3];                        /* IN: MUST be zero. */
 };
-- 
2.17.1

[Xen-devel] [PATCH 3/8] tools/libxc: misc: Mark const the parameter 'keys' of xc_send_debug_keys()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

OCaml is using a string to describe the parameter 'keys' of
xc_send_debug_keys(). Since Ocaml 4.06.01, String_val() will return a
const char * when using -safe-string. This will result to a build
failure because xc_send_debug_keys() expects a char *.

The function should never modify the parameter 'keys' and therefore the
parameter should be const. Unfortunately, this is not directly possible
because DECLARE_HYPERCALL_BOUNCE() is expecting a non-const variable.

A new macro DECLARE_HYPERCALL_BOUNCE_IN() is introduced and will take
care of const parameter. The first user will be xc_send_debug_keys() but
this can be used in more place in the future.

Reported-by: Dario Faggioli <dfaggioli@suse.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 tools/libxc/include/xenctrl.h | 2 +-
 tools/libxc/xc_misc.c         | 4 ++--
 tools/libxc/xc_private.h      | 8 ++++++++
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index fc6e57a1a0..d8874eb846 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1225,7 +1225,7 @@ int xc_readconsolering(xc_interface *xch,
                        unsigned int *pnr_chars,
                        int clear, int incremental, uint32_t *pindex);
 
-int xc_send_debug_keys(xc_interface *xch, char *keys);
+int xc_send_debug_keys(xc_interface *xch, const char *keys);
 int xc_set_parameters(xc_interface *xch, char *params);
 
 typedef struct xen_sysctl_physinfo xc_physinfo_t;
diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index 093fa44081..957c03415c 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -167,11 +167,11 @@ int xc_readconsolering(xc_interface *xch,
     return ret;
 }
 
-int xc_send_debug_keys(xc_interface *xch, char *keys)
+int xc_send_debug_keys(xc_interface *xch, const char *keys)
 {
     int ret, len = strlen(keys);
     DECLARE_SYSCTL;
-    DECLARE_HYPERCALL_BOUNCE(keys, len, XC_HYPERCALL_BUFFER_BOUNCE_IN);
+    DECLARE_HYPERCALL_BOUNCE_IN(keys, len);
 
     if ( xc_hypercall_bounce_pre(xch, keys) )
         return -1;
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index adc3b6a571..c77edb3c4c 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -181,6 +181,14 @@ enum {
  */
 #define DECLARE_HYPERCALL_BOUNCE(_ubuf, _sz, _dir) DECLARE_NAMED_HYPERCALL_BOUNCE(_ubuf, _ubuf, _sz, _dir)
 
+/*
+ * Declare a bounce buffer shadowing the named user data pointer that
+ * cannot be modified.
+ */
+#define DECLARE_HYPERCALL_BOUNCE_IN(_ubuf, _sz)                     \
+    DECLARE_NAMED_HYPERCALL_BOUNCE(_ubuf, (void *)(_ubuf), _sz,     \
+                                   XC_HYPERCALL_BUFFER_BOUNCE_IN)
+
 /*
  * Set the size of data to bounce. Useful when the size is not known
  * when the bounce buffer is declared.
-- 
2.17.1

[Xen-devel] [PATCH 4/8] tools/libxc: misc: Mark const the parameter 'params' of xc_set_parameters()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

The parameter 'params' of xc_set_parameters() should never be modified.
So mark it as const.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 tools/libxc/include/xenctrl.h | 2 +-
 tools/libxc/xc_misc.c         | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index d8874eb846..58fa931de1 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1226,7 +1226,7 @@ int xc_readconsolering(xc_interface *xch,
                        int clear, int incremental, uint32_t *pindex);
 
 int xc_send_debug_keys(xc_interface *xch, const char *keys);
-int xc_set_parameters(xc_interface *xch, char *params);
+int xc_set_parameters(xc_interface *xch, const char *params);
 
 typedef struct xen_sysctl_physinfo xc_physinfo_t;
 typedef struct xen_sysctl_cputopo xc_cputopo_t;
diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index 957c03415c..fe477bf344 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -187,11 +187,11 @@ int xc_send_debug_keys(xc_interface *xch, const char *keys)
     return ret;
 }
 
-int xc_set_parameters(xc_interface *xch, char *params)
+int xc_set_parameters(xc_interface *xch, const char *params)
 {
     int ret, len = strlen(params);
     DECLARE_SYSCTL;
-    DECLARE_HYPERCALL_BOUNCE(params, len, XC_HYPERCALL_BUFFER_BOUNCE_IN);
+    DECLARE_HYPERCALL_BOUNCE_IN(params, len);
 
     if ( xc_hypercall_bounce_pre(xch, params) )
         return -1;
-- 
2.17.1

[Xen-devel] [PATCH 5/8] tools/ocaml: libxc: Check error return in stub_xc_vcpu_context_get()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

xc_vcpu_getcontext() may fail to retrieve the vcpu context. Rather than
ignoring the return value, check it and throw an error if needed.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 tools/ocaml/libs/xc/xenctrl_stubs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index 904da45c4f..0fdbeac158 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -497,6 +497,8 @@ CAMLprim value stub_xc_vcpu_context_get(value xch, value domid,
 	vcpu_guest_context_any_t ctxt;
 
 	ret = xc_vcpu_getcontext(_H(xch), _D(domid), Int_val(cpu), &ctxt);
+	if ( ret < 0 )
+		failwith_xc(_H(xch));
 
 	context = caml_alloc_string(sizeof(ctxt));
 	memcpy(String_val(context), (char *) &ctxt.c, sizeof(ctxt.c));
-- 
2.17.1

[Xen-devel] [PATCH 6/8] tools/ocaml: libxb: Harden stub_header_of_string()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

stub_header_of_string() should not modify the header. So mark the
variable 'hdr' as const.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 tools/ocaml/libs/xb/xenbus_stubs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/ocaml/libs/xb/xenbus_stubs.c b/tools/ocaml/libs/xb/xenbus_stubs.c
index a68e783f70..001bb03371 100644
--- a/tools/ocaml/libs/xb/xenbus_stubs.c
+++ b/tools/ocaml/libs/xb/xenbus_stubs.c
@@ -40,12 +40,12 @@ CAMLprim value stub_header_of_string(value s)
 {
 	CAMLparam1(s);
 	CAMLlocal1(ret);
-	struct xsd_sockmsg *hdr;
+	const struct xsd_sockmsg *hdr;
 
 	if (caml_string_length(s) != sizeof(struct xsd_sockmsg))
 		caml_failwith("xb header incomplete");
 	ret = caml_alloc_tuple(4);
-	hdr = (struct xsd_sockmsg *) String_val(s);
+	hdr = (const struct xsd_sockmsg *) String_val(s);
 	Store_field(ret, 0, Val_int(hdr->tx_id));
 	Store_field(ret, 1, Val_int(hdr->req_id));
 	Store_field(ret, 2, Val_int(hdr->type));
-- 
2.17.1

[Xen-devel] [PATCH 7/8] tools/ocaml: libxb: Avoid to use String_val() when value is bytes

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Commit ec7d54dd1a "ocaml/libs/xb: Use bytes in place of strings for
mutable buffers" switch mutable buffers from string to bytes. However
the C code were still using String_Val() to access them.

While the underlying structure is the same between string and bytes, a
string is meant to be immutable. OCaml 4.06.1 and later will enforce it.
Therefore, it will not be possible to build the OCaml libs when using
-safe-string. This is because String_val() will return a const value.

To avoid plain cast in the code, the code is now switched to use
Bytes_val(). As the macro is not defined in older OCaml version, we need
to provide a stub.

Take the opportunity to switch to const the buffer in
ml_interface_write() as it should not be modified.

Reported-by: Dario Faggioli <dfaggioli@suse.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 tools/ocaml/libs/xb/xs_ring_stubs.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/ocaml/libs/xb/xs_ring_stubs.c b/tools/ocaml/libs/xb/xs_ring_stubs.c
index 473787064a..7537a23949 100644
--- a/tools/ocaml/libs/xb/xs_ring_stubs.c
+++ b/tools/ocaml/libs/xb/xs_ring_stubs.c
@@ -36,6 +36,14 @@
 
 #define GET_C_STRUCT(a) ((struct mmap_interface *) a)
 
+/*
+ * Bytes_val has been introduced by Ocaml 4.06.1. So define our own version
+ * if needed.
+ */
+#ifndef Bytes_val
+#define Bytes_val(x) ((unsigned char *) Bp_val(x))
+#endif
+
 CAMLprim value ml_interface_read(value ml_interface,
                                  value ml_buffer,
                                  value ml_len)
@@ -44,7 +52,7 @@ CAMLprim value ml_interface_read(value ml_interface,
 	CAMLlocal1(ml_result);
 
 	struct mmap_interface *interface = GET_C_STRUCT(ml_interface);
-	char *buffer = String_val(ml_buffer);
+	unsigned char *buffer = Bytes_val(ml_buffer);
 	int len = Int_val(ml_len);
 	int result;
 
@@ -103,7 +111,7 @@ CAMLprim value ml_interface_write(value ml_interface,
 	CAMLlocal1(ml_result);
 
 	struct mmap_interface *interface = GET_C_STRUCT(ml_interface);
-	char *buffer = String_val(ml_buffer);
+	const unsigned char *buffer = Bytes_val(ml_buffer);
 	int len = Int_val(ml_len);
 	int result;
 
-- 
2.17.1

[Xen-devel] [PATCH 8/8] tools/ocaml: Fix stubs build when OCaml has been compiled with -safe-string

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

The OCaml code has been fixed to handle properly -safe-string in Xen
4.11, however the stubs part were missed.

On OCaml newer than 4.06.1, String_Val() will return a const char *
when using -safe-string leading to build failure when this is used
in place where char * is expected.

The main use in Xen code base is when a new string is allocated. The
suggested approach by the OCaml community [1] is to use the helper
caml_alloc_initialized_string() but it was introduced by OCaml 4.06.1.

The next best approach is to cast String_val() to (char *) as the helper
would have done. So use it when we need to update the new string using
memcpy().

Take the opportunity to remove the unnecessary cast of the source as
mempcy() is expecting a void *.

[1] https://github.com/ocaml/ocaml/pull/1274

Reported-by: Dario Faggioli <dfaggioli@suse.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>

---

    I thought about detecting whether caml_alloc_initialized_string()
    strings exist and implement it if not. This requires a bit more
    work, so I would like to get feedback first whether this is worth
    it.
---
 tools/ocaml/libs/xb/xenbus_stubs.c  | 2 +-
 tools/ocaml/libs/xc/xenctrl_stubs.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/ocaml/libs/xb/xenbus_stubs.c b/tools/ocaml/libs/xb/xenbus_stubs.c
index 001bb03371..3065181a55 100644
--- a/tools/ocaml/libs/xb/xenbus_stubs.c
+++ b/tools/ocaml/libs/xb/xenbus_stubs.c
@@ -65,7 +65,7 @@ CAMLprim value stub_string_of_header(value tid, value rid, value ty, value len)
 	};
 
 	ret = caml_alloc_string(sizeof(struct xsd_sockmsg));
-	memcpy(String_val(ret), &xsd, sizeof(struct xsd_sockmsg));
+	memcpy((char *) String_val(ret), &xsd, sizeof(struct xsd_sockmsg));
 
 	CAMLreturn(ret);
 }
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index 0fdbeac158..94aba38a42 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -501,7 +501,7 @@ CAMLprim value stub_xc_vcpu_context_get(value xch, value domid,
 		failwith_xc(_H(xch));
 
 	context = caml_alloc_string(sizeof(ctxt));
-	memcpy(String_val(context), (char *) &ctxt.c, sizeof(ctxt.c));
+	memcpy((char *) String_val(context), &ctxt.c, sizeof(ctxt.c));
 
 	CAMLreturn(context);
 }
@@ -680,7 +680,7 @@ CAMLprim value stub_xc_readconsolering(value xch)
 		conring_size = size;
 
 	ring = caml_alloc_string(count);
-	memcpy(String_val(ring), str, count);
+	memcpy((char *) String_val(ring), str, count);
 	free(str);
 
 	CAMLreturn(ring);
-- 
2.17.1

Re: [PATCH 1/8] xen/guest_access: Harden copy_to_guest_offset to prevent const dest operand

[Jan Beulich]

On 30.03.2020 21:21, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment, copy_to_guest_offset() will allow the hypervisor to copy
> data to guest handle marked const.
> 
> Thankfully, no users of the helper will do that. Rather than hoping this
> can be caught during review, harden copy_to_guest_offset() so the build
> will fail if such users are introduced.

But there are other implications you break:

> --- a/xen/include/asm-arm/guest_access.h
> +++ b/xen/include/asm-arm/guest_access.h
> @@ -126,7 +126,7 @@ int access_guest_memory_by_ipa(struct domain *d, paddr_t ipa, void *buf,
>  
>  #define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
>      const typeof(*(ptr)) *_s = (ptr);                   \
> -    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
> +    typeof(*((hnd).p)) *_d = (hnd).p;                   \
>      ((void)((hnd).p == (ptr)));                         \
>      __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\

Until this change, it is "ptr" which all sizes get derived from,
i.e. it is the internally used type rather than the handle type
which controls this. I'm sure we use this in a few places, to
copy to e.g. a handle derived from "void". Compatibility of types
(disallowing other than void) is checked by the comparison on the
line immediately after the line you change. Yes "_d+(off)" right
above here then changes its result. I consider it pretty likely
you'd notice this issue once you go beyond just build testing.

To address this, I guess we need to find an expression along the
lines of that comparison, which does not cause any code to be
generated, but which verifies the properties we care about. The
line you change should be left alone, from all I can tell right
now.

Jan

Re: [PATCH 2/8] xen/public: sysctl: set_parameter.params and debug.keys should be const

[Jan Beulich]

On 30.03.2020 21:21, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The fields set_parameter.params and debug.keys should never be modified
> by the hypervisor. So mark them as const.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> I am not entirely sure whether we should bump the systcl version for
> this change. Any thoughts?

No, it should be left as is - it's about _binary_ compatibility (e.g.
if structure layout changes, or a sub-function gets dropped). The
need to potentially address build issues resulting from changes like
the one here isn't covered by it, but by the __XEN__ / __XEN_TOOLS__
conditional at the top of the header.

Jan

Re: [PATCH 3/8] tools/libxc: misc: Mark const the parameter 'keys' of xc_send_debug_keys()

[Ian Jackson]

Julien Grall writes ("[PATCH 3/8] tools/libxc: misc: Mark const the parameter 'keys' of xc_send_debug_keys()"):
> From: Julien Grall <jgrall@amazon.com>
> 
> OCaml is using a string to describe the parameter 'keys' of
> xc_send_debug_keys(). Since Ocaml 4.06.01, String_val() will return a
> const char * when using -safe-string. This will result to a build
> failure because xc_send_debug_keys() expects a char *.
> 
> The function should never modify the parameter 'keys' and therefore the
> parameter should be const. Unfortunately, this is not directly possible
> because DECLARE_HYPERCALL_BOUNCE() is expecting a non-const variable.
> 
> A new macro DECLARE_HYPERCALL_BOUNCE_IN() is introduced and will take
> care of const parameter. The first user will be xc_send_debug_keys() but
> this can be used in more place in the future.
> 
> Reported-by: Dario Faggioli <dfaggioli@suse.com>
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH 4/8] tools/libxc: misc: Mark const the parameter 'params' of xc_set_parameters()

[Ian Jackson]

Julien Grall writes ("[PATCH 4/8] tools/libxc: misc: Mark const the parameter 'params' of xc_set_parameters()"):
> From: Julien Grall <jgrall@amazon.com>
> 
> The parameter 'params' of xc_set_parameters() should never be modified.
> So mark it as const.

Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH 0/8] Fix build with using OCaml 4.06.1 and -safe-string

[Ian Jackson]

Julien Grall writes ("[PATCH 0/8] Fix build with using OCaml 4.06.1 and -safe-string"):
> This series is meant to solve the build issue reported by Dario when
> using recent version of OCaml and -safe-string.

Thanks.  I have reviewed the C tools parts here.  I think the ocaml
parts ought to have a review from someone familiar with the ocaml FFI.

> I took the opportunity to harden a bit more the code by using const more
> often.

I approve.

Perhaps we should start building our C code with -Wwrite-strings,
which makes "" have type const char* ?  Result would be a giant
constification patch, probably.

Ian.

Re: [PATCH 1/8] xen/guest_access: Harden copy_to_guest_offset to prevent const dest operand

[Julien Grall]

Hi Jan,

On 31/03/2020 08:26, Jan Beulich wrote:
> On 30.03.2020 21:21, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> At the moment, copy_to_guest_offset() will allow the hypervisor to copy
>> data to guest handle marked const.
>>
>> Thankfully, no users of the helper will do that. Rather than hoping this
>> can be caught during review, harden copy_to_guest_offset() so the build
>> will fail if such users are introduced.
> 
> But there are other implications you break:
> 
>> --- a/xen/include/asm-arm/guest_access.h
>> +++ b/xen/include/asm-arm/guest_access.h
>> @@ -126,7 +126,7 @@ int access_guest_memory_by_ipa(struct domain *d, paddr_t ipa, void *buf,
>>   
>>   #define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
>>       const typeof(*(ptr)) *_s = (ptr);                   \
>> -    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
>> +    typeof(*((hnd).p)) *_d = (hnd).p;                   \
>>       ((void)((hnd).p == (ptr)));                         \
>>       __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
> 
> Until this change, it is "ptr" which all sizes get derived from,
> i.e. it is the internally used type rather than the handle type
> which controls this. I'm sure we use this in a few places, to
> copy to e.g. a handle derived from "void". Compatibility of types
> (disallowing other than void) is checked by the comparison on the
> line immediately after the line you change. Yes "_d+(off)" right
> above here then changes its result. I consider it pretty likely
> you'd notice this issue once you go beyond just build testing.

I missed that part. To be honest, it feels wrong to me to have "off" != 
0 and use a void type for the handle. Would it make sense to forbid it?

As a side node, I have updated __copy_to_guest_offset() but forgot to 
update copy_to_guest_offset(). I will look to apply the modifications we 
agree on both side.

> 
> To address this, I guess we need to find an expression along the
> lines of that comparison, which does not cause any code to be
> generated, but which verifies the properties we care about. The
> line you change should be left alone, from all I can tell right
> now.

I am not aware of any way before C11 to check if a variable is const or 
not. If we wanted to keep allow void type the handle then a possible 
approach would be:

#define copy_to_guest_offset(hnd, off, ptr, nr) ({              \
     const typeof(*(ptr)) *_s = (ptr);                           \
     typeof(*((hnd).p)) *_d = (hnd).p;                           \
     size_t mul = (sizeof(*(hnd).p) > 1) ? 1 : sizeof (*_s);     \
     ((void)((hnd).p == (ptr)));                                 \
     raw_copy_to_guest(_d + (off) * mul, _s, sizeof(*_s)*(nr));  \
})

I don't particularly like it but I could not come up with better so far.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/8] xen/guest_access: Harden copy_to_guest_offset to prevent const dest operand

[Jan Beulich]

On 31.03.2020 21:13, Julien Grall wrote:
> On 31/03/2020 08:26, Jan Beulich wrote:
>> On 30.03.2020 21:21, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> At the moment, copy_to_guest_offset() will allow the hypervisor to copy
>>> data to guest handle marked const.
>>>
>>> Thankfully, no users of the helper will do that. Rather than hoping this
>>> can be caught during review, harden copy_to_guest_offset() so the build
>>> will fail if such users are introduced.
>>
>> But there are other implications you break:
>>
>>> --- a/xen/include/asm-arm/guest_access.h
>>> +++ b/xen/include/asm-arm/guest_access.h
>>> @@ -126,7 +126,7 @@ int access_guest_memory_by_ipa(struct domain *d, paddr_t ipa, void *buf,
>>>     #define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
>>>       const typeof(*(ptr)) *_s = (ptr);                   \
>>> -    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
>>> +    typeof(*((hnd).p)) *_d = (hnd).p;                   \
>>>       ((void)((hnd).p == (ptr)));                         \
>>>       __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
>>
>> Until this change, it is "ptr" which all sizes get derived from,
>> i.e. it is the internally used type rather than the handle type
>> which controls this. I'm sure we use this in a few places, to
>> copy to e.g. a handle derived from "void". Compatibility of types
>> (disallowing other than void) is checked by the comparison on the
>> line immediately after the line you change. Yes "_d+(off)" right
>> above here then changes its result. I consider it pretty likely
>> you'd notice this issue once you go beyond just build testing.
> 
> I missed that part. To be honest, it feels wrong to me to have
> "off" != 0 and use a void type for the handle. Would it make
> sense to forbid it?

I don't think so - the idea (aiui) has always been for the Xen
internal object's type to control what gets copied, and hence a
void handle is to be fine for both copy-in and copy-out.

> As a side node, I have updated __copy_to_guest_offset() but
> forgot to update copy_to_guest_offset(). I will look to apply
> the modifications we agree on both side.

Ah, sure.

>> To address this, I guess we need to find an expression along the
>> lines of that comparison, which does not cause any code to be
>> generated, but which verifies the properties we care about. The
>> line you change should be left alone, from all I can tell right
>> now.
> 
> I am not aware of any way before C11 to check if a variable is
> const or not. If we wanted to keep allow void type the handle
> then a possible approach would be:
> 
> #define copy_to_guest_offset(hnd, off, ptr, nr) ({              \
>     const typeof(*(ptr)) *_s = (ptr);                           \
>     typeof(*((hnd).p)) *_d = (hnd).p;                           \
>     size_t mul = (sizeof(*(hnd).p) > 1) ? 1 : sizeof (*_s);     \
>     ((void)((hnd).p == (ptr)));                                 \
>     raw_copy_to_guest(_d + (off) * mul, _s, sizeof(*_s)*(nr));  \
> })
> 
> I don't particularly like it but I could not come up with better so far.

Not very nice indeed, and the conditional expression - at the
first glance being the wrong way round - seems outright
confusing to me. I'll try to find time to experiment some with
this as well, since unless we can find a reasonably neat
solution here, I'm inclined to suggest to leave this as it is
now.

Jan

Re: [PATCH 1/8] xen/guest_access: Harden copy_to_guest_offset to prevent const dest operand

[Julien Grall]

Hi Jan,

On 01/04/2020 07:48, Jan Beulich wrote:
> On 31.03.2020 21:13, Julien Grall wrote:
>> On 31/03/2020 08:26, Jan Beulich wrote:
>>> On 30.03.2020 21:21, Julien Grall wrote:
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> At the moment, copy_to_guest_offset() will allow the hypervisor to copy
>>>> data to guest handle marked const.
>>>>
>>>> Thankfully, no users of the helper will do that. Rather than hoping this
>>>> can be caught during review, harden copy_to_guest_offset() so the build
>>>> will fail if such users are introduced.
>>>
>>> But there are other implications you break:
>>>
>>>> --- a/xen/include/asm-arm/guest_access.h
>>>> +++ b/xen/include/asm-arm/guest_access.h
>>>> @@ -126,7 +126,7 @@ int access_guest_memory_by_ipa(struct domain *d, paddr_t ipa, void *buf,
>>>>      #define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
>>>>        const typeof(*(ptr)) *_s = (ptr);                   \
>>>> -    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
>>>> +    typeof(*((hnd).p)) *_d = (hnd).p;                   \
>>>>        ((void)((hnd).p == (ptr)));                         \
>>>>        __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
>>>
>>> Until this change, it is "ptr" which all sizes get derived from,
>>> i.e. it is the internally used type rather than the handle type
>>> which controls this. I'm sure we use this in a few places, to
>>> copy to e.g. a handle derived from "void". Compatibility of types
>>> (disallowing other than void) is checked by the comparison on the
>>> line immediately after the line you change. Yes "_d+(off)" right
>>> above here then changes its result. I consider it pretty likely
>>> you'd notice this issue once you go beyond just build testing.
>>
>> I missed that part. To be honest, it feels wrong to me to have
>> "off" != 0 and use a void type for the handle. Would it make
>> sense to forbid it?
> 
> I don't think so - the idea (aiui) has always been for the Xen
> internal object's type to control what gets copied, and hence a
> void handle is to be fine for both copy-in and copy-out.
> 
>> As a side node, I have updated __copy_to_guest_offset() but
>> forgot to update copy_to_guest_offset(). I will look to apply
>> the modifications we agree on both side.
> 
> Ah, sure.
> 
>>> To address this, I guess we need to find an expression along the
>>> lines of that comparison, which does not cause any code to be
>>> generated, but which verifies the properties we care about. The
>>> line you change should be left alone, from all I can tell right
>>> now.
>>
>> I am not aware of any way before C11 to check if a variable is
>> const or not. If we wanted to keep allow void type the handle
>> then a possible approach would be:
>>
>> #define copy_to_guest_offset(hnd, off, ptr, nr) ({              \
>>      const typeof(*(ptr)) *_s = (ptr);                           \
>>      typeof(*((hnd).p)) *_d = (hnd).p;                           \
>>      size_t mul = (sizeof(*(hnd).p) > 1) ? 1 : sizeof (*_s);     \
>>      ((void)((hnd).p == (ptr)));                                 \
>>      raw_copy_to_guest(_d + (off) * mul, _s, sizeof(*_s)*(nr));  \
>> })
>>
>> I don't particularly like it but I could not come up with better so far.
> 
> Not very nice indeed, and the conditional expression - at the
> first glance being the wrong way round - seems outright
> confusing to me.

It is the correct way. If the handle is a void (sizeof(*(hnd).p) == 1), 
then we need to know the size of the Xen buffer so we can compute the 
offset correctly. Otherwise, as we have the correct type, we can apply 
the offset directly and let the compiler do the math for us.

> I'll try to find time to experiment some with
> this as well, since unless we can find a reasonably neat
> solution here, I'm inclined to suggest to leave this as it is
> now.

It is difficult to catch the hypervisor who errorneously write to 
"const" handler. So I would not like a statu-quo.

A slightly uglier version (with more comments) is going to be better 
than what we currently have.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/8] xen/guest_access: Harden copy_to_guest_offset to prevent const dest operand

[Jan Beulich]

On 31.03.2020 21:13, Julien Grall wrote:
> I am not aware of any way before C11 to check if a variable is
> const or not. If we wanted to keep allow void type the handle
> then a possible approach would be:
> 
> #define copy_to_guest_offset(hnd, off, ptr, nr) ({              \
>     const typeof(*(ptr)) *_s = (ptr);                           \
>     typeof(*((hnd).p)) *_d = (hnd).p;                           \
>     size_t mul = (sizeof(*(hnd).p) > 1) ? 1 : sizeof (*_s);     \
>     ((void)((hnd).p == (ptr)));                                 \
>     raw_copy_to_guest(_d + (off) * mul, _s, sizeof(*_s)*(nr));  \
> })
> 
> I don't particularly like it but I could not come up with better so far.

Having looked at how in particular copy_field_to_guest() (which
doesn't have this issue afaict) works, here's an imo much better
alternative:

@@ -87,6 +87,7 @@
 #define copy_to_guest_offset(hnd, off, ptr, nr) ({      \
     const typeof(*(ptr)) *_s = (ptr);                   \
     char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
+    void *__maybe_unused _t = (hnd).p;                  \
     ((void)((hnd).p == (ptr)));                         \
     raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));  \
 })
@@ -143,6 +144,7 @@ static inline void put_guest_handle(void
 #define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
     const typeof(*(ptr)) *_s = (ptr);                   \
     char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
+    void *__maybe_unused _t = (hnd).p;                  \
     ((void)((hnd).p == (ptr)));                         \
     __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
 })

Jan

Re: [PATCH 2/8] xen/public: sysctl: set_parameter.params and debug.keys should be const

[Julien Grall]

Hi Jan,

On 31/03/2020 08:30, Jan Beulich wrote:
> On 30.03.2020 21:21, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> The fields set_parameter.params and debug.keys should never be modified
>> by the hypervisor. So mark them as const.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
>> I am not entirely sure whether we should bump the systcl version for
>> this change. Any thoughts?
> 
> No, it should be left as is - it's about _binary_ compatibility (e.g.
> if structure layout changes, or a sub-function gets dropped). The
> need to potentially address build issues resulting from changes like
> the one here isn't covered by it, but by the __XEN__ / __XEN_TOOLS__
> conditional at the top of the header.

Thank you for the examplanation! I will commit the patch.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/8] xen/guest_access: Harden copy_to_guest_offset to prevent const dest operand

[Julien Grall]

Hi Jan,

On 01/04/2020 10:25, Jan Beulich wrote:
> On 31.03.2020 21:13, Julien Grall wrote:
>> I am not aware of any way before C11 to check if a variable is
>> const or not. If we wanted to keep allow void type the handle
>> then a possible approach would be:
>>
>> #define copy_to_guest_offset(hnd, off, ptr, nr) ({              \
>>      const typeof(*(ptr)) *_s = (ptr);                           \
>>      typeof(*((hnd).p)) *_d = (hnd).p;                           \
>>      size_t mul = (sizeof(*(hnd).p) > 1) ? 1 : sizeof (*_s);     \
>>      ((void)((hnd).p == (ptr)));                                 \
>>      raw_copy_to_guest(_d + (off) * mul, _s, sizeof(*_s)*(nr));  \
>> })
>>
>> I don't particularly like it but I could not come up with better so far.
> 
> Having looked at how in particular copy_field_to_guest() (which
> doesn't have this issue afaict) works, here's an imo much better
> alternative:
> 
> @@ -87,6 +87,7 @@
>   #define copy_to_guest_offset(hnd, off, ptr, nr) ({      \
>       const typeof(*(ptr)) *_s = (ptr);                   \
>       char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
> +    void *__maybe_unused _t = (hnd).p;                  \
>       ((void)((hnd).p == (ptr)));                         \
>       raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));  \
>   })
> @@ -143,6 +144,7 @@ static inline void put_guest_handle(void
>   #define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
>       const typeof(*(ptr)) *_s = (ptr);                   \
>       char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
> +    void *__maybe_unused _t = (hnd).p;                  \
>       ((void)((hnd).p == (ptr)));                         \
>       __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
>   })

I actually thought about this one but discarded it because it was using 
unused variable. But I am happy with it, I will have a look to respin 
the patch.

Cheers,

-- 
Julien Grall

Re: [PATCH 0/8] Fix build with using OCaml 4.06.1 and -safe-string

[Julien Grall]

Hi Ian,

On 31/03/2020 12:17, Ian Jackson wrote:
> Julien Grall writes ("[PATCH 0/8] Fix build with using OCaml 4.06.1 and -safe-string"):
>> This series is meant to solve the build issue reported by Dario when
>> using recent version of OCaml and -safe-string.
> 
> Thanks.  I have reviewed the C tools parts here.  I think the ocaml
> parts ought to have a review from someone familiar with the ocaml FFI.
> 
>> I took the opportunity to harden a bit more the code by using const more
>> often.
> 
> I approve.
> 
> Perhaps we should start building our C code with -Wwrite-strings,
> which makes "" have type const char* ?  Result would be a giant
> constification patch, probably.

So I thought I would give a try and see how far I can go:

    * hypervisor (xen): It is fairly easy to convert, although this is 
touching code that was imported from other projects (such as acpica). I 
need to have a look at whether other projects fixed there code and we 
can backport.
    * libxc: This is pretty trivial, I will send a patch for it
    * libxl: This is where it is getting tricky, the main issue is the 
flexarray framework as we would use it with string (now const char *). I 
thought we could make the interface const, but it looks like there are a 
couple of places where we need to modify the content (such as in 
libxl_json.c). I am not sure yet how to deal with it.

In any case, even if we can't use -Wwrite-strings, I can still send 
patches to use const in more places.

Cheers,

-- 
Julien Grall

Re: [PATCH 0/8] Fix build with using OCaml 4.06.1 and -safe-string

[Julien Grall]

Hi,

Gentle ping. I am missing reviews for the OCaml part.

Cheers,

On 30/03/2020 20:21, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Hi all,
> 
> This series is meant to solve the build issue reported by Dario when
> using recent version of OCaml and -safe-string.
> 
> I took the opportunity to harden a bit more the code by using const more
> often.
> 
> This series was only build tested.
> 
> Cheers,
> 
> Julien Grall (8):
>    xen/guest_access: Harden copy_to_guest_offset to prevent const dest
>      operand
>    xen/public: sysctl: set_parameter.params and debug.keys should be
>      const
>    tools/libxc: misc: Mark const the parameter 'keys' of
>      xc_send_debug_keys()
>    tools/libxc: misc: Mark const the parameter 'params' of
>      xc_set_parameters()
>    tools/ocaml: libxc: Check error return in stub_xc_vcpu_context_get()
>    tools/ocaml: libxb: Harden stub_header_of_string()
>    tools/ocaml: libxb: Avoid to use String_val() when value is bytes
>    tools/ocaml: Fix stubs build when OCaml has been compiled with
>      -safe-string
> 
>   tools/libxc/include/xenctrl.h       |  4 ++--
>   tools/libxc/xc_misc.c               |  8 ++++----
>   tools/libxc/xc_private.h            |  8 ++++++++
>   tools/ocaml/libs/xb/xenbus_stubs.c  |  6 +++---
>   tools/ocaml/libs/xb/xs_ring_stubs.c | 12 ++++++++++--
>   tools/ocaml/libs/xc/xenctrl_stubs.c |  6 ++++--
>   xen/include/asm-arm/guest_access.h  |  2 +-
>   xen/include/asm-x86/guest_access.h  |  2 +-
>   xen/include/public/sysctl.h         |  4 ++--
>   9 files changed, 35 insertions(+), 17 deletions(-)
> 

-- 
Julien Grall

Re: [PATCH 0/8] Fix build with using OCaml 4.06.1 and -safe-string

[Christian Lindig]

The changes in the OCaml C stubs look good to me. They don't really touch the interface but are mostly concerned with types on the C side by adding casts, const, and so on. The extended error handling is an improvement.

-- Christian

-- 
Acked-by: Christian Lindig <christian.lindig@citrix.com>

________________________________________
From: Julien Grall <julien@xen.org>
Sent: 16 April 2020 12:25
To: xen-devel@lists.xenproject.org; Christian Lindig; David Scott
Cc: dfaggioli@suse.com; Julien Grall; Stefano Stabellini; Volodymyr Babchuk; Jan Beulich; Andrew Cooper; Wei Liu; Roger Pau Monne; George Dunlap; Ian Jackson
Subject: Re: [PATCH 0/8] Fix build with using OCaml 4.06.1 and -safe-string

Hi,

Gentle ping. I am missing reviews for the OCaml part.

Cheers,

On 30/03/2020 20:21, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
>
> Hi all,
>
> This series is meant to solve the build issue reported by Dario when
> using recent version of OCaml and -safe-string.
>
> I took the opportunity to harden a bit more the code by using const more
> often.
>
> This series was only build tested.
>
> Cheers,
>
> Julien Grall (8):
>    xen/guest_access: Harden copy_to_guest_offset to prevent const dest
>      operand
>    xen/public: sysctl: set_parameter.params and debug.keys should be
>      const
>    tools/libxc: misc: Mark const the parameter 'keys' of
>      xc_send_debug_keys()
>    tools/libxc: misc: Mark const the parameter 'params' of
>      xc_set_parameters()
>    tools/ocaml: libxc: Check error return in stub_xc_vcpu_context_get()
>    tools/ocaml: libxb: Harden stub_header_of_string()
>    tools/ocaml: libxb: Avoid to use String_val() when value is bytes
>    tools/ocaml: Fix stubs build when OCaml has been compiled with
>      -safe-string
>
>   tools/libxc/include/xenctrl.h       |  4 ++--
>   tools/libxc/xc_misc.c               |  8 ++++----
>   tools/libxc/xc_private.h            |  8 ++++++++
>   tools/ocaml/libs/xb/xenbus_stubs.c  |  6 +++---
>   tools/ocaml/libs/xb/xs_ring_stubs.c | 12 ++++++++++--
>   tools/ocaml/libs/xc/xenctrl_stubs.c |  6 ++++--
>   xen/include/asm-arm/guest_access.h  |  2 +-
>   xen/include/asm-x86/guest_access.h  |  2 +-
>   xen/include/public/sysctl.h         |  4 ++--
>   9 files changed, 35 insertions(+), 17 deletions(-)
>

--
Julien Grall

Re: [PATCH 0/8] Fix build with using OCaml 4.06.1 and -safe-string

[Julien Grall]

Hi Christian,

On 16/04/2020 14:25, Christian Lindig wrote:
> 
> The changes in the OCaml C stubs look good to me. They don't really touch the interface but are mostly concerned with types on the C side by adding casts, const, and so on. The extended error handling is an improvement.

Thank you for the review! I will commit the rest of the series.

As an aside, the acked-by was adding as part of the signature.

Not sure whether this is intentional but some e-mail clients are hiding 
the signature so the acked-by can be easily missed.

Cheers,

-- 
Julien Grall