[PATCH v2 0/2] hypercall string inputs

[PATCH v2 0/2] hypercall string inputs

[Jan Beulich]

While doing the XSA-492 work I further noticed an inefficiency with
safe_copy_string_from_guest(). All callers pass PAGE_SIZE as the maximum
buffer size, and with the function adding 1 to append a nul terminator
the resulting allocations are all order-1 ones. Which we'd better avoid.
Require respective callers of hypercalls to nul-terminate the strings
within the buffer supplied. While an ABI change, I think it's an
acceptable one.

1: libxc: drop size parameter from xc_flask_context_to_sid()
2: lib: make safe_copy_string_from_guest() validate input

Jan

[PATCH v2 1/2] libxc: drop size parameter from xc_flask_context_to_sid()

[Jan Beulich]

Nul-terminated strings are passed in all cases, so the strlen() can very
well be invoked by the function itself. In preparation for a hypervisor
change also include the nul terminator in the size calculation.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Ideally libxl_flask_context_to_sid() would follow suit, but aiui doing so
would break its (stable) API.

Of course the casts in xc_flask_access() are suspicious.
---
v2: Avoid assert() use in libxl.

--- a/tools/helpers/init-xenstore-domain.c
+++ b/tools/helpers/init-xenstore-domain.c
@@ -108,7 +108,7 @@ static int build(xc_interface *xch)
 
     if ( flask )
     {
-        rv = xc_flask_context_to_sid(xch, flask, strlen(flask), &config.ssidref);
+        rv = xc_flask_context_to_sid(xch, flask, &config.ssidref);
         if ( rv )
         {
             fprintf(stderr, "xc_flask_context_to_sid failed\n");
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2372,7 +2372,7 @@ long xc_sharing_used_frames(xc_interface
 /*** End sharing interface ***/
 
 int xc_flask_load(xc_interface *xc_handle, char *buf, uint32_t size);
-int xc_flask_context_to_sid(xc_interface *xc_handle, char *buf, uint32_t size, uint32_t *sid);
+int xc_flask_context_to_sid(xc_interface *xc_handle, char *buf, uint32_t *sid);
 int xc_flask_sid_to_context(xc_interface *xc_handle, int sid, char *buf, uint32_t size);
 int xc_flask_getenforce(xc_interface *xc_handle);
 int xc_flask_setenforce(xc_interface *xc_handle, int mode);
--- a/tools/libs/ctrl/xc_flask.c
+++ b/tools/libs/ctrl/xc_flask.c
@@ -83,10 +83,11 @@ int xc_flask_load(xc_interface *xch, cha
     return err;
 }
 
-int xc_flask_context_to_sid(xc_interface *xch, char *buf, uint32_t size, uint32_t *sid)
+int xc_flask_context_to_sid(xc_interface *xch, char *buf, uint32_t *sid)
 {
     int err;
     struct xen_flask_op op = {};
+    size_t size = strlen(buf) + 1;
     DECLARE_HYPERCALL_BOUNCE(buf, size, XC_HYPERCALL_BUFFER_BOUNCE_IN);
 
     if ( xc_hypercall_bounce_pre(xch, buf) )
@@ -249,7 +250,7 @@ static int xc_flask_add(xc_interface *xc
     int err;
     struct xen_flask_op op = {};
 
-    err = xc_flask_context_to_sid(xch, scontext, strlen(scontext), &sid);
+    err = xc_flask_context_to_sid(xch, scontext, &sid);
     if ( err )
         return err;
 
@@ -325,10 +326,10 @@ int xc_flask_access(xc_interface *xch, c
     struct xen_flask_op op = {};
     int err;
 
-    err = xc_flask_context_to_sid(xch, (char*)scon, strlen(scon), &op.u.access.ssid);
+    err = xc_flask_context_to_sid(xch, (char*)scon, &op.u.access.ssid);
     if ( err )
         return err;
-    err = xc_flask_context_to_sid(xch, (char*)tcon, strlen(tcon), &op.u.access.tsid);
+    err = xc_flask_context_to_sid(xch, (char*)tcon, &op.u.access.tsid);
     if ( err )
         return err;
 
--- a/tools/libs/light/libxl_flask.c
+++ b/tools/libs/light/libxl_flask.c
@@ -21,7 +21,10 @@ int libxl_flask_context_to_sid(libxl_ctx
 {
     int rc;
 
-    rc = xc_flask_context_to_sid(ctx->xch, buf, len, ssidref);
+    if (len != strlen(buf))
+        return ERROR_INVAL;
+
+    rc = xc_flask_context_to_sid(ctx->xch, buf, ssidref);
 
     return rc;
 }
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -1754,7 +1754,7 @@ static PyObject *pyflask_context_to_sid(
         return PyErr_SetFromErrno(xc_error_obj);
     }
 
-    ret = xc_flask_context_to_sid(xc_handle, ctx, strlen(ctx), &sid);
+    ret = xc_flask_context_to_sid(xc_handle, ctx, &sid);
 
     xc_interface_close(xc_handle);
 

Re: [PATCH v2 1/2] libxc: drop size parameter from xc_flask_context_to_sid()

[Anthony PERARD]

[-- Attachment #1: Type: text/plain, Size: 566 bytes --]

On Wed, Jul 01, 2026 at 04:47:52PM +0200, Jan Beulich wrote:
> Nul-terminated strings are passed in all cases, so the strlen() can very
> well be invoked by the function itself. In preparation for a hypervisor
> change also include the nul terminator in the size calculation.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,


--
 | Vates

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v2 1/2] libxc: drop size parameter from xc_flask_context_to_sid()

[Daniel P. Smith]

On 7/1/26 10:47 AM, Jan Beulich wrote:
> Nul-terminated strings are passed in all cases, so the strlen() can very
> well be invoked by the function itself. In preparation for a hypervisor
> change also include the nul terminator in the size calculation.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> Ideally libxl_flask_context_to_sid() would follow suit, but aiui doing so
> would break its (stable) API.
> 
> Of course the casts in xc_flask_access() are suspicious.
> ---
> v2: Avoid assert() use in libxl.
> 
> --- a/tools/helpers/init-xenstore-domain.c
> +++ b/tools/helpers/init-xenstore-domain.c
> @@ -108,7 +108,7 @@ static int build(xc_interface *xch)
>   
>       if ( flask )
>       {
> -        rv = xc_flask_context_to_sid(xch, flask, strlen(flask), &config.ssidref);
> +        rv = xc_flask_context_to_sid(xch, flask, &config.ssidref);
>           if ( rv )
>           {
>               fprintf(stderr, "xc_flask_context_to_sid failed\n");
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -2372,7 +2372,7 @@ long xc_sharing_used_frames(xc_interface
>   /*** End sharing interface ***/
>   
>   int xc_flask_load(xc_interface *xc_handle, char *buf, uint32_t size);
> -int xc_flask_context_to_sid(xc_interface *xc_handle, char *buf, uint32_t size, uint32_t *sid);
> +int xc_flask_context_to_sid(xc_interface *xc_handle, char *buf, uint32_t *sid);
>   int xc_flask_sid_to_context(xc_interface *xc_handle, int sid, char *buf, uint32_t size);
>   int xc_flask_getenforce(xc_interface *xc_handle);
>   int xc_flask_setenforce(xc_interface *xc_handle, int mode);
> --- a/tools/libs/ctrl/xc_flask.c
> +++ b/tools/libs/ctrl/xc_flask.c
> @@ -83,10 +83,11 @@ int xc_flask_load(xc_interface *xch, cha
>       return err;
>   }
>   
> -int xc_flask_context_to_sid(xc_interface *xch, char *buf, uint32_t size, uint32_t *sid)
> +int xc_flask_context_to_sid(xc_interface *xch, char *buf, uint32_t *sid)
>   {
>       int err;
>       struct xen_flask_op op = {};
> +    size_t size = strlen(buf) + 1;
>       DECLARE_HYPERCALL_BOUNCE(buf, size, XC_HYPERCALL_BUFFER_BOUNCE_IN);
>   
>       if ( xc_hypercall_bounce_pre(xch, buf) )
> @@ -249,7 +250,7 @@ static int xc_flask_add(xc_interface *xc
>       int err;
>       struct xen_flask_op op = {};
>   
> -    err = xc_flask_context_to_sid(xch, scontext, strlen(scontext), &sid);
> +    err = xc_flask_context_to_sid(xch, scontext, &sid);
>       if ( err )
>           return err;
>   
> @@ -325,10 +326,10 @@ int xc_flask_access(xc_interface *xch, c
>       struct xen_flask_op op = {};
>       int err;
>   
> -    err = xc_flask_context_to_sid(xch, (char*)scon, strlen(scon), &op.u.access.ssid);
> +    err = xc_flask_context_to_sid(xch, (char*)scon, &op.u.access.ssid);
>       if ( err )
>           return err;
> -    err = xc_flask_context_to_sid(xch, (char*)tcon, strlen(tcon), &op.u.access.tsid);
> +    err = xc_flask_context_to_sid(xch, (char*)tcon, &op.u.access.tsid);
>       if ( err )
>           return err;
>   
> --- a/tools/libs/light/libxl_flask.c
> +++ b/tools/libs/light/libxl_flask.c
> @@ -21,7 +21,10 @@ int libxl_flask_context_to_sid(libxl_ctx
>   {
>       int rc;
>   
> -    rc = xc_flask_context_to_sid(ctx->xch, buf, len, ssidref);
> +    if (len != strlen(buf))
> +        return ERROR_INVAL;
> +
> +    rc = xc_flask_context_to_sid(ctx->xch, buf, ssidref);
>   
>       return rc;
>   }
> --- a/tools/python/xen/lowlevel/xc/xc.c
> +++ b/tools/python/xen/lowlevel/xc/xc.c
> @@ -1754,7 +1754,7 @@ static PyObject *pyflask_context_to_sid(
>           return PyErr_SetFromErrno(xc_error_obj);
>       }
>   
> -    ret = xc_flask_context_to_sid(xc_handle, ctx, strlen(ctx), &sid);
> +    ret = xc_flask_context_to_sid(xc_handle, ctx, &sid);
>   
>       xc_interface_close(xc_handle);
>   
> 

Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>

[PATCH v2 2/2] lib: make safe_copy_string_from_guest() validate input

[Jan Beulich]

... rather than papering over guest flaws: Strings passed ought to be nul-
terminated (yet sadly libxc hasn't been doing so thus far). This way we
also avoid order-1 allocations, seeing that all present callers pass
PAGE_SIZE for max_size.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I can't spot any caller side use of FLASK_DEVICETREE_LABEL, hence there's
no corresponding prereq patch.

--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,8 @@ The format is based on [Keep a Changelog
 ## [4.23.0 UNRELEASED](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=staging) - TBD
 
 ### Changed
+ - XEN_DOMCTL_DEV_DT's, FLASK_[GS]ETBOOL's, and FLASK_DEVICETREE_LABEL's input
+   string sizes need to include the nul terminator.
 
 ### Added
 
--- a/xen/lib/guest-strcpy.c
+++ b/xen/lib/guest-strcpy.c
@@ -3,8 +3,8 @@
 #include <xen/err.h>
 
 /*
- * The function copies a string from the guest and adds a NUL to
- * make sure the string is correctly terminated.
+ * The function copies a string from the guest and checks there's a NUL
+ * terminating the string.
  */
 char *safe_copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf,
                                   size_t size, size_t max_size)
@@ -14,8 +14,7 @@ char *safe_copy_string_from_guest(XEN_GU
     if ( size > max_size )
         return ERR_PTR(-ENOBUFS);
 
-    /* Add an extra +1 to append \0 */
-    tmp = xmalloc_array(char, size + 1);
+    tmp = xmalloc_array(char, size);
     if ( !tmp )
         return ERR_PTR(-ENOMEM);
 
@@ -24,7 +23,12 @@ char *safe_copy_string_from_guest(XEN_GU
         xfree(tmp);
         return ERR_PTR(-EFAULT);
     }
-    tmp[size] = '\0';
+
+    if ( !memchr(tmp, 0, size) )
+    {
+        xfree(tmp);
+        return ERR_PTR(-EMSGSIZE);
+    }
 
     return tmp;
 }
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -574,7 +574,7 @@ struct xen_domctl_assign_device {
             uint32_t machine_sbdf;   /* machine PCI ID of assigned device */
         } pci;
         struct {
-            uint32_t size; /* Length of the path */
+            uint32_t size; /* Length of the path, including nul terminator */
             XEN_GUEST_HANDLE_64(char) path; /* Path to the device tree node */
 #ifdef __XEN__
             struct dt_device_node *dev; /* Resolved device node of the above */
--- a/xen/include/public/xsm/flask_op.h
+++ b/xen/include/public/xsm/flask_op.h
@@ -26,7 +26,8 @@ typedef struct xen_flask_setenforce xen_
 struct xen_flask_sid_context {
     /* IN/OUT: sid to convert to/from string */
     uint32_t sid;
-    /* IN: size of the context buffer
+    /*
+     * IN: size of the context buffer, including nul terminator
      * OUT: actual size of the output context string
      */
     uint32_t size;
@@ -86,8 +87,11 @@ struct xen_flask_boolean {
     uint8_t new_value;
     /* IN: commit new value instead of only setting pending [SET] */
     uint8_t commit;
-    /* IN: size of boolean name buffer [GET/SET]
-     * OUT: actual size of name [GET only] */
+    /*
+     * IN: size of boolean name buffer [GET/SET]; must cover nul terminator
+     *     if "name" (below) is an input
+     * OUT: actual size of name [GET only]
+     */
     uint32_t size;
     /* IN: if bool_id is -1, used to find boolean [GET/SET]
      * OUT: textual name of boolean [GET only]
@@ -150,7 +154,7 @@ typedef struct xen_flask_relabel xen_fla
 struct xen_flask_devicetree_label {
     /* IN */
     uint32_t sid;
-    uint32_t length;
+    uint32_t length; /* length of the path, including nul terminator */
     XEN_GUEST_HANDLE(char) path;
 };
 typedef struct xen_flask_devicetree_label xen_flask_devicetree_label_t;

Re: [PATCH v2 2/2] lib: make safe_copy_string_from_guest() validate input

[Anthony PERARD]

[-- Attachment #1: Type: text/plain, Size: 556 bytes --]

On Wed, Jul 01, 2026 at 04:48:33PM +0200, Jan Beulich wrote:
> ... rather than papering over guest flaws: Strings passed ought to be nul-
> terminated (yet sadly libxc hasn't been doing so thus far). This way we
> also avoid order-1 allocations, seeing that all present callers pass
> PAGE_SIZE for max_size.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v2 2/2] lib: make safe_copy_string_from_guest() validate input

[Oleksii Kurochko]

On 7/1/26 4:48 PM, Jan Beulich wrote:
> ... rather than papering over guest flaws: Strings passed ought to be nul-
> terminated (yet sadly libxc hasn't been doing so thus far). This way we
> also avoid order-1 allocations, seeing that all present callers pass
> PAGE_SIZE for max_size.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I can't spot any caller side use of FLASK_DEVICETREE_LABEL, hence there's
> no corresponding prereq patch.
> 
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -7,6 +7,8 @@ The format is based on [Keep a Changelog
>   ## [4.23.0 UNRELEASED](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=staging) - TBD
>   
>   ### Changed
> + - XEN_DOMCTL_DEV_DT's, FLASK_[GS]ETBOOL's, and FLASK_DEVICETREE_LABEL's input
> +   string sizes need to include the nul terminator.
>   

Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> # Changelog

>   ### Added
>   
> --- a/xen/lib/guest-strcpy.c
> +++ b/xen/lib/guest-strcpy.c
> @@ -3,8 +3,8 @@
>   #include <xen/err.h>
>   
>   /*
> - * The function copies a string from the guest and adds a NUL to
> - * make sure the string is correctly terminated.
> + * The function copies a string from the guest and checks there's a NUL
> + * terminating the string.
>    */
>   char *safe_copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf,
>                                     size_t size, size_t max_size)
> @@ -14,8 +14,7 @@ char *safe_copy_string_from_guest(XEN_GU
>       if ( size > max_size )
>           return ERR_PTR(-ENOBUFS);
>   
> -    /* Add an extra +1 to append \0 */
> -    tmp = xmalloc_array(char, size + 1);
> +    tmp = xmalloc_array(char, size);
>       if ( !tmp )
>           return ERR_PTR(-ENOMEM);
>   
> @@ -24,7 +23,12 @@ char *safe_copy_string_from_guest(XEN_GU
>           xfree(tmp);
>           return ERR_PTR(-EFAULT);
>       }
> -    tmp[size] = '\0';
> +
> +    if ( !memchr(tmp, 0, size) )
> +    {
> +        xfree(tmp);
> +        return ERR_PTR(-EMSGSIZE);
> +    }
>   
>       return tmp;
>   }
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -574,7 +574,7 @@ struct xen_domctl_assign_device {
>               uint32_t machine_sbdf;   /* machine PCI ID of assigned device */
>           } pci;
>           struct {
> -            uint32_t size; /* Length of the path */
> +            uint32_t size; /* Length of the path, including nul terminator */
>               XEN_GUEST_HANDLE_64(char) path; /* Path to the device tree node */
>   #ifdef __XEN__
>               struct dt_device_node *dev; /* Resolved device node of the above */
> --- a/xen/include/public/xsm/flask_op.h
> +++ b/xen/include/public/xsm/flask_op.h
> @@ -26,7 +26,8 @@ typedef struct xen_flask_setenforce xen_
>   struct xen_flask_sid_context {
>       /* IN/OUT: sid to convert to/from string */
>       uint32_t sid;
> -    /* IN: size of the context buffer
> +    /*
> +     * IN: size of the context buffer, including nul terminator
>        * OUT: actual size of the output context string
>        */
>       uint32_t size;
> @@ -86,8 +87,11 @@ struct xen_flask_boolean {
>       uint8_t new_value;
>       /* IN: commit new value instead of only setting pending [SET] */
>       uint8_t commit;
> -    /* IN: size of boolean name buffer [GET/SET]
> -     * OUT: actual size of name [GET only] */
> +    /*
> +     * IN: size of boolean name buffer [GET/SET]; must cover nul terminator
> +     *     if "name" (below) is an input
> +     * OUT: actual size of name [GET only]
> +     */
>       uint32_t size;
>       /* IN: if bool_id is -1, used to find boolean [GET/SET]
>        * OUT: textual name of boolean [GET only]
> @@ -150,7 +154,7 @@ typedef struct xen_flask_relabel xen_fla
>   struct xen_flask_devicetree_label {
>       /* IN */
>       uint32_t sid;
> -    uint32_t length;
> +    uint32_t length; /* length of the path, including nul terminator */
>       XEN_GUEST_HANDLE(char) path;
>   };
>   typedef struct xen_flask_devicetree_label xen_flask_devicetree_label_t;
> 

Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii