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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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@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
> @@ -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@xxxxxxxxx> # 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@xxxxxxxxx>

Thanks.

~ Oleksii