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

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

Le 16/06/2026 à 11:05, Jan Beulich a écrit :
> ... 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.
I'm not sure to fully understand the commit message, is it more about 
possible PAGE_SIZE+1 allocations (hence 2 pages required) or something 
else ?
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> 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
> @@ -11,6 +11,8 @@ The format is based on [Keep a Changelog
>      to obtain an automatically allocated domid.  The prior sentinel values (0
>      since the start of Xen, and DOMID_INVALID since Xen 4.21) now no longer
>      represent a wildcard input.
> + - XEN_DOMCTL_DEV_DT's, FLASK_[GS]ETBOOL's, and FLASK_DEVICETREE_LABEL's input
> +   string sizes need to include the nul terminator.
>    - On x86:
>      - Enable pf-fixup option by default for PVH dom0.
>      - The libxenguest bzImage loader now uses the system liblz4 library.
> --- 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);
> +    }
EMSGSIZE feels a bit odd as the issue here is that there is no NUL 
terminator rather than the message being too large.
What about EILSEQ as we encountered a malformed string ?

>   
>       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;
The rest looks good to me.

Teddy

Attachment: OpenPGP_0x660FA9D102CBCFD0.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature