... 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;
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
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
© 2016 - 2026 Red Hat, Inc.