[PATCH v2 0/5] Fix truncation of various XENVER_* subops

[PATCH v2 0/5] Fix truncation of various XENVER_* subops

[Andrew Cooper]

See patch 3 for details of the problem.  Other patches fix other errors found
while investigating.

Some patches committed straight from v1.  Several new patches with additional
cleanup.

Andrew Cooper (5):
  xen/version: Drop bogus return values for XENVER_platform_parameters
  xen/version: Calculate xen_capabilities_info once at boot
  xen/version: Introduce non-truncating XENVER_* subops
  xen/version: Fold build_id handling into xenver_varbuf_op()
  xen/version: Misc style fixes

 xen/arch/arm/setup.c         |  19 ++----
 xen/arch/x86/setup.c         |  31 ++++------
 xen/common/kernel.c          | 139 ++++++++++++++++++++++++++++++-------------
 xen/common/version.c         |   4 +-
 xen/include/public/version.h |  95 ++++++++++++++++++++++++++++-
 xen/include/xen/hypercall.h  |   2 -
 xen/include/xen/version.h    |   2 +
 xen/include/xlat.lst         |   1 +
 xen/xsm/flask/hooks.c        |   4 ++
 9 files changed, 214 insertions(+), 83 deletions(-)

-- 
2.11.0

[PATCH v2 1/5] xen/version: Drop bogus return values for XENVER_platform_parameters

[Andrew Cooper]

A split in virtual address space is only applicable for x86 PV guests.
Furthermore, the information returned for x86 64bit PV guests is wrong.

Explain the problem in version.h, stating the other information that PV guests
need to know.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>

The only reason this does not get an XSA is because Xen does not have any form
of KALSR.

v2:
 * Retain the useless return value for 64bit PV guests in release builds only.
 * Rewrite comments.
---
 xen/common/kernel.c          | 20 +++++++++++++++++---
 xen/include/public/version.h | 27 +++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 4f268d83e3cb..f7b1f65f373c 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -518,11 +518,15 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     
     case XENVER_platform_parameters:
     {
+        const struct vcpu *curr = current;
+
 #ifdef CONFIG_COMPAT
-        if ( current->hcall_compat )
+        if ( curr->hcall_compat )
         {
             compat_platform_parameters_t params = {
-                .virt_start = HYPERVISOR_COMPAT_VIRT_START(current->domain),
+                .virt_start = is_pv_vcpu(curr)
+                            ? HYPERVISOR_COMPAT_VIRT_START(curr->domain)
+                            : 0,
             };
 
             if ( copy_to_guest(arg, &params, 1) )
@@ -532,7 +536,17 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 #endif
         {
             xen_platform_parameters_t params = {
-                .virt_start = HYPERVISOR_VIRT_START,
+                /*
+                 * Out of an abundance of caution, retain the useless return
+                 * value for 64bit PV guests, but in release builds only.
+                 *
+                 * This is not expected to cause any problems, but if it does,
+                 * the developer impacted will be the one best suited to fix
+                 * the caller not to issue this hypercall.
+                 */
+                .virt_start = !IS_ENABLED(CONFIG_DEBUG) && is_pv_vcpu(curr)
+                              ? HYPERVISOR_VIRT_START
+                              : 0,
             };
 
             if ( copy_to_guest(arg, &params, 1) )
diff --git a/xen/include/public/version.h b/xen/include/public/version.h
index 0ff8bd9077c6..cbc4ef7a46e6 100644
--- a/xen/include/public/version.h
+++ b/xen/include/public/version.h
@@ -42,6 +42,33 @@ typedef char xen_capabilities_info_t[1024];
 typedef char xen_changeset_info_t[64];
 #define XEN_CHANGESET_INFO_LEN (sizeof(xen_changeset_info_t))
 
+/*
+ * This API is problematic.
+ *
+ * It is only applicable to guests which share pagetables with Xen (x86 PV
+ * guests), but unfortunately has leaked into other guest types and
+ * architectures with an expectation of never failing.
+ *
+ * It is intended to identify the virtual address split between guest kernel
+ * and Xen.
+ *
+ * For 32bit PV guests, there is a split, and it is variable (between two
+ * fixed bounds), and this boundary is reported to guests.  The detail missing
+ * from the hypercall is that the second boundary is the 32bit architectural
+ * boundary at 4G.
+ *
+ * For 64bit PV guests, Xen lives at the bottom of the upper canonical range.
+ * This hypercall happens to report the architectural boundary, not the one
+ * which would be necessary to make a variable split work.  As such, this
+ * hypercall entirely useless for 64bit PV guests, and all inspected
+ * implementations at the time of writing were found to have compile time
+ * expectations about the split.
+ *
+ * For architectures where this hypercall is implemented, for backwards
+ * compatibility with the expectation of the hypercall never failing Xen will
+ * return 0 instead of failing with -ENOSYS in cases where the guest should
+ * not be making the hypercall.
+ */
 #define XENVER_platform_parameters 5
 struct xen_platform_parameters {
     xen_ulong_t virt_start;
-- 
2.11.0

[PATCH v2 2/5] xen/version: Calculate xen_capabilities_info once at boot

[Andrew Cooper]

The arch_get_xen_caps() infrastructure is horribly inefficient, for something
that is constant after features have been resolved on boot.

Every instance used snprintf() to format constants into a string (which gets
shorter when %d gets resolved!), which gets double buffered on the stack.

Switch to using string literals with the "3.0" inserted - these numbers
haven't changed in 18 years (The Xen 3.0 release was Dec 5th 2005).

Use initcalls to format the data into xen_cap_info, which is deliberately not
of type xen_capabilities_info_t because a 1k array is a silly overhead for
storing a maximum of 77 chars (the x86 version) and isn't liable to need any
more space in the forseeable future.

This speeds up the the XENVER_capabilities hypercall, but the purpose of the
change is to allow us to introduce a better XENVER_* API that doesn't force us
to put a 1k buffer on the stack.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>

v2:
 * New

If Xen had strncpy(), then the hunk in do_xen_version() could read:

  if ( deny )
     memset(info, 0, sizeof(info));
  else
     strncpy(info, xen_cap_info, sizeof(info));

to avoid double processing the start of the buffer, but given the ABI (must
write 1k chars into the guest), I cannot see any way of taking info off the
stack without some kind of strncpy_to_guest() API.

Moving to __initcall() also allows new architectures to not implement this
API, and I'm going to recommend strongly that they dont.  Its a very dubious
way of signalling about 3 bits of info to the toolstack, and inefficient to
use (the toolstack has to do string parsing on the result figure out if
PV64/PV32/HVM is available).
---
 xen/arch/arm/setup.c        | 19 ++++++-------------
 xen/arch/x86/setup.c        | 31 ++++++++++---------------------
 xen/common/kernel.c         |  3 ++-
 xen/include/xen/hypercall.h |  2 --
 xen/include/xen/version.h   |  2 ++
 5 files changed, 20 insertions(+), 37 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 1f26f67b90e3..b71b4bc506e0 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -1194,24 +1194,17 @@ void __init start_xen(unsigned long boot_phys_offset,
     switch_stack_and_jump(idle_vcpu[0]->arch.cpu_info, init_done);
 }
 
-void arch_get_xen_caps(xen_capabilities_info_t *info)
+static int __init init_xen_cap_info(void)
 {
-    /* Interface name is always xen-3.0-* for Xen-3.x. */
-    int major = 3, minor = 0;
-    char s[32];
-
-    (*info)[0] = '\0';
-
 #ifdef CONFIG_ARM_64
-    snprintf(s, sizeof(s), "xen-%d.%d-aarch64 ", major, minor);
-    safe_strcat(*info, s);
+    safe_strcat(xen_cap_info, "xen-3.0-aarch64 ");
 #endif
     if ( cpu_has_aarch32 )
-    {
-        snprintf(s, sizeof(s), "xen-%d.%d-armv7l ", major, minor);
-        safe_strcat(*info, s);
-    }
+        safe_strcat(xen_cap_info, "xen-3.0-armv7l ");
+
+    return 0;
 }
+__initcall(init_xen_cap_info);
 
 /*
  * Local variables:
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 566422600d94..f80821469ece 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1956,35 +1956,24 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     unreachable();
 }
 
-void arch_get_xen_caps(xen_capabilities_info_t *info)
+static int __init cf_check init_xen_cap_info(void)
 {
-    /* Interface name is always xen-3.0-* for Xen-3.x. */
-    int major = 3, minor = 0;
-    char s[32];
-
-    (*info)[0] = '\0';
-
     if ( IS_ENABLED(CONFIG_PV) )
     {
-        snprintf(s, sizeof(s), "xen-%d.%d-x86_64 ", major, minor);
-        safe_strcat(*info, s);
+        safe_strcat(xen_cap_info, "xen-3.0-x86_64 ");
 
         if ( opt_pv32 )
-        {
-            snprintf(s, sizeof(s), "xen-%d.%d-x86_32p ", major, minor);
-            safe_strcat(*info, s);
-        }
+            safe_strcat(xen_cap_info, "xen-3.0-x86_32p ");
     }
     if ( hvm_enabled )
-    {
-        snprintf(s, sizeof(s), "hvm-%d.%d-x86_32 ", major, minor);
-        safe_strcat(*info, s);
-        snprintf(s, sizeof(s), "hvm-%d.%d-x86_32p ", major, minor);
-        safe_strcat(*info, s);
-        snprintf(s, sizeof(s), "hvm-%d.%d-x86_64 ", major, minor);
-        safe_strcat(*info, s);
-    }
+        safe_strcat(xen_cap_info,
+                    "hvm-3.0-x86_32 "
+                    "hvm-3.0-x86_32p "
+                    "hvm-3.0-x86_64 ");
+
+    return 0;
 }
+__initcall(init_xen_cap_info);
 
 int __hwdom_init xen_in_range(unsigned long mfn)
 {
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index f7b1f65f373c..4fa1d6710115 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -30,6 +30,7 @@ enum system_state system_state = SYS_STATE_early_boot;
 
 xen_commandline_t saved_cmdline;
 static const char __initconst opt_builtin_cmdline[] = CONFIG_CMDLINE;
+char __ro_after_init xen_cap_info[128];
 
 static int assign_integer_param(const struct kernel_param *param, uint64_t val)
 {
@@ -509,7 +510,7 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
         memset(info, 0, sizeof(info));
         if ( !deny )
-            arch_get_xen_caps(&info);
+            safe_strcpy(info, xen_cap_info);
 
         if ( copy_to_guest(arg, info, ARRAY_SIZE(info)) )
             return -EFAULT;
diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h
index f307dfb59760..15b6be6ec818 100644
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -56,6 +56,4 @@ common_vcpu_op(int cmd,
     struct vcpu *v,
     XEN_GUEST_HANDLE_PARAM(void) arg);
 
-void arch_get_xen_caps(xen_capabilities_info_t *info);
-
 #endif /* __XEN_HYPERCALL_H__ */
diff --git a/xen/include/xen/version.h b/xen/include/xen/version.h
index 93c58773630c..4856ad1b446d 100644
--- a/xen/include/xen/version.h
+++ b/xen/include/xen/version.h
@@ -19,6 +19,8 @@ const char *xen_deny(void);
 const char *xen_build_info(void);
 int xen_build_id(const void **p, unsigned int *len);
 
+extern char xen_cap_info[128];
+
 #ifdef BUILD_ID
 void xen_build_init(void);
 int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
-- 
2.11.0

[PATCH v2 3/5] xen/version: Introduce non-truncating XENVER_* subops

[Andrew Cooper]

Recently in XenServer, we have encountered problems caused by both
XENVER_extraversion and XENVER_commandline having fixed bounds.

More than just the invariant size, the APIs/ABIs also broken by typedef-ing an
array, and using an unqualified 'char' which has implementation-specific
signed-ness

Provide brand new ops, which are capable of expressing variable length
strings, and mark the older ops as broken.

This fixes all issues around XENVER_extraversion being longer than 15 chars.
More work is required to remove other assumptions about XENVER_commandline
being 1023 chars long.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>
CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>
CC: Daniel Smith <dpsmith@apertussolutions.com>
CC: Jason Andryuk <jandryuk@gmail.com>

v2:
 * Remove xen_capabilities_info_t from the stack now that arch_get_xen_caps()
   has gone.
 * Use an arbitrary limit check much lower than INT_MAX.
 * Use "buf" rather than "string" terminology.
 * Expand the API comment.

Tested by forcing XENVER_extraversion to be 20 chars long, and confirming that
an untruncated version can be obtained.
---
 xen/common/kernel.c          | 62 +++++++++++++++++++++++++++++++++++++++++++
 xen/include/public/version.h | 63 ++++++++++++++++++++++++++++++++++++++++++--
 xen/include/xlat.lst         |  1 +
 xen/xsm/flask/hooks.c        |  4 +++
 4 files changed, 128 insertions(+), 2 deletions(-)

diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 4fa1d6710115..cc5d8247b04d 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -24,6 +24,7 @@
 CHECK_build_id;
 CHECK_compile_info;
 CHECK_feature_info;
+CHECK_varbuf;
 #endif
 
 enum system_state system_state = SYS_STATE_early_boot;
@@ -470,6 +471,59 @@ static int __init cf_check param_init(void)
 __initcall(param_init);
 #endif
 
+static long xenver_varbuf_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
+{
+    struct xen_varbuf user_str;
+    const char *str = NULL;
+    size_t sz;
+
+    switch ( cmd )
+    {
+    case XENVER_extraversion2:
+        str = xen_extra_version();
+        break;
+
+    case XENVER_changeset2:
+        str = xen_changeset();
+        break;
+
+    case XENVER_commandline2:
+        str = saved_cmdline;
+        break;
+
+    case XENVER_capabilities2:
+        str = xen_cap_info;
+        break;
+
+    default:
+        ASSERT_UNREACHABLE();
+        return -ENODATA;
+    }
+
+    sz = strlen(str);
+
+    if ( sz > KB(64) ) /* Arbitrary limit.  Avoid long-running operations. */
+        return -E2BIG;
+
+    if ( guest_handle_is_null(arg) ) /* Length request */
+        return sz;
+
+    if ( copy_from_guest(&user_str, arg, 1) )
+        return -EFAULT;
+
+    if ( user_str.len == 0 )
+        return -EINVAL;
+
+    if ( sz > user_str.len )
+        return -ENOBUFS;
+
+    if ( copy_to_guest_offset(arg, offsetof(struct xen_varbuf, buf),
+                              str, sz) )
+        return -EFAULT;
+
+    return sz;
+}
+
 long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
     bool_t deny = !!xsm_xen_version(XSM_OTHER, cmd);
@@ -683,6 +737,14 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
         return sz;
     }
+
+    case XENVER_extraversion2:
+    case XENVER_capabilities2:
+    case XENVER_changeset2:
+    case XENVER_commandline2:
+        if ( deny )
+            return -EPERM;
+        return xenver_varbuf_op(cmd, arg);
     }
 
     return -ENOSYS;
diff --git a/xen/include/public/version.h b/xen/include/public/version.h
index cbc4ef7a46e6..9287b5d3ff50 100644
--- a/xen/include/public/version.h
+++ b/xen/include/public/version.h
@@ -19,12 +19,20 @@
 /* arg == NULL; returns major:minor (16:16). */
 #define XENVER_version      0
 
-/* arg == xen_extraversion_t. */
+/*
+ * arg == xen_extraversion_t.
+ *
+ * This API/ABI is broken.  Use XENVER_extraversion2 instead.
+ */
 #define XENVER_extraversion 1
 typedef char xen_extraversion_t[16];
 #define XEN_EXTRAVERSION_LEN (sizeof(xen_extraversion_t))
 
-/* arg == xen_compile_info_t. */
+/*
+ * arg == xen_compile_info_t.
+ *
+ * This API/ABI is broken and truncates data.
+ */
 #define XENVER_compile_info 2
 struct xen_compile_info {
     char compiler[64];
@@ -34,10 +42,20 @@ struct xen_compile_info {
 };
 typedef struct xen_compile_info xen_compile_info_t;
 
+/*
+ * arg == xen_capabilities_info_t.
+ *
+ * This API/ABI is broken.  Use XENVER_capabilities2 instead.
+ */
 #define XENVER_capabilities 3
 typedef char xen_capabilities_info_t[1024];
 #define XEN_CAPABILITIES_INFO_LEN (sizeof(xen_capabilities_info_t))
 
+/*
+ * arg == xen_changeset_info_t.
+ *
+ * This API/ABI is broken.  Use XENVER_changeset2 instead.
+ */
 #define XENVER_changeset 4
 typedef char xen_changeset_info_t[64];
 #define XEN_CHANGESET_INFO_LEN (sizeof(xen_changeset_info_t))
@@ -95,6 +113,11 @@ typedef struct xen_feature_info xen_feature_info_t;
  */
 #define XENVER_guest_handle 8
 
+/*
+ * arg == xen_commandline_t.
+ *
+ * This API/ABI is broken.  Use XENVER_commandline2 instead.
+ */
 #define XENVER_commandline 9
 typedef char xen_commandline_t[1024];
 
@@ -110,6 +133,42 @@ struct xen_build_id {
 };
 typedef struct xen_build_id xen_build_id_t;
 
+/*
+ * Container for an arbitrary variable length buffer.
+ */
+struct xen_varbuf {
+    uint32_t len;                          /* IN:  size of buf[] in bytes. */
+    unsigned char buf[XEN_FLEX_ARRAY_DIM]; /* OUT: requested data.         */
+};
+typedef struct xen_varbuf xen_varbuf_t;
+
+/*
+ * arg == xen_varbuf_t
+ *
+ * Equivalent to the original ops, but with a non-truncating API/ABI.
+ *
+ * These hypercalls can fail for a number of reasons.  All callers must handle
+ * -XEN_xxx return values appropriately.
+ *
+ * Passing arg == NULL is a request for size, which will be signalled with a
+ * non-negative return value.  Note: a return size of 0 may be legitimate for
+ * the requested subop.
+ *
+ * Otherwise, the input xen_varbuf_t provides the size of the following
+ * buffer.  Xen will fill the buffer, and return the number of bytes written
+ * (e.g. if the input buffer was longer than necessary).
+ *
+ * Some subops may return binary data.  Some subops may be expected to return
+ * textural data.  These are returned without a NUL terminator, and while the
+ * contents is expected to be ASCII/UTF-8, Xen makes no guarentees to this
+ * effect.  e.g. Xen has no control over the formatting used for the command
+ * line.
+ */
+#define XENVER_extraversion2 11
+#define XENVER_capabilities2 12
+#define XENVER_changeset2    13
+#define XENVER_commandline2  14
+
 #endif /* __XEN_PUBLIC_VERSION_H__ */
 
 /*
diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
index d601a8a98421..762c8a77fb27 100644
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -172,6 +172,7 @@
 ?	build_id                        version.h
 ?	compile_info                    version.h
 ?	feature_info                    version.h
+?	varbuf                          version.h
 ?	xenoprof_init			xenoprof.h
 ?	xenoprof_passive		xenoprof.h
 ?	flask_access			xsm/flask_op.h
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 78225f68c15c..a671dcd0322e 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1777,15 +1777,18 @@ static int cf_check flask_xen_version(uint32_t op)
         /* These sub-ops ignore the permission checks and return data. */
         return 0;
     case XENVER_extraversion:
+    case XENVER_extraversion2:
         return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION,
                             VERSION__XEN_EXTRAVERSION, NULL);
     case XENVER_compile_info:
         return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION,
                             VERSION__XEN_COMPILE_INFO, NULL);
     case XENVER_capabilities:
+    case XENVER_capabilities2:
         return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION,
                             VERSION__XEN_CAPABILITIES, NULL);
     case XENVER_changeset:
+    case XENVER_changeset2:
         return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION,
                             VERSION__XEN_CHANGESET, NULL);
     case XENVER_pagesize:
@@ -1795,6 +1798,7 @@ static int cf_check flask_xen_version(uint32_t op)
         return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION,
                             VERSION__XEN_GUEST_HANDLE, NULL);
     case XENVER_commandline:
+    case XENVER_commandline2:
         return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION,
                             VERSION__XEN_COMMANDLINE, NULL);
     case XENVER_build_id:
-- 
2.11.0

[PATCH v2 4/5] xen/version: Fold build_id handling into xenver_varbuf_op()

[Andrew Cooper]

struct xen_build_id and struct xen_varbuf are identical from an ABI point of
view, so XENVER_build_id can reuse xenver_varbuf_op() rather than having it's
own almost identical copy of the logic.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>

v2:
 * New
---
 xen/common/kernel.c          | 49 +++++++++++++-------------------------------
 xen/include/public/version.h |  5 ++++-
 2 files changed, 18 insertions(+), 36 deletions(-)

diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index cc5d8247b04d..7ab410ac7c28 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -476,9 +476,22 @@ static long xenver_varbuf_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     struct xen_varbuf user_str;
     const char *str = NULL;
     size_t sz;
+    int rc;
 
     switch ( cmd )
     {
+    case XENVER_build_id:
+    {
+        unsigned int local_sz;
+
+        rc = xen_build_id((const void **)&str, &local_sz);
+        if ( rc )
+            return rc;
+
+        sz = local_sz;
+        goto have_len;
+    }
+
     case XENVER_extraversion2:
         str = xen_extra_version();
         break;
@@ -502,6 +515,7 @@ static long xenver_varbuf_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
     sz = strlen(str);
 
+ have_len:
     if ( sz > KB(64) ) /* Arbitrary limit.  Avoid long-running operations. */
         return -E2BIG;
 
@@ -703,41 +717,6 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     }
 
     case XENVER_build_id:
