| Message ID | 20200211134220.9194-2-sergey.dyasli@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xsm: hide detailed Xen version | expand |
On 11/02/2020 13:42, Sergey Dyasli wrote: > Add Kconfig option to make it possible to configure the string returned > to non-privileged guests instead of the default "<denied>" which could > propagate to UI / logs after the subsequent patch that hides detailed > Xen version information from unprivileged guests. > > Introduce XENVER_denied_string to allow guests to set up UI / logs > filtering which dependens on the new CONFIG_XSM_DENIED_STRING. No. This is even worse than other suggestions. It is entirely unacceptable to expect guests to have to modify them to figure out when they're being lied to. And it is now possible *without source code modifications* to create a Xen which reports one string in this hypercall, and has empty strings elsewhere, which is even more chaotic for guests. ~Andrew
On 11.02.2020 14:42, Sergey Dyasli wrote: > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -228,6 +228,14 @@ choice > bool "SILO" if XSM_SILO > endchoice > > +config XSM_DENIED_STRING > + string "xen_version hypercall denied information replacement string" > + default "<denied>" > + depends on XSM Why would this string want to be configurable only in XSM- enabled builds? Jan
On 11.02.2020 14:56, Andrew Cooper wrote: > On 11/02/2020 13:42, Sergey Dyasli wrote: >> Add Kconfig option to make it possible to configure the string returned >> to non-privileged guests instead of the default "<denied>" which could >> propagate to UI / logs after the subsequent patch that hides detailed >> Xen version information from unprivileged guests. >> >> Introduce XENVER_denied_string to allow guests to set up UI / logs >> filtering which dependens on the new CONFIG_XSM_DENIED_STRING. > > No. This is even worse than other suggestions. > > It is entirely unacceptable to expect guests to have to modify them to > figure out when they're being lied to. Why "lied to"? Denying data access is different from lying imo. Plus your proposal to return empty strings then is even more of a lie, for being not even recognizable a "access denied". > And it is now possible *without source code modifications* to create a > Xen which reports one string in this hypercall, and has empty strings > elsewhere, which is even more chaotic for guests. Empty strings elsewhere? Do you mean because of access having been denied, or because they in fact are empty? In the former case I'd like to ask for at least one example, while in the latter case I don't see what wrong you see. Jan
On 12/02/2020 09:32, Jan Beulich wrote: > On 11.02.2020 14:42, Sergey Dyasli wrote: >> --- a/xen/common/Kconfig >> +++ b/xen/common/Kconfig >> @@ -228,6 +228,14 @@ choice >> bool "SILO" if XSM_SILO >> endchoice >> >> +config XSM_DENIED_STRING >> + string "xen_version hypercall denied information replacement string" >> + default "<denied>" >> + depends on XSM > > Why would this string want to be configurable only in XSM- > enabled builds? For some reason I assumed that xsm_xen_version() is a no-op when CONFIG_XSM is undefined. I can now see that it doesn't depend on any config in which case the dependency (and #ifdef) should indeed be removed. -- Thanks, Sergey
diff --git a/xen/common/Kconfig b/xen/common/Kconfig index a6914fcae9..4a1a9398cd 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -228,6 +228,14 @@ choice bool "SILO" if XSM_SILO endchoice +config XSM_DENIED_STRING + string "xen_version hypercall denied information replacement string" + default "<denied>" + depends on XSM + ---help--- + A string which substitutes sensitive information returned via + xen_version hypercall to non-privileged guests + config LATE_HWDOM bool "Dedicated hardware domain" default n diff --git a/xen/common/kernel.c b/xen/common/kernel.c index 22941cec94..1c22e5d167 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -561,6 +561,17 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) return sz; } + + case XENVER_denied_string: + { + xen_denied_string_t str; + + safe_strcpy(str, xen_deny()); + if ( copy_to_guest(arg, str, XEN_DENIED_STRING_LEN) ) + return -EFAULT; + + return 0; + } } return -ENOSYS; diff --git a/xen/common/version.c b/xen/common/version.c index 937eb1281c..fbd0ef4668 100644 --- a/xen/common/version.c +++ b/xen/common/version.c @@ -67,7 +67,11 @@ const char *xen_banner(void) const char *xen_deny(void) { +#ifdef CONFIG_XSM_DENIED_STRING + return CONFIG_XSM_DENIED_STRING; +#else return "<denied>"; +#endif } static const void *build_id_p __read_mostly; diff --git a/xen/include/public/version.h b/xen/include/public/version.h index 17a81e23cd..f65001d2d9 100644 --- a/xen/include/public/version.h +++ b/xen/include/public/version.h @@ -100,6 +100,11 @@ struct xen_build_id { }; typedef struct xen_build_id xen_build_id_t; +/* arg == xen_denied_string_t. */ +#define XENVER_denied_string 11 +typedef char xen_denied_string_t[64]; +#define XEN_DENIED_STRING_LEN (sizeof(xen_denied_string_t)) + #endif /* __XEN_PUBLIC_VERSION_H__ */ /* diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index b8e185e6fa..72a101b106 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -748,6 +748,7 @@ static XSM_INLINE int xsm_xen_version (XSM_DEFAULT_ARG uint32_t op) case XENVER_version: case XENVER_platform_parameters: case XENVER_get_features: + case XENVER_denied_string: /* These sub-ops ignore the permission checks and return data. */ return 0; case XENVER_extraversion:
Add Kconfig option to make it possible to configure the string returned to non-privileged guests instead of the default "<denied>" which could propagate to UI / logs after the subsequent patch that hides detailed Xen version information from unprivileged guests. Introduce XENVER_denied_string to allow guests to set up UI / logs filtering which dependens on the new CONFIG_XSM_DENIED_STRING. Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> --- v3 --> v4: - Updated kconfig prompt description - Added XENVER_denied_string - Added #ifdef to fix build when CONFIG_XSM is not set v2 --> v3: - new patch --- xen/common/Kconfig | 8 ++++++++ xen/common/kernel.c | 11 +++++++++++ xen/common/version.c | 4 ++++ xen/include/public/version.h | 5 +++++ xen/include/xsm/dummy.h | 1 + 5 files changed, 29 insertions(+)