Re: [PATCH v3 02/10] xen/version: Introduce non-truncating deterministically-signed XENVER_* subops

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 15 Aug 2023, Andrew Cooper 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.
> Further work beyond just this API is needed to remove other assumptions about
> XENVER_commandline being 1023 chars long.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jason Andryuk <jandryuk@xxxxxxxxx>
> ---
> CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> CC: Daniel Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> CC: Jason Andryuk <jandryuk@xxxxxxxxx>
> CC: Henry Wang <Henry.Wang@xxxxxxx>
> 
> v3:
>  * Modify dummy.h's xsm_xen_version() in the same way as flask.
> 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/include/xsm/dummy.h      |  3 ++
>  xen/xsm/flask/hooks.c        |  4 +++
>  5 files changed, 131 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> index f822480a8ef3..79c008c7ee5f 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;
> @@ -498,6 +499,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;

Realistically do we want this buffer to cross page boundaries? We could
use KB(4) here or even KB(4)-4 (size of len).



> +    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);
> @@ -711,6 +765,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..0dd6bbcb43cc 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 where possible.

Like Jan and Julien I also don't like the word "broken" especially
without explanation of why it is broken next to it.

Instead, I would say:

"XENVER_extraversion is deprecated. Please use XENVER_extraversion2."

If you want to convey the message that the API has problems, then I would
say:

"XENVER_extraversion might cause truncation. Please use XENVER_extraversion2."

Or even:

"XENVER_extraversion has problems. Please use XENVER_extraversion2."



> + */
>  #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.

"XENVER_compile_info is deprecated and can truncate 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 where possible.

"XENVER_capabilities is deprecated. Please use XENVER_capabilities2."


> + */
>  #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 where possible.

"XENVER_changeset is deprecated. Please use XENVER_changeset2."


> + */
>  #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 where possible.

"XENVER_commandline is deprecated. Please use XENVER_commandline2."


> + */
>  #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.         */

I realize that you just copied struct xen_build_id but I recall from
MISRA C training that we should use plain "char" for strings for good
reasons, not "unsigned char"?

If this is meant to be generic data, I think we should use uint8_t
instead.


> +};
> +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__ */
>  
>  /*