-    {
-        xen_build_id_t build_id;
-        unsigned int sz;
-        int rc;
-        const void *p;
-
-        if ( deny )
-            return -EPERM;
-
-        /* Only return size. */
-        if ( !guest_handle_is_null(arg) )
-        {
-            if ( copy_from_guest(&build_id, arg, 1) )
-                return -EFAULT;
-
-            if ( build_id.len == 0 )
-                return -EINVAL;
-        }
-
-        rc = xen_build_id(&p, &sz);
-        if ( rc )
-            return rc;
-
-        if ( guest_handle_is_null(arg) )
-            return sz;
-
-        if ( sz > build_id.len )
-            return -ENOBUFS;
-
-        if ( copy_to_guest_offset(arg, offsetof(xen_build_id_t, buf), p, sz) )
-            return -EFAULT;
-
-        return sz;
-    }
-
     case XENVER_extraversion2:
     case XENVER_capabilities2:
     case XENVER_changeset2:
diff --git a/xen/include/public/version.h b/xen/include/public/version.h
index 9287b5d3ff50..ee32d4c6b30b 100644
--- a/xen/include/public/version.h
+++ b/xen/include/public/version.h
@@ -124,8 +124,10 @@ typedef char xen_commandline_t[1024];
 /*
  * Return value is the number of bytes written, or XEN_Exx on error.
  * Calling with empty parameter returns the size of build_id.
+ *
+ * Note: structure only kept for backwards compatibility.  Xen operates in
+ * terms of xen_varbuf_t.
  */
-#define XENVER_build_id 10
 struct xen_build_id {
         uint32_t        len; /* IN: size of buf[]. */
         unsigned char   buf[XEN_FLEX_ARRAY_DIM];
@@ -164,6 +166,7 @@ typedef struct xen_varbuf xen_varbuf_t;
  * effect.  e.g. Xen has no control over the formatting used for the command
  * line.
  */
+#define XENVER_build_id      10
 #define XENVER_extraversion2 11
 #define XENVER_capabilities2 12
 #define XENVER_changeset2    13
-- 
2.11.0

[PATCH v2 5/5] xen/version: Misc style fixes

[Andrew Cooper]

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>
---
 xen/common/kernel.c  | 11 +++++------
 xen/common/version.c |  4 ++--
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 7ab410ac7c28..f1d4a66d8885 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -1,6 +1,6 @@
 /******************************************************************************
  * kernel.c
- * 
+ *
  * Copyright (c) 2002-2005 K A Fraser
  */
 
@@ -540,7 +540,7 @@ static long xenver_varbuf_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
 long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
-    bool_t deny = !!xsm_xen_version(XSM_OTHER, cmd);
+    bool deny = xsm_xen_version(XSM_OTHER, cmd);
 
     switch ( cmd )
     {
@@ -584,7 +584,7 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
             return -EFAULT;
         return 0;
     }
-    
+
     case XENVER_platform_parameters:
     {
         const struct vcpu *curr = current;
@@ -623,9 +623,8 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         }
 
         return 0;
-        
     }
-    
+
     case XENVER_changeset:
     {
         xen_changeset_info_t chgset;
@@ -652,7 +651,7 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
             if ( VM_ASSIST(d, pae_extended_cr3) )
                 fi.submap |= (1U << XENFEAT_pae_pgdir_above_4gb);
             if ( paging_mode_translate(d) )
-                fi.submap |= 
+                fi.submap |=
                     (1U << XENFEAT_writable_page_tables) |
                     (1U << XENFEAT_auto_translated_physmap);
             if ( is_hardware_domain(d) )
diff --git a/xen/common/version.c b/xen/common/version.c
index d32013520863..8e738672debe 100644
--- a/xen/common/version.c
+++ b/xen/common/version.c
@@ -209,11 +209,11 @@ void __init xen_build_init(void)
             }
         }
     }
-#endif
+#endif /* CONFIG_X86 */
     if ( !rc )
         printk(XENLOG_INFO "build-id: %*phN\n", build_id_len, build_id_p);
 }
-#endif
+#endif /* BUILD_ID */
 /*
  * Local variables:
  * mode: C
-- 
2.11.0

Re: [PATCH v2 1/5] xen/version: Drop bogus return values for XENVER_platform_parameters

[Jan Beulich]

On 14.01.2023 00:08, Andrew Cooper wrote:
> A split in virtual address space is only applicable for x86 PV guests.
> Furthermore, the information returned for x86 64bit PV guests is wrong.
> 
> Explain the problem in version.h, stating the other information that PV guests
> need to know.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> The only reason this does not get an XSA is because Xen does not have any form
> of KALSR.

I continue to question this. I think I did say before that even with KASLR Xen
would need to constrain itself to the 16 L4 slots that the ABI reserves for
its purpose. And nothing is being returned about the inner structure of that
virtual address range.

Jan

Re: [PATCH v2 2/5] xen/version: Calculate xen_capabilities_info once at boot

[Jan Beulich]

On 14.01.2023 00:08, Andrew Cooper wrote:
> The arch_get_xen_caps() infrastructure is horribly inefficient, for something
> that is constant after features have been resolved on boot.
> 
> Every instance used snprintf() to format constants into a string (which gets
> shorter when %d gets resolved!), which gets double buffered on the stack.
> 
> Switch to using string literals with the "3.0" inserted - these numbers
> haven't changed in 18 years (The Xen 3.0 release was Dec 5th 2005).
> 
> Use initcalls to format the data into xen_cap_info, which is deliberately not
> of type xen_capabilities_info_t because a 1k array is a silly overhead for
> storing a maximum of 77 chars (the x86 version) and isn't liable to need any
> more space in the forseeable future.

So I was wondering if once we arrived at the new ABI (and hence the 3.0 one
is properly legacy) we shouldn't declare Xen 5.0 and then also mark the new
ABI's availability here by a string including "5.0" where at present we
expose (only) "3.0".

> If Xen had strncpy(), then the hunk in do_xen_version() could read:
> 
>   if ( deny )
>      memset(info, 0, sizeof(info));
>   else
>      strncpy(info, xen_cap_info, sizeof(info));
> 
> to avoid double processing the start of the buffer, but given the ABI (must
> write 1k chars into the guest), I cannot see any way of taking info off the
> stack without some kind of strncpy_to_guest() API.

How about using clear_guest() for the 1k range, then copy_to_guest() for
merely the string? Plus - are we even required to clear the buffer past
the nul terminator?

Jan

Re: [PATCH v2 3/5] xen/version: Introduce non-truncating XENVER_* subops

[Jan Beulich]

On 14.01.2023 00:08, Andrew Cooper wrote:
> @@ -470,6 +471,59 @@ static int __init cf_check param_init(void)
>  __initcall(param_init);
>  #endif
>  
> +static long xenver_varbuf_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> +{
> +    struct xen_varbuf user_str;
> +    const char *str = NULL;

This takes away from the compiler any chance of reporting "str" as
uninitialized (particularly by future changes; the way it's here is
obvious enough of course). The NULL also doesn't buy you anything, as
...

> +    size_t sz;
> +
> +    switch ( cmd )
> +    {
> +    case XENVER_extraversion2:
> +        str = xen_extra_version();
> +        break;
> +
> +    case XENVER_changeset2:
> +        str = xen_changeset();
> +        break;
> +
> +    case XENVER_commandline2:
> +        str = saved_cmdline;
> +        break;
> +
> +    case XENVER_capabilities2:
> +        str = xen_cap_info;
> +        break;
> +
> +    default:
> +        ASSERT_UNREACHABLE();
> +        return -ENODATA;
> +    }
> +
> +    sz = strlen(str);

... we will still crash here in case the variable doesn't get any other
value assigned.

> +    if ( sz > KB(64) ) /* Arbitrary limit.  Avoid long-running operations. */
> +        return -E2BIG;
> +
> +    if ( guest_handle_is_null(arg) ) /* Length request */
> +        return sz;
> +
> +    if ( copy_from_guest(&user_str, arg, 1) )
> +        return -EFAULT;
> +
> +    if ( user_str.len == 0 )
> +        return -EINVAL;
> +
> +    if ( sz > user_str.len )
> +        return -ENOBUFS;

The earlier of these last two checks makes it that one can't successfully
call this function when the size query has returned 0.

> --- a/xen/include/public/version.h
> +++ b/xen/include/public/version.h
> @@ -19,12 +19,20 @@
>  /* arg == NULL; returns major:minor (16:16). */
>  #define XENVER_version      0
>  
> -/* arg == xen_extraversion_t. */
> +/*
> + * arg == xen_extraversion_t.
> + *
> + * This API/ABI is broken.  Use XENVER_extraversion2 instead.

Personally I don't like these "broken" that you're adding. These interfaces
simply are the way they are, with certain limitations. We also won't be
able to remove the old variants (except in the new ABI), so telling people
to avoid them provides us about nothing.

Jan

Re: [PATCH v2 4/5] xen/version: Fold build_id handling into xenver_varbuf_op()

[Jan Beulich]

On 14.01.2023 00:08, Andrew Cooper wrote:
> struct xen_build_id and struct xen_varbuf are identical from an ABI point of
> view, so XENVER_build_id can reuse xenver_varbuf_op() rather than having it's
> own almost identical copy of the logic.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit with a style related question plus remark:

> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -476,9 +476,22 @@ static long xenver_varbuf_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>      struct xen_varbuf user_str;
>      const char *str = NULL;
>      size_t sz;
> +    int rc;

Why is this declared here, yet ...

>      switch ( cmd )
>      {
> +    case XENVER_build_id:
> +    {
> +        unsigned int local_sz;

... this declared here? Both could live in switch()'s scope, allowing
for re-use in other case blocks, but making clear that the values are
unavailable outside of the switch().

> +        rc = xen_build_id((const void **)&str, &local_sz);
> +        if ( rc )
> +            return rc;
> +
> +        sz = local_sz;
> +        goto have_len;

Personally I certainly dislike "goto" in general, and I thought the
common grounds were to permit its use in error handling (only).

Jan

Re: [PATCH v2 5/5] xen/version: Misc style fixes

[Jan Beulich]

On 14.01.2023 00:08, Andrew Cooper wrote:
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v2 3/5] xen/version: Introduce non-truncating XENVER_* subops

[Julien Grall]

Hi,

On 16/01/2023 16:06, Jan Beulich wrote:
>> --- a/xen/include/public/version.h
>> +++ b/xen/include/public/version.h
>> @@ -19,12 +19,20 @@
>>   /* arg == NULL; returns major:minor (16:16). */
>>   #define XENVER_version      0
>>   
>> -/* arg == xen_extraversion_t. */
>> +/*
>> + * arg == xen_extraversion_t.
>> + *
>> + * This API/ABI is broken.  Use XENVER_extraversion2 instead.
> 
> Personally I don't like these "broken" that you're adding. These interfaces
> simply are the way they are, with certain limitations. We also won't be
> able to remove the old variants (except in the new ABI), so telling people
> to avoid them provides us about nothing.

+1. This is inline with the comment that I made in v1.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 2/5] xen/version: Calculate xen_capabilities_info once at boot

[Andrew Cooper]

On 16/01/2023 3:53 pm, Jan Beulich wrote:
> On 14.01.2023 00:08, Andrew Cooper wrote:
>> The arch_get_xen_caps() infrastructure is horribly inefficient, for something
>> that is constant after features have been resolved on boot.
>>
>> Every instance used snprintf() to format constants into a string (which gets
>> shorter when %d gets resolved!), which gets double buffered on the stack.
>>
>> Switch to using string literals with the "3.0" inserted - these numbers
>> haven't changed in 18 years (The Xen 3.0 release was Dec 5th 2005).
>>
>> Use initcalls to format the data into xen_cap_info, which is deliberately not
>> of type xen_capabilities_info_t because a 1k array is a silly overhead for
>> storing a maximum of 77 chars (the x86 version) and isn't liable to need any
>> more space in the forseeable future.
> So I was wondering if once we arrived at the new ABI (and hence the 3.0 one
> is properly legacy) we shouldn't declare Xen 5.0 and then also mark the new
> ABI's availability here by a string including "5.0" where at present we
> expose (only) "3.0".

"the new ABI" is is still two things.

The one part is changes to the in-guest ABI which does make it GPA based
(for HVM), but this does need to be broadly backwards compatible.  This
ABI string lives in the PV guest elfnotes (and is ultimately the thing
that distinguishes PV32pae vs PV64), but nowhere interesting for HVM
guests as far as I can see (furthermore, the 3 variations of hvm-3.0-
are bogus.

xen-3.0-x86_64la57 would probably be the least invasive way to extend PV
support to 5-level paging.

The other part is a stable tools API/ABI.  This can have any kind of
interface we choose, and frankly there are better interfaces than this
stringly typed one.


"xen-3.0" is even hardcoded in libelf.  I can't foresee a good reason to
bump 3 -> 5 and break all current PV guests.

>> If Xen had strncpy(), then the hunk in do_xen_version() could read:
>>
>>   if ( deny )
>>      memset(info, 0, sizeof(info));
>>   else
>>      strncpy(info, xen_cap_info, sizeof(info));
>>
>> to avoid double processing the start of the buffer, but given the ABI (must
>> write 1k chars into the guest), I cannot see any way of taking info off the
>> stack without some kind of strncpy_to_guest() API.
> How about using clear_guest() for the 1k range, then copy_to_guest() for
> merely the string? Plus - are we even required to clear the buffer past
> the nul terminator?

Well, we have previously always copied 1k bytes.  But this is always
been a NUL terminated API of a string persuasion, so I find it hard to
believe that any caller cares beyond the NUL.

Because of safe_strcpy(), xen_cap_info is guaranteed to be NUL
terminated, so if we don't care about padding the buffer with extra
zeroes, we don't even need the clear_guest().

Also, similar reasoning would apply to XENVER_cmdline which is typically
rather less than 1k in length (at least it's not on the stack, but it is
still an excessive copy).

~Andrew

Re: [PATCH v2 3/5] xen/version: Introduce non-truncating XENVER_* subops

[Andrew Cooper]

On 16/01/2023 4:06 pm, Jan Beulich wrote:
> On 14.01.2023 00:08, Andrew Cooper wrote:
>> @@ -470,6 +471,59 @@ static int __init cf_check param_init(void)
>>  __initcall(param_init);
>>  #endif
>>  
>> +static long xenver_varbuf_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>> +{
>> +    struct xen_varbuf user_str;
>> +    const char *str = NULL;
> This takes away from the compiler any chance of reporting "str" as
> uninitialized

Yes...

It is also the classic false positive pattern in GCC 4.x which is still
supported despite attempts to retire it.

>> +    if ( sz > KB(64) ) /* Arbitrary limit.  Avoid long-running operations. */
>> +        return -E2BIG;
>> +
>> +    if ( guest_handle_is_null(arg) ) /* Length request */
>> +        return sz;
>> +
>> +    if ( copy_from_guest(&user_str, arg, 1) )
>> +        return -EFAULT;
>> +
>> +    if ( user_str.len == 0 )
>> +        return -EINVAL;
>> +
>> +    if ( sz > user_str.len )
>> +        return -ENOBUFS;
> The earlier of these last two checks makes it that one can't successfully
> call this function when the size query has returned 0.

