| Message ID | 20230113230835.29356-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix truncation of various XENVER_* subops | expand |
On 14.01.2023 00:08, Andrew Cooper wrote: > @@ -470,6 +471,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; This takes away from the compiler any chance of reporting "str" as uninitialized (particularly by future changes; the way it's here is obvious enough of course). The NULL also doesn't buy you anything, as ... > + 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); ... we will still crash here in case the variable doesn't get any other value assigned. > + 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; The earlier of these last two checks makes it that one can't successfully call this function when the size query has returned 0. > --- 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 instead. Personally I don't like these "broken" that you're adding. These interfaces simply are the way they are, with certain limitations. We also won't be able to remove the old variants (except in the new ABI), so telling people to avoid them provides us about nothing. Jan
Hi, On 16/01/2023 16:06, Jan Beulich wrote: >> --- 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 instead. > > Personally I don't like these "broken" that you're adding. These interfaces > simply are the way they are, with certain limitations. We also won't be > able to remove the old variants (except in the new ABI), so telling people > to avoid them provides us about nothing. +1. This is inline with the comment that I made in v1. Cheers,
On 16/01/2023 4:06 pm, Jan Beulich wrote: > On 14.01.2023 00:08, Andrew Cooper wrote: >> @@ -470,6 +471,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; > This takes away from the compiler any chance of reporting "str" as > uninitialized Yes... It is also the classic false positive pattern in GCC 4.x which is still supported despite attempts to retire it. >> + 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; > The earlier of these last two checks makes it that one can't successfully > call this function when the size query has returned 0. This is actually a check that the build_id path already has. I did consider it somewhat dubious to special case 0 here, but it needs to stay for the following patch to have no functional change. >> --- 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 instead. > Personally I don't like these "broken" that you're adding. These interfaces > simply are the way they are, with certain limitations. We also won't be > able to remove the old variants (except in the new ABI), so telling people > to avoid them provides us about nothing. Incorrect. First, the breakage here isn't only truncation; it's char-signedness with data that's not guaranteed to be ASCII text. Yet another demonstration of why C is an inappropriate way of defining an ABI. Secondly, it is unreasonable for ABI errors and correction information such as this not to be documented *somewhere*. It should live with the API technical reference, which happens to be exactly (and only) here. These comments won't fix existing implementations. What they will do is cause anyone new implementing guests, or anyone who re-syncs the headers, to notice and hopefully take mitigating action. ~Andrew
On 16.01.2023 22:47, Andrew Cooper wrote: > On 16/01/2023 4:06 pm, Jan Beulich wrote: >> On 14.01.2023 00:08, Andrew Cooper wrote: >>> @@ -470,6 +471,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; >> This takes away from the compiler any chance of reporting "str" as >> uninitialized > > Yes... > > It is also the classic false positive pattern in GCC 4.x which is still > supported despite attempts to retire it. Hmm, I didn't think this was the classic pattern, but instead it was when there are two variables, where the value of one identifies whether the other was (also) initialized. But yes, if proven to be needed for 4.x, then keeping it there would be unavoidable for the time being (but we should then remind ourselves to drop this once we've raised the baseline, perhaps short of adding a gcc version conditional around the initializer right away). >>> + 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; >> The earlier of these last two checks makes it that one can't successfully >> call this function when the size query has returned 0. > > This is actually a check that the build_id path already has. I did > consider it somewhat dubious to special case 0 here, but it needs to > stay for the following patch to have no functional change. Would such a minor functional change actually be a problem there? >>> --- 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 instead. >> Personally I don't like these "broken" that you're adding. These interfaces >> simply are the way they are, with certain limitations. We also won't be >> able to remove the old variants (except in the new ABI), so telling people >> to avoid them provides us about nothing. > > Incorrect. > > First, the breakage here isn't only truncation; it's char-signedness > with data that's not guaranteed to be ASCII text. Yet another > demonstration of why C is an inappropriate way of defining an ABI. > > Secondly, it is unreasonable for ABI errors and correction information > such as this not to be documented *somewhere*. It should live with the > API technical reference, which happens to be exactly (and only) here. I agree with spelling out shortcomings and restrictions. The only thing I do not agree with is the use of the word "broken" (or anything to the same effect). Instead of using that word, how about you actually spell out what the limitations are, so that people have grounds to pick between these and the new interfaces you're adding (and being nudged towards the latter)? Jan
On Fri, Jan 13, 2023 at 6:08 PM Andrew Cooper <andrew.cooper3@citrix.com> 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. > More work is required to remove other assumptions about XENVER_commandline > being 1023 chars long. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: George Dunlap <George.Dunlap@eu.citrix.com> > CC: Jan Beulich <JBeulich@suse.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Wei Liu <wl@xen.org> > CC: Julien Grall <julien@xen.org> > CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> > CC: Daniel Smith <dpsmith@apertussolutions.com> > CC: Jason Andryuk <jandryuk@gmail.com> > > 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/xsm/flask/hooks.c | 4 +++ The Flask change looks good, so that part is: Reviewed-by: Jason Andryuk <jandryuk@gmail.com> # Flask Looking at include/xsm/dummy.h, these new subops would fall under the default case and require XSM_PRIV. Is that the desired permission, and guests would just have to handle EPERM? Regards, Jason
On 17/01/2023 4:21 pm, Jason Andryuk wrote: > On Fri, Jan 13, 2023 at 6:08 PM Andrew Cooper <andrew.cooper3@citrix.com> 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. >> More work is required to remove other assumptions about XENVER_commandline >> being 1023 chars long. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: George Dunlap <George.Dunlap@eu.citrix.com> >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Stefano Stabellini <sstabellini@kernel.org> >> CC: Wei Liu <wl@xen.org> >> CC: Julien Grall <julien@xen.org> >> CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> >> CC: Daniel Smith <dpsmith@apertussolutions.com> >> CC: Jason Andryuk <jandryuk@gmail.com> >> >> 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/xsm/flask/hooks.c | 4 +++ > The Flask change looks good, so that part is: > Reviewed-by: Jason Andryuk <jandryuk@gmail.com> # Flask Thanks. > > Looking at include/xsm/dummy.h, these new subops would fall under the > default case and require XSM_PRIV. Is that the desired permission, > and guests would just have to handle EPERM? I noticed that during further testing. I made a modification to dummy in the same manner as flask. Given that it's the same piece of information (just with a less broken API), permission handling for the two ops should be the same. ~Andrew
diff --git a/xen/common/kernel.c b/xen/common/kernel.c index 4fa1d6710115..cc5d8247b04d 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; @@ -470,6 +471,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); @@ -683,6 +737,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..9287b5d3ff50 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 instead. + */ #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 instead. + */ #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 instead. + */ #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 instead. + */ #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 d601a8a98421..762c8a77fb27 100644 --- a/xen/include/xlat.lst +++ b/xen/include/xlat.lst @@ -172,6 +172,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/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:
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. More work is required to remove other assumptions about XENVER_commandline being 1023 chars long. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: George Dunlap <George.Dunlap@eu.citrix.com> CC: Jan Beulich <JBeulich@suse.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Wei Liu <wl@xen.org> CC: Julien Grall <julien@xen.org> CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> CC: Daniel Smith <dpsmith@apertussolutions.com> CC: Jason Andryuk <jandryuk@gmail.com> 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/xsm/flask/hooks.c | 4 +++ 4 files changed, 128 insertions(+), 2 deletions(-)