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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.11.2023 23:30, Stefano Stabellini wrote:
> Hi everyone following this thread,
> 
> please see:
> https://marc.info/?l=xen-devel&m=170135718323946
> https://cryptpad.fr/form/#/2/form/view/7ByH95Vd7KiDOvN4wjV5iUGlMuZbkVdwk7cYpZdluWo/
> 
> For a vote on the usage of the word "broken"

So I did vote before becoming aware of this context. I would have voted
differently if I had known that this _alone_ is the context. Yet then
I'm also not going to change my vote, because as written _there_ it is
intended to be more general. If the wording of the text describing what
to vote on changed, things would be different.

Jan

> 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;
>> +
>> +    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.
>> + */
>>  #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.
>> + */
>>  #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.
>> + */
>>  #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.
>> + */
>>  #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.
>> + */
>>  #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.         
>> */
>> +};
>> +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__ */
>>  
>>  /*
>> diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
>> index 9c41948514bf..a61ba85ed0ca 100644
>> --- a/xen/include/xlat.lst
>> +++ b/xen/include/xlat.lst
>> @@ -173,6 +173,7 @@
>>  ?   build_id                        version.h
>>  ?   compile_info                    version.h
>>  ?   feature_info                    version.h
>> +?   varbuf                          version.h
>>  ?   xenoprof_init                   xenoprof.h
>>  ?   xenoprof_passive                xenoprof.h
>>  ?   flask_access                    xsm/flask_op.h
>> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
>> index 8671af1ba4d3..a4a920f74e6e 100644
>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -828,9 +828,12 @@ static XSM_INLINE int cf_check 
>> xsm_xen_version(XSM_DEFAULT_ARG uint32_t op)
>>          block_speculation();
>>          return 0;
>>      case XENVER_extraversion:
>> +    case XENVER_extraversion2:
>>      case XENVER_compile_info:
>>      case XENVER_capabilities:
>> +    case XENVER_capabilities2:
>>      case XENVER_changeset:
>> +    case XENVER_changeset2:
>>      case XENVER_pagesize:
>>      case XENVER_guest_handle:
>>          /* These MUST always be accessible to any guest by default. */
>> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
>> index 78225f68c15c..a671dcd0322e 100644
>> --- a/xen/xsm/flask/hooks.c
>> +++ b/xen/xsm/flask/hooks.c
>> @@ -1777,15 +1777,18 @@ static int cf_check flask_xen_version(uint32_t op)
>>          /* These sub-ops ignore the permission checks and return data. */
>>          return 0;
>>      case XENVER_extraversion:
>> +    case XENVER_extraversion2:
>>          return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION,
>>                              VERSION__XEN_EXTRAVERSION, NULL);
>>      case XENVER_compile_info:
>>          return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION,
>>                              VERSION__XEN_COMPILE_INFO, NULL);
>>      case XENVER_capabilities:
>> +    case XENVER_capabilities2:
>>          return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION,
>>                              VERSION__XEN_CAPABILITIES, NULL);
>>      case XENVER_changeset:
>> +    case XENVER_changeset2:
>>          return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION,
>>                              VERSION__XEN_CHANGESET, NULL);
>>      case XENVER_pagesize:
>> @@ -1795,6 +1798,7 @@ static int cf_check flask_xen_version(uint32_t op)
>>          return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION,
>>                              VERSION__XEN_GUEST_HANDLE, NULL);
>>      case XENVER_commandline:
>> +    case XENVER_commandline2:
>>          return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION,
>>                              VERSION__XEN_COMMANDLINE, NULL);
>>      case XENVER_build_id:
>> -- 
>> 2.30.2
>>