This is actually a check that the build_id path already has.  I did
consider it somewhat dubious to special case 0 here, but it needs to
stay for the following patch to have no functional change.

>> --- a/xen/include/public/version.h
>> +++ b/xen/include/public/version.h
>> @@ -19,12 +19,20 @@
>>  /* arg == NULL; returns major:minor (16:16). */
>>  #define XENVER_version      0
>>  
>> -/* arg == xen_extraversion_t. */
>> +/*
>> + * arg == xen_extraversion_t.
>> + *
>> + * This API/ABI is broken.  Use XENVER_extraversion2 instead.
> Personally I don't like these "broken" that you're adding. These interfaces
> simply are the way they are, with certain limitations. We also won't be
> able to remove the old variants (except in the new ABI), so telling people
> to avoid them provides us about nothing.

Incorrect.

First, the breakage here isn't only truncation; it's char-signedness
with data that's not guaranteed to be ASCII text.  Yet another
demonstration of why C is an inappropriate way of defining an ABI.

Secondly, it is unreasonable for ABI errors and correction information
such as this not to be documented *somewhere*.  It should live with the
API technical reference, which happens to be exactly (and only) here.

These comments won't fix existing implementations.  What they will do is
cause anyone new implementing guests, or anyone who re-syncs the
headers, to notice and hopefully take mitigating action.

~Andrew

Re: [PATCH v2 4/5] xen/version: Fold build_id handling into xenver_varbuf_op()

[Andrew Cooper]

On 16/01/2023 4:14 pm, Jan Beulich wrote:
> On 14.01.2023 00:08, Andrew Cooper wrote:
>> struct xen_build_id and struct xen_varbuf are identical from an ABI point of
>> view, so XENVER_build_id can reuse xenver_varbuf_op() rather than having it's
>> own almost identical copy of the logic.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>> --- a/xen/common/kernel.c
>> +++ b/xen/common/kernel.c
>> @@ -476,9 +476,22 @@ static long xenver_varbuf_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>      struct xen_varbuf user_str;
>>      const char *str = NULL;
>>      size_t sz;
>> +    int rc;
> Why is this declared here, yet ...
>
>>      switch ( cmd )
>>      {
>> +    case XENVER_build_id:
>> +    {
>> +        unsigned int local_sz;
> ... this declared here?

Because rc is more likely to be used outside in the future, and...

>  Both could live in switch()'s scope,

... this would have to be reverted tree-wide to use
trivial-initialisation hardening, which we absolutely should be doing by
default already.


I was sorely tempted to correct xen_build_id() to use size_t, but I
don't have time to correct everything which is wrong here.  That can
wait until later clean-up.

Alternatively, this is a pattern we have in quite a few places,
returning a {ptr, sz} pair.  All architectures we compile for (and even
x86 32bit given a suitable code-gen flag) are capable of returning at
least 2 GPRs worth of data (ARM can do 4), so switching to some kind of

struct pair {
    void *ptr;
    size_t sz;
};

return value would improve the code generation (and performance for that
matter) across the board by avoiding unnecessary spills of
pointers/sizes/secondary error information to the stack.

The wins for hvm get/set_segment_register() modest bug absolutely
worthwhile (and I notice I still haven't got those patches published. 
/sigh).

>> +        rc = xen_build_id((const void **)&str, &local_sz);
>> +        if ( rc )
>> +            return rc;
>> +
>> +        sz = local_sz;
>> +        goto have_len;
> Personally I certainly dislike "goto" in general, and I thought the
> common grounds were to permit its use in error handling (only).

That's not written in CODING_STYLE, nor has it (to my knowledge) ever
been an expressed view on xen-devel.

I don't use goto's gratuitously, and this one isn't.  Just try and write
this patch without a goto and then judge which version is cleaner/easier
to follow.

~Andrew

Re: [PATCH v2 2/5] xen/version: Calculate xen_capabilities_info once at boot

[Jan Beulich]

On 16.01.2023 21:52, Andrew Cooper wrote:
> On 16/01/2023 3:53 pm, Jan Beulich wrote:
>> On 14.01.2023 00:08, Andrew Cooper wrote:
>>> The arch_get_xen_caps() infrastructure is horribly inefficient, for something
>>> that is constant after features have been resolved on boot.
>>>
>>> Every instance used snprintf() to format constants into a string (which gets
>>> shorter when %d gets resolved!), which gets double buffered on the stack.
>>>
>>> Switch to using string literals with the "3.0" inserted - these numbers
>>> haven't changed in 18 years (The Xen 3.0 release was Dec 5th 2005).
>>>
>>> Use initcalls to format the data into xen_cap_info, which is deliberately not
>>> of type xen_capabilities_info_t because a 1k array is a silly overhead for
>>> storing a maximum of 77 chars (the x86 version) and isn't liable to need any
>>> more space in the forseeable future.
>> So I was wondering if once we arrived at the new ABI (and hence the 3.0 one
>> is properly legacy) we shouldn't declare Xen 5.0 and then also mark the new
>> ABI's availability here by a string including "5.0" where at present we
>> expose (only) "3.0".
> 
> "the new ABI" is is still two things.
> 
> The one part is changes to the in-guest ABI which does make it GPA based
> (for HVM), but this does need to be broadly backwards compatible.  This
> ABI string lives in the PV guest elfnotes (and is ultimately the thing
> that distinguishes PV32pae vs PV64), but nowhere interesting for HVM
> guests as far as I can see (furthermore, the 3 variations of hvm-3.0-
> are bogus.
> 
> xen-3.0-x86_64la57 would probably be the least invasive way to extend PV
> support to 5-level paging.
> 
> The other part is a stable tools API/ABI.  This can have any kind of
> interface we choose, and frankly there are better interfaces than this
> stringly typed one.
> 
> 
> "xen-3.0" is even hardcoded in libelf.  I can't foresee a good reason to
> bump 3 -> 5 and break all current PV guests.

I didn't by any means mean to suggest to bump. I was seeing us add new
5.0 entries, with the presence of the 3.0 ones indicating the backwards
compatible ABI. An option might then later be to make the compatible
ABI compile (kconfig) time conditional.

>>> If Xen had strncpy(), then the hunk in do_xen_version() could read:
>>>
>>>   if ( deny )
>>>      memset(info, 0, sizeof(info));
>>>   else
>>>      strncpy(info, xen_cap_info, sizeof(info));
>>>
>>> to avoid double processing the start of the buffer, but given the ABI (must
>>> write 1k chars into the guest), I cannot see any way of taking info off the
>>> stack without some kind of strncpy_to_guest() API.
>> How about using clear_guest() for the 1k range, then copy_to_guest() for
>> merely the string? Plus - are we even required to clear the buffer past
>> the nul terminator?
> 
> Well, we have previously always copied 1k bytes.  But this is always
> been a NUL terminated API of a string persuasion, so I find it hard to
> believe that any caller cares beyond the NUL.
> 
> Because of safe_strcpy(), xen_cap_info is guaranteed to be NUL
> terminated, so if we don't care about padding the buffer with extra
> zeroes, we don't even need the clear_guest().

Right, that's what I was hinting at as a possible option to do right here,
or to do subsequently.

> Also, similar reasoning would apply to XENVER_cmdline which is typically
> rather less than 1k in length (at least it's not on the stack, but it is
> still an excessive copy).

Indeed, but I understood your primary concern was the on-stack copy, so my
suggestion first focused on that.

Jan

Re: [PATCH v2 3/5] xen/version: Introduce non-truncating XENVER_* subops

[Jan Beulich]

On 16.01.2023 22:47, Andrew Cooper wrote:
> On 16/01/2023 4:06 pm, Jan Beulich wrote:
>> On 14.01.2023 00:08, Andrew Cooper wrote:
>>> @@ -470,6 +471,59 @@ static int __init cf_check param_init(void)
>>>  __initcall(param_init);
>>>  #endif
>>>  
>>> +static long xenver_varbuf_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>> +{
>>> +    struct xen_varbuf user_str;
>>> +    const char *str = NULL;
>> This takes away from the compiler any chance of reporting "str" as
>> uninitialized
> 
> Yes...
> 
> It is also the classic false positive pattern in GCC 4.x which is still
> supported despite attempts to retire it.

Hmm, I didn't think this was the classic pattern, but instead it was when
there are two variables, where the value of one identifies whether the
other was (also) initialized. But yes, if proven to be needed for 4.x,
then keeping it there would be unavoidable for the time being (but we
should then remind ourselves to drop this once we've raised the baseline,
perhaps short of adding a gcc version conditional around the initializer
right away).

>>> +    if ( sz > KB(64) ) /* Arbitrary limit.  Avoid long-running operations. */
>>> +        return -E2BIG;
>>> +
>>> +    if ( guest_handle_is_null(arg) ) /* Length request */
>>> +        return sz;
>>> +
>>> +    if ( copy_from_guest(&user_str, arg, 1) )
>>> +        return -EFAULT;
>>> +
>>> +    if ( user_str.len == 0 )
>>> +        return -EINVAL;
>>> +
>>> +    if ( sz > user_str.len )
>>> +        return -ENOBUFS;
>> The earlier of these last two checks makes it that one can't successfully
>> call this function when the size query has returned 0.
> 
> This is actually a check that the build_id path already has.  I did
> consider it somewhat dubious to special case 0 here, but it needs to
> stay for the following patch to have no functional change.

