| Message ID | 20191217154625.31561-1-sergey.dyasli@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xsm: hide detailed Xen version from unprivileged guests | expand |
On 17.12.2019 16:46, Sergey Dyasli wrote: > Hide the following information that can help identify the running Xen > binary version: > > XENVER_extraversion > XENVER_compile_info > XENVER_capabilities What's wrong with exposing this one? > XENVER_changeset > XENVER_commandline > XENVER_build_id > > Return a more customer friendly empty string instead of "<denied>" > which would be shown in tools like dmidecode. I think "<denied>" is quite fine for many of the original purposes. Maybe it would be better to filter for this when populating guest DMI tables? > But allow guests to see this information in Debug builds of Xen. Behavior like this would imo better not differ between debug and release builds, or else guest software may behave entirely differently once you put it on a production build. > --- a/xen/include/xsm/dummy.h > +++ b/xen/include/xsm/dummy.h > @@ -750,16 +750,21 @@ static XSM_INLINE int xsm_xen_version (XSM_DEFAULT_ARG uint32_t op) > case XENVER_get_features: > /* These sub-ops ignore the permission checks and return data. */ > return 0; > - case XENVER_extraversion: > - case XENVER_compile_info: > - case XENVER_capabilities: > - case XENVER_changeset: > case XENVER_pagesize: > case XENVER_guest_handle: > /* These MUST always be accessible to any guest by default. */ This comment, not the least because of its use of capitals, suggests to me that there's further justification needed for your change, including discussion of why there's no risk of breaking existing guests. > return xsm_default_action(XSM_HOOK, current->domain, NULL); > + > + case XENVER_extraversion: > + case XENVER_compile_info: > + case XENVER_capabilities: > + case XENVER_changeset: > + case XENVER_commandline: > + case XENVER_build_id: > default: There's no need to add all of these next to the default case. Note how commandline and build_id have been coming here already (and there would need to be further justification for exposing them on debug builds, should this questionable behavior - see above - be retained). Jan
On 18/12/2019 11:06, Jan Beulich wrote: > On 17.12.2019 16:46, Sergey Dyasli wrote: >> Hide the following information that can help identify the running Xen >> binary version: >> >> XENVER_extraversion >> XENVER_compile_info >> XENVER_capabilities > > What's wrong with exposing this one? extraversion can help identify the exact running Xen binary (and I must say that at Citrix we add some additional version information there). compile_info will identify compiler and many speculative mitigations are provided by compilers these days. Not sure if it's worth hiding capabilities, but OTOH I don't see how guests could meaningfully use it. > >> XENVER_changeset >> XENVER_commandline >> XENVER_build_id >> >> Return a more customer friendly empty string instead of "<denied>" >> which would be shown in tools like dmidecode.> > I think "<denied>" is quite fine for many of the original purposes. > Maybe it would be better to filter for this when populating guest > DMI tables? I don't know how DMI tables are populated, but nothing stops a guest from using these hypercalls directly. > >> But allow guests to see this information in Debug builds of Xen. > > Behavior like this would imo better not differ between debug and > release builds, or else guest software may behave entirely > differently once you put it on a production build. I agree on this one, it's not much worth providing this information in debug builds. > >> --- a/xen/include/xsm/dummy.h >> +++ b/xen/include/xsm/dummy.h >> @@ -750,16 +750,21 @@ static XSM_INLINE int xsm_xen_version (XSM_DEFAULT_ARG uint32_t op) >> case XENVER_get_features: >> /* These sub-ops ignore the permission checks and return data. */ >> return 0; >> - case XENVER_extraversion: >> - case XENVER_compile_info: >> - case XENVER_capabilities: >> - case XENVER_changeset: >> case XENVER_pagesize: >> case XENVER_guest_handle: >> /* These MUST always be accessible to any guest by default. */ > > This comment, not the least because of its use of capitals, > suggests to me that there's further justification needed for > your change, including discussion of why there's no risk of > breaking existing guests. Not sure about this comment, maybe the author (Konrad) remembers? We had this change in production for very long time now and haven't seen any guest regressions. > >> return xsm_default_action(XSM_HOOK, current->domain, NULL); >> + >> + case XENVER_extraversion: >> + case XENVER_compile_info: >> + case XENVER_capabilities: >> + case XENVER_changeset: >> + case XENVER_commandline: >> + case XENVER_build_id: >> default: > > There's no need to add all of these next to the default case. > Note how commandline and build_id have been coming here already > (and there would need to be further justification for exposing > them on debug builds, should this questionable behavior - see > above - be retained). I find that explicitly listing all the cases is better for code readability, but I don't have a strong opinion here. -- Thanks, Sergey
On 19.12.2019 12:23, Sergey Dyasli wrote: > On 18/12/2019 11:06, Jan Beulich wrote: >> On 17.12.2019 16:46, Sergey Dyasli wrote: >>> Hide the following information that can help identify the running Xen >>> binary version: >>> >>> XENVER_extraversion >>> XENVER_compile_info >>> XENVER_capabilities >> >> What's wrong with exposing this one? > > extraversion can help identify the exact running Xen binary (and I must > say that at Citrix we add some additional version information there). > compile_info will identify compiler and many speculative mitigations > are provided by compilers these days. Not sure if it's worth hiding > capabilities, but OTOH I don't see how guests could meaningfully use it. Well, my question (using "this", not "these") was really mainly on the last item. I can see how extraversion can provide clues. I'm having difficulty seeing how the compiler (little bit of) details can provide sufficient information to become leveragable. >>> XENVER_changeset >>> XENVER_commandline >>> XENVER_build_id >>> >>> Return a more customer friendly empty string instead of "<denied>" >>> which would be shown in tools like dmidecode.> >> I think "<denied>" is quite fine for many of the original purposes. >> Maybe it would be better to filter for this when populating guest >> DMI tables? > > I don't know how DMI tables are populated, but nothing stops a guest > from using these hypercalls directly. And this is precisely the case where I think "<denied>" is better than an empty string. >>> return xsm_default_action(XSM_HOOK, current->domain, NULL); >>> + >>> + case XENVER_extraversion: >>> + case XENVER_compile_info: >>> + case XENVER_capabilities: >>> + case XENVER_changeset: >>> + case XENVER_commandline: >>> + case XENVER_build_id: >>> default: >> >> There's no need to add all of these next to the default case. >> Note how commandline and build_id have been coming here already >> (and there would need to be further justification for exposing >> them on debug builds, should this questionable behavior - see >> above - be retained). > > I find that explicitly listing all the cases is better for code > readability, but I don't have a strong opinion here. Well, I'm viewing it kind of the opposite, as being unnecessary clutter (and hence harming readability). We'll see what others think. Jan
Hi, On 19/12/2019 11:35, Jan Beulich wrote: > On 19.12.2019 12:23, Sergey Dyasli wrote: >> On 18/12/2019 11:06, Jan Beulich wrote: >>> On 17.12.2019 16:46, Sergey Dyasli wrote: >>>> Hide the following information that can help identify the running Xen >>>> binary version: >>>> >>>> XENVER_extraversion >>>> XENVER_compile_info >>>> XENVER_capabilities >>> >>> What's wrong with exposing this one? >> >> extraversion can help identify the exact running Xen binary (and I must >> say that at Citrix we add some additional version information there). >> compile_info will identify compiler and many speculative mitigations >> are provided by compilers these days. Not sure if it's worth hiding >> capabilities, but OTOH I don't see how guests could meaningfully use it. > > Well, my question (using "this", not "these") was really mainly on > the last item. I can see how extraversion can provide clues. I'm > having difficulty seeing how the compiler (little bit of) details > can provide sufficient information to become leveragable. > >>>> XENVER_changeset >>>> XENVER_commandline >>>> XENVER_build_id >>>> >>>> Return a more customer friendly empty string instead of "<denied>" >>>> which would be shown in tools like dmidecode.> >>> I think "<denied>" is quite fine for many of the original purposes. >>> Maybe it would be better to filter for this when populating guest >>> DMI tables? >> >> I don't know how DMI tables are populated, but nothing stops a guest >> from using these hypercalls directly. > > And this is precisely the case where I think "<denied>" is better > than an empty string. +1. The more the behavior would change for tools checking whether the string is valid (i.e != "<denied>"). > >>>> return xsm_default_action(XSM_HOOK, current->domain, NULL); >>>> + >>>> + case XENVER_extraversion: >>>> + case XENVER_compile_info: >>>> + case XENVER_capabilities: >>>> + case XENVER_changeset: >>>> + case XENVER_commandline: >>>> + case XENVER_build_id: >>>> default: >>> >>> There's no need to add all of these next to the default case. >>> Note how commandline and build_id have been coming here already >>> (and there would need to be further justification for exposing >>> them on debug builds, should this questionable behavior - see >>> above - be retained). >> >> I find that explicitly listing all the cases is better for code >> readability, but I don't have a strong opinion here. > > Well, I'm viewing it kind of the opposite, as being unnecessary > clutter (and hence harming readability). We'll see what others > think. I am on Sergey side here, with a "default" label you have to check what falls under it. So explicit case makes easier to find out how each hypercalls are dealt with. Cheers,
On 19/12/2019 11:35, Jan Beulich wrote: >>>> XENVER_changeset >>>> XENVER_commandline >>>> XENVER_build_id >>>> >>>> Return a more customer friendly empty string instead of "<denied>" >>>> which would be shown in tools like dmidecode.> >>> I think "<denied>" is quite fine for many of the original purposes. >>> Maybe it would be better to filter for this when populating guest >>> DMI tables? >> I don't know how DMI tables are populated, but nothing stops a guest >> from using these hypercalls directly. > And this is precisely the case where I think "<denied>" is better > than an empty string. "<denied>" was a terrible choice back when it was introduced, and its still a terrible choice today. These are ASCII string fields, and the empty string is a perfectly good string. Nothing is going to break, because it would have broken the first time around. The end result without denied sprayed all over this interface is much cleaner overall. ~Andrew
On 19/12/2019 23:15, Andrew Cooper wrote: > On 19/12/2019 11:35, Jan Beulich wrote: >>>>> XENVER_changeset >>>>> XENVER_commandline >>>>> XENVER_build_id >>>>> >>>>> Return a more customer friendly empty string instead of "<denied>" >>>>> which would be shown in tools like dmidecode.> >>>> I think "<denied>" is quite fine for many of the original purposes. >>>> Maybe it would be better to filter for this when populating guest >>>> DMI tables? >>> I don't know how DMI tables are populated, but nothing stops a guest >>> from using these hypercalls directly. >> And this is precisely the case where I think "<denied>" is better >> than an empty string. > "<denied>" was a terrible choice back when it was introduced, and its > still a terrible choice today. > > These are ASCII string fields, and the empty string is a perfectly good > string. Nothing is going to break, because it would have broken the > first time around. Sorry - send just too early. This has shipped in several versions of XenServer already. It is part of a effort to prevent easy fingerprinting of the binary hypervisor - a security measure which was requested specifically by a number of customers. ~Andrew
On 20.12.2019 00:15, Andrew Cooper wrote: > On 19/12/2019 11:35, Jan Beulich wrote: >>>>> XENVER_changeset >>>>> XENVER_commandline >>>>> XENVER_build_id >>>>> >>>>> Return a more customer friendly empty string instead of "<denied>" >>>>> which would be shown in tools like dmidecode.> >>>> I think "<denied>" is quite fine for many of the original purposes. >>>> Maybe it would be better to filter for this when populating guest >>>> DMI tables? >>> I don't know how DMI tables are populated, but nothing stops a guest >>> from using these hypercalls directly. >> And this is precisely the case where I think "<denied>" is better >> than an empty string. > > "<denied>" was a terrible choice back when it was introduced, and its > still a terrible choice today. That's a matter of taste - it's not terrible at all to me. > These are ASCII string fields, and the empty string is a perfectly good > string. Nothing is going to break, because it would have broken the > first time around. In some cases an empty string may have a meaning of "none" or "nothing", which is not the same as "I won't tell you". Jan
On 12/19/19 11:15 PM, Andrew Cooper wrote: > On 19/12/2019 11:35, Jan Beulich wrote: >>>>> XENVER_changeset >>>>> XENVER_commandline >>>>> XENVER_build_id >>>>> >>>>> Return a more customer friendly empty string instead of "<denied>" >>>>> which would be shown in tools like dmidecode.> >>>> I think "<denied>" is quite fine for many of the original purposes. >>>> Maybe it would be better to filter for this when populating guest >>>> DMI tables? >>> I don't know how DMI tables are populated, but nothing stops a guest >>> from using these hypercalls directly. >> And this is precisely the case where I think "<denied>" is better >> than an empty string. > > "<denied>" was a terrible choice back when it was introduced, and its > still a terrible choice today. > > These are ASCII string fields, and the empty string is a perfectly good > string. Nothing is going to break, because it would have broken the > first time around. > > The end result without denied sprayed all over this interface is much > cleaner overall. Unfortunately this mail doesn't contain any facts or arguments, just unsubstantiated value judgements. What's so terrible about "<denied>" -- what bad effect does it have? Why is "" better / cleaner? One negative effect of returning "" is that if you have a tool which doesn't check the value but just dumps it into a log somewhere, then the log just contains nothing at all. A log which contains "<denied>" makes it clear to the person reading it that something has been hidden on purpose. You can totally imagine someone wasting several hours trying to figure out why their logging isn't working, only to discover that it is working, but that it was just logging an empty string. And is it so bad for dmidecode to return something like "<denied>" in that case? -George
On 06/01/2020 11:28, George Dunlap wrote: > On 12/19/19 11:15 PM, Andrew Cooper wrote: >> On 19/12/2019 11:35, Jan Beulich wrote: >>>>>> XENVER_changeset >>>>>> XENVER_commandline >>>>>> XENVER_build_id >>>>>> >>>>>> Return a more customer friendly empty string instead of "<denied>" >>>>>> which would be shown in tools like dmidecode.> >>>>> I think "<denied>" is quite fine for many of the original purposes. >>>>> Maybe it would be better to filter for this when populating guest >>>>> DMI tables? >>>> I don't know how DMI tables are populated, but nothing stops a guest >>>> from using these hypercalls directly. >>> And this is precisely the case where I think "<denied>" is better >>> than an empty string. >> >> "<denied>" was a terrible choice back when it was introduced, and its >> still a terrible choice today. >> >> These are ASCII string fields, and the empty string is a perfectly good >> string. Nothing is going to break, because it would have broken the >> first time around. >> >> The end result without denied sprayed all over this interface is much >> cleaner overall. > > Unfortunately this mail doesn't contain any facts or arguments, just > unsubstantiated value judgements. What's so terrible about "<denied>" > -- what bad effect does it have? Why is "" better / cleaner? It can be explained with a picture (attached) ;) > > One negative effect of returning "" is that if you have a tool which > doesn't check the value but just dumps it into a log somewhere, then the > log just contains nothing at all. A log which contains "<denied>" makes > it clear to the person reading it that something has been hidden on > purpose. You can totally imagine someone wasting several hours trying > to figure out why their logging isn't working, only to discover that it > is working, but that it was just logging an empty string. > > And is it so bad for dmidecode to return something like "<denied>" in > that case? > > -George >
On 06.01.2020 15:35, Sergey Dyasli wrote: > On 06/01/2020 11:28, George Dunlap wrote: >> On 12/19/19 11:15 PM, Andrew Cooper wrote: >>> On 19/12/2019 11:35, Jan Beulich wrote: >>>>>>> XENVER_changeset >>>>>>> XENVER_commandline >>>>>>> XENVER_build_id >>>>>>> >>>>>>> Return a more customer friendly empty string instead of "<denied>" >>>>>>> which would be shown in tools like dmidecode.> >>>>>> I think "<denied>" is quite fine for many of the original purposes. >>>>>> Maybe it would be better to filter for this when populating guest >>>>>> DMI tables? >>>>> I don't know how DMI tables are populated, but nothing stops a guest >>>>> from using these hypercalls directly. >>>> And this is precisely the case where I think "<denied>" is better >>>> than an empty string. >>> >>> "<denied>" was a terrible choice back when it was introduced, and its >>> still a terrible choice today. >>> >>> These are ASCII string fields, and the empty string is a perfectly good >>> string. Nothing is going to break, because it would have broken the >>> first time around. >>> >>> The end result without denied sprayed all over this interface is much >>> cleaner overall. >> >> Unfortunately this mail doesn't contain any facts or arguments, just >> unsubstantiated value judgements. What's so terrible about "<denied>" >> -- what bad effect does it have? Why is "" better / cleaner? > > It can be explained with a picture (attached) ;) But that's something better addressed at or close to the presentation layer, not deep down in Xen. Jan
On 06/01/2020 14:40, Jan Beulich wrote: > On 06.01.2020 15:35, Sergey Dyasli wrote: >> On 06/01/2020 11:28, George Dunlap wrote: >>> On 12/19/19 11:15 PM, Andrew Cooper wrote: >>>> On 19/12/2019 11:35, Jan Beulich wrote: >>>>>>>> XENVER_changeset >>>>>>>> XENVER_commandline >>>>>>>> XENVER_build_id >>>>>>>> >>>>>>>> Return a more customer friendly empty string instead of "<denied>" >>>>>>>> which would be shown in tools like dmidecode.> >>>>>>> I think "<denied>" is quite fine for many of the original purposes. >>>>>>> Maybe it would be better to filter for this when populating guest >>>>>>> DMI tables? >>>>>> I don't know how DMI tables are populated, but nothing stops a guest >>>>>> from using these hypercalls directly. >>>>> And this is precisely the case where I think "<denied>" is better >>>>> than an empty string. >>>> >>>> "<denied>" was a terrible choice back when it was introduced, and its >>>> still a terrible choice today. >>>> >>>> These are ASCII string fields, and the empty string is a perfectly good >>>> string. Nothing is going to break, because it would have broken the >>>> first time around. >>>> >>>> The end result without denied sprayed all over this interface is much >>>> cleaner overall. >>> >>> Unfortunately this mail doesn't contain any facts or arguments, just >>> unsubstantiated value judgements. What's so terrible about "<denied>" >>> -- what bad effect does it have? Why is "" better / cleaner? >> >> It can be explained with a picture (attached) ;) > > But that's something better addressed at or close to the presentation > layer, not deep down in Xen. I agree with that. And looks like the following diff does the trick: diff --git a/tools/firmware/hvmloader/smbios.c b/tools/firmware/hvmloader/smbios.c index 97a054e9e3..b4d72c375f 100644 --- a/tools/firmware/hvmloader/smbios.c +++ b/tools/firmware/hvmloader/smbios.c @@ -275,6 +275,8 @@ hvm_write_smbios_tables( xen_minor_version = (uint16_t) xen_version; hypercall_xen_version(XENVER_extraversion, xen_extra_version); + if ( strcmp(xen_extra_version, "<denied>") == 0 ) + memset(xen_extra_version, 0, sizeof(xen_extra_version)); /* build up human-readable Xen version string */ p = xen_version_str; -- Thanks, Sergey
On 07.01.2020 12:02, Sergey Dyasli wrote: > On 06/01/2020 14:40, Jan Beulich wrote: >> On 06.01.2020 15:35, Sergey Dyasli wrote: >>> On 06/01/2020 11:28, George Dunlap wrote: >>>> On 12/19/19 11:15 PM, Andrew Cooper wrote: >>>>> On 19/12/2019 11:35, Jan Beulich wrote: >>>>>>>>> XENVER_changeset >>>>>>>>> XENVER_commandline >>>>>>>>> XENVER_build_id >>>>>>>>> >>>>>>>>> Return a more customer friendly empty string instead of "<denied>" >>>>>>>>> which would be shown in tools like dmidecode.> >>>>>>>> I think "<denied>" is quite fine for many of the original purposes. >>>>>>>> Maybe it would be better to filter for this when populating guest >>>>>>>> DMI tables? >>>>>>> I don't know how DMI tables are populated, but nothing stops a guest >>>>>>> from using these hypercalls directly. >>>>>> And this is precisely the case where I think "<denied>" is better >>>>>> than an empty string. >>>>> >>>>> "<denied>" was a terrible choice back when it was introduced, and its >>>>> still a terrible choice today. >>>>> >>>>> These are ASCII string fields, and the empty string is a perfectly good >>>>> string. Nothing is going to break, because it would have broken the >>>>> first time around. >>>>> >>>>> The end result without denied sprayed all over this interface is much >>>>> cleaner overall. >>>> >>>> Unfortunately this mail doesn't contain any facts or arguments, just >>>> unsubstantiated value judgements. What's so terrible about "<denied>" >>>> -- what bad effect does it have? Why is "" better / cleaner? >>> >>> It can be explained with a picture (attached) ;) >> >> But that's something better addressed at or close to the presentation >> layer, not deep down in Xen. > > I agree with that. And looks like the following diff does the trick: > > diff --git a/tools/firmware/hvmloader/smbios.c b/tools/firmware/hvmloader/smbios.c > index 97a054e9e3..b4d72c375f 100644 > --- a/tools/firmware/hvmloader/smbios.c > +++ b/tools/firmware/hvmloader/smbios.c > @@ -275,6 +275,8 @@ hvm_write_smbios_tables( > xen_minor_version = (uint16_t) xen_version; > > hypercall_xen_version(XENVER_extraversion, xen_extra_version); > + if ( strcmp(xen_extra_version, "<denied>") == 0 ) > + memset(xen_extra_version, 0, sizeof(xen_extra_version)); > > /* build up human-readable Xen version string */ > p = xen_version_str; When you submit this as a proper patch, feel free to add my ack right away (as long you give it a non-empty and half way useful description). Jan
diff --git a/xen/common/version.c b/xen/common/version.c index 937eb1281c..cc621ab76a 100644 --- a/xen/common/version.c +++ b/xen/common/version.c @@ -67,7 +67,7 @@ const char *xen_banner(void) const char *xen_deny(void) { - return "<denied>"; + return ""; } static const void *build_id_p __read_mostly; diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index b8e185e6fa..4a1a1bf2bd 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -750,16 +750,21 @@ static XSM_INLINE int xsm_xen_version (XSM_DEFAULT_ARG uint32_t op) case XENVER_get_features: /* These sub-ops ignore the permission checks and return data. */ return 0; - case XENVER_extraversion: - case XENVER_compile_info: - case XENVER_capabilities: - case XENVER_changeset: case XENVER_pagesize: case XENVER_guest_handle: /* These MUST always be accessible to any guest by default. */ return xsm_default_action(XSM_HOOK, current->domain, NULL); + + case XENVER_extraversion: + case XENVER_compile_info: + case XENVER_capabilities: + case XENVER_changeset: + case XENVER_commandline: + case XENVER_build_id: default: - return xsm_default_action(XSM_PRIV, current->domain, NULL); + /* Hide information from guests only in Release builds. */ + return xsm_default_action(debug_build() ? XSM_HOOK : XSM_PRIV, + current->domain, NULL); } }
Hide the following information that can help identify the running Xen binary version: XENVER_extraversion XENVER_compile_info XENVER_capabilities XENVER_changeset XENVER_commandline XENVER_build_id Return a more customer friendly empty string instead of "<denied>" which would be shown in tools like dmidecode. But allow guests to see this information in Debug builds of Xen. Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> --- CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: George Dunlap <George.Dunlap@eu.citrix.com> CC: Ian Jackson <ian.jackson@eu.citrix.com> CC: Jan Beulich <jbeulich@suse.com> CC: Julien Grall <julien@xen.org> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Wei Liu <wl@xen.org> CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> --- xen/common/version.c | 2 +- xen/include/xsm/dummy.h | 15 ++++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-)