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 <[email protected]> >> Reviewed-by: Jason Andryuk <[email protected]> >> --- >> CC: George Dunlap <[email protected]> >> CC: Jan Beulich <[email protected]> >> CC: Stefano Stabellini <[email protected]> >> CC: Wei Liu <[email protected]> >> CC: Julien Grall <[email protected]> >> CC: Daniel De Graaf <[email protected]> >> CC: Daniel Smith <[email protected]> >> CC: Jason Andryuk <[email protected]> >> CC: Henry Wang <[email protected]> >> >> 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 >>