Would such a minor functional change actually be a problem there?

>>> --- a/xen/include/public/version.h
>>> +++ b/xen/include/public/version.h
>>> @@ -19,12 +19,20 @@
>>>  /* arg == NULL; returns major:minor (16:16). */
>>>  #define XENVER_version      0
>>>  
>>> -/* arg == xen_extraversion_t. */
>>> +/*
>>> + * arg == xen_extraversion_t.
>>> + *
>>> + * This API/ABI is broken.  Use XENVER_extraversion2 instead.
>> Personally I don't like these "broken" that you're adding. These interfaces
>> simply are the way they are, with certain limitations. We also won't be
>> able to remove the old variants (except in the new ABI), so telling people
>> to avoid them provides us about nothing.
> 
> Incorrect.
> 
> First, the breakage here isn't only truncation; it's char-signedness
> with data that's not guaranteed to be ASCII text.  Yet another
> demonstration of why C is an inappropriate way of defining an ABI.
> 
> Secondly, it is unreasonable for ABI errors and correction information
> such as this not to be documented *somewhere*.  It should live with the
> API technical reference, which happens to be exactly (and only) here.

I agree with spelling out shortcomings and restrictions. The only thing
I do not agree with is the use of the word "broken" (or anything to the
same effect). Instead of using that word, how about you actually spell
out what the limitations are, so that people have grounds to pick
between these and the new interfaces you're adding (and being nudged
towards the latter)?

Jan

Re: [PATCH v2 4/5] xen/version: Fold build_id handling into xenver_varbuf_op()

[Jan Beulich]

On 16.01.2023 23:14, Andrew Cooper wrote:
> On 16/01/2023 4:14 pm, Jan Beulich wrote:
>> On 14.01.2023 00:08, Andrew Cooper wrote:
>>> --- a/xen/common/kernel.c
>>> +++ b/xen/common/kernel.c
>>> @@ -476,9 +476,22 @@ static long xenver_varbuf_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>      struct xen_varbuf user_str;
>>>      const char *str = NULL;
>>>      size_t sz;
>>> +    int rc;
>> Why is this declared here, yet ...
>>
>>>      switch ( cmd )
>>>      {
>>> +    case XENVER_build_id:
>>> +    {
>>> +        unsigned int local_sz;
>> ... this declared here?
> 
> Because rc is more likely to be used outside in the future, and...
> 
>>  Both could live in switch()'s scope,
> 
> ... this would have to be reverted tree-wide to use
> trivial-initialisation hardening, which we absolutely should be doing by
> default already.

Interesting; thanks for giving some background.

> I was sorely tempted to correct xen_build_id() to use size_t, but I
> don't have time to correct everything which is wrong here.  That can
> wait until later clean-up.
> 
> Alternatively, this is a pattern we have in quite a few places,
> returning a {ptr, sz} pair.  All architectures we compile for (and even
> x86 32bit given a suitable code-gen flag) are capable of returning at
> least 2 GPRs worth of data (ARM can do 4), so switching to some kind of
> 
> struct pair {
>     void *ptr;
>     size_t sz;
> };
> 
> return value would improve the code generation (and performance for that
> matter) across the board by avoiding unnecessary spills of
> pointers/sizes/secondary error information to the stack.

Sounds like a possible plan (ideally we'd check with RISC-V and PPC as
well in this regard, as these look to be the two upcoming new ports).

> The wins for hvm get/set_segment_register() modest bug absolutely
> worthwhile (and I notice I still haven't got those patches published. 
> /sigh).

Did I ever post my 128-bit retval extension for altcall?

>>> +        rc = xen_build_id((const void **)&str, &local_sz);
>>> +        if ( rc )
>>> +            return rc;
>>> +
>>> +        sz = local_sz;
>>> +        goto have_len;
>> Personally I certainly dislike "goto" in general, and I thought the
>> common grounds were to permit its use in error handling (only).
> 
> That's not written in CODING_STYLE, nor has it (to my knowledge) ever
> been an expressed view on xen-devel.

It has been, but that was years ago.

> I don't use goto's gratuitously, and this one isn't.  Just try and write
> this patch without a goto and then judge which version is cleaner/easier
> to follow.

I wouldn't have given my R-b if I didn't realize that.

Jan

Re: [PATCH v2 3/5] xen/version: Introduce non-truncating XENVER_* subops

[Jason Andryuk]

On Fri, Jan 13, 2023 at 6:08 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> Recently in XenServer, we have encountered problems caused by both
> XENVER_extraversion and XENVER_commandline having fixed bounds.
>
> More than just the invariant size, the APIs/ABIs also broken by typedef-ing an
> array, and using an unqualified 'char' which has implementation-specific
> signed-ness
>
> Provide brand new ops, which are capable of expressing variable length
> strings, and mark the older ops as broken.
>
> This fixes all issues around XENVER_extraversion being longer than 15 chars.
> More work is required to remove other assumptions about XENVER_commandline
> being 1023 chars long.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Wei Liu <wl@xen.org>
> CC: Julien Grall <julien@xen.org>
> CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> CC: Daniel Smith <dpsmith@apertussolutions.com>
> CC: Jason Andryuk <jandryuk@gmail.com>
>
> v2:
>  * Remove xen_capabilities_info_t from the stack now that arch_get_xen_caps()
>    has gone.
>  * Use an arbitrary limit check much lower than INT_MAX.
>  * Use "buf" rather than "string" terminology.
>  * Expand the API comment.
>
> Tested by forcing XENVER_extraversion to be 20 chars long, and confirming that
> an untruncated version can be obtained.
> ---
>  xen/common/kernel.c          | 62 +++++++++++++++++++++++++++++++++++++++++++
>  xen/include/public/version.h | 63 ++++++++++++++++++++++++++++++++++++++++++--
>  xen/include/xlat.lst         |  1 +
>  xen/xsm/flask/hooks.c        |  4 +++

The Flask change looks good, so that part is:
Reviewed-by: Jason Andryuk <jandryuk@gmail.com> # Flask

Looking at include/xsm/dummy.h, these new subops would fall under the
default case and require XSM_PRIV.  Is that the desired permission,
and guests would just have to handle EPERM?

Regards,
Jason

Re: [PATCH v2 3/5] xen/version: Introduce non-truncating XENVER_* subops

[Andrew Cooper]

On 17/01/2023 4:21 pm, Jason Andryuk wrote:
> On Fri, Jan 13, 2023 at 6:08 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> Recently in XenServer, we have encountered problems caused by both
>> XENVER_extraversion and XENVER_commandline having fixed bounds.
>>
>> More than just the invariant size, the APIs/ABIs also broken by typedef-ing an
>> array, and using an unqualified 'char' which has implementation-specific
>> signed-ness
>>
>> Provide brand new ops, which are capable of expressing variable length
>> strings, and mark the older ops as broken.
>>
>> This fixes all issues around XENVER_extraversion being longer than 15 chars.
>> More work is required to remove other assumptions about XENVER_commandline
>> being 1023 chars long.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: George Dunlap <George.Dunlap@eu.citrix.com>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Wei Liu <wl@xen.org>
>> CC: Julien Grall <julien@xen.org>
>> CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>> CC: Daniel Smith <dpsmith@apertussolutions.com>
>> CC: Jason Andryuk <jandryuk@gmail.com>
>>
>> v2:
>>  * Remove xen_capabilities_info_t from the stack now that arch_get_xen_caps()
>>    has gone.
>>  * Use an arbitrary limit check much lower than INT_MAX.
>>  * Use "buf" rather than "string" terminology.
>>  * Expand the API comment.
>>
>> Tested by forcing XENVER_extraversion to be 20 chars long, and confirming that
>> an untruncated version can be obtained.
>> ---
>>  xen/common/kernel.c          | 62 +++++++++++++++++++++++++++++++++++++++++++
>>  xen/include/public/version.h | 63 ++++++++++++++++++++++++++++++++++++++++++--
>>  xen/include/xlat.lst         |  1 +
>>  xen/xsm/flask/hooks.c        |  4 +++
> The Flask change looks good, so that part is:
> Reviewed-by: Jason Andryuk <jandryuk@gmail.com> # Flask

Thanks.

>
> Looking at include/xsm/dummy.h, these new subops would fall under the
> default case and require XSM_PRIV.  Is that the desired permission,
> and guests would just have to handle EPERM?

I noticed that during further testing.  I made a modification to dummy
in the same manner as flask.

Given that it's the same piece of information (just with a less broken
API), permission handling for the two ops should be the same.

~Andrew