| Message ID | 20200110103723.29538-1-sergey.dyasli@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] xsm: hide detailed Xen version from unprivileged guests | expand |
On 10/01/2020 10:37, Sergey Dyasli wrote: > Hide the following information that can help identify the running Xen > binary version: XENVER_extraversion, XENVER_compile_info, XENVER_changeset. > Add explicit cases for XENVER_commandline and XENVER_build_id as well. > > Introduce xsm_filter_denied() to hvmloader to remove "<denied>" string > from guest's DMI tables that otherwise would be shown in tools like > dmidecode. > > Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> > --- > v1 --> v2: > - Added xsm_filter_denied() to hvmloader instead of modifying xen_deny() > - Made behaviour the same for both Release and Debug builds > - XENVER_capabilities is no longer hided > > 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> I realise there are arguments over how to fix this, but we (the Xen community) have already f*cked up once here, and this is doing so a second time. Nack. Fixing it anywhere other than Xen is simply not appropriate. The reason for this (which ought to be obvious, but I guess only to those who actually do customer support) is basic human physiology. "denied" means something has gone wrong. It scares people, and causes them to seek help to change fix whatever is broken. It is not appropriate for it to find its way into the guest in the first place, and that includes turning up in `dmesg` and other logs, and expecting guest runtime to filter for it is complete nonsense. As said several times before, the empty string is completely fine ABI wise, doesn't confuse customers, and really really does work in practice. ~Andrew
On 10.01.2020 11:37, Sergey Dyasli wrote: > --- a/tools/firmware/hvmloader/util.c > +++ b/tools/firmware/hvmloader/util.c > @@ -995,6 +995,12 @@ void hvmloader_acpi_build_tables(struct acpi_config *config, > hvm_param_set(HVM_PARAM_VM_GENERATION_ID_ADDR, config->vm_gid_addr); > } > > +void xsm_filter_denied(char *str, size_t len) > +{ > + if ( strcmp(str, "<denied>") == 0 ) > + memset(str, 0, len); > +} I think you can get away without passing in "len": void xsm_filter_denied(char *str, size_t len) { if ( strcmp(str, "<denied>") == 0 ) *str = 0; } Any reason you think you need to zap the entire buffer? > --- a/xen/include/xsm/dummy.h > +++ b/xen/include/xsm/dummy.h > @@ -750,14 +750,17 @@ 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: Could you take the opportunity and also add the missing blank line here, just like you do below? > - 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_changeset: > + case XENVER_commandline: > + case XENVER_build_id: > default: > return xsm_default_action(XSM_PRIV, current->domain, NULL); I continue to not see the need to have "default:" accompanied by various specific case labels. I don't think we do so elsewhere. And if you do, "default:" should gain ASSERT_UNREACHABLE() imo. I also remain unconvinced of the very brief reasoning - as indicated before, it would seem at least desirable to me to discuss why the previous decision was wrong (iirc the implication back then was that anyone wanting to tighten this would be supposed to use a respective XSM policy). In any event - if the hvmloader change was submitted as a separate patch, I'd ack it with the change suggested (or a suitable verbal clarification in reply to my remark above). Jan
On 1/10/20 11:02 AM, Andrew Cooper wrote: > On 10/01/2020 10:37, Sergey Dyasli wrote: >> Hide the following information that can help identify the running Xen >> binary version: XENVER_extraversion, XENVER_compile_info, XENVER_changeset. >> Add explicit cases for XENVER_commandline and XENVER_build_id as well. >> >> Introduce xsm_filter_denied() to hvmloader to remove "<denied>" string >> from guest's DMI tables that otherwise would be shown in tools like >> dmidecode. >> >> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> >> --- >> v1 --> v2: >> - Added xsm_filter_denied() to hvmloader instead of modifying xen_deny() >> - Made behaviour the same for both Release and Debug builds >> - XENVER_capabilities is no longer hided >> >> 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> > > I realise there are arguments over how to fix this, but we (the Xen > community) have already f*cked up once here, and this is doing so a > second time. > > Nack. > > Fixing it anywhere other than Xen is simply not appropriate. > > The reason for this (which ought to be obvious, but I guess only to > those who actually do customer support) is basic human physiology. > "denied" means something has gone wrong. It scares people, and causes > them to seek help to change fix whatever is broken. This seems like a reasonable argument that "<denied>" causes issues. But that doesn't change the fact that "" also causes issues. What about changing the string to "<build-id hidden>" or something like that? That makes it more clear what would have been in that place, and "hidden" is a lot less scary than "denied". -George
On 10.01.2020 16:28, George Dunlap wrote: > On 1/10/20 11:02 AM, Andrew Cooper wrote: >> On 10/01/2020 10:37, Sergey Dyasli wrote: >>> Hide the following information that can help identify the running Xen >>> binary version: XENVER_extraversion, XENVER_compile_info, XENVER_changeset. >>> Add explicit cases for XENVER_commandline and XENVER_build_id as well. >>> >>> Introduce xsm_filter_denied() to hvmloader to remove "<denied>" string >>> from guest's DMI tables that otherwise would be shown in tools like >>> dmidecode. >>> >>> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> >>> --- >>> v1 --> v2: >>> - Added xsm_filter_denied() to hvmloader instead of modifying xen_deny() >>> - Made behaviour the same for both Release and Debug builds >>> - XENVER_capabilities is no longer hided >>> >>> 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> >> >> I realise there are arguments over how to fix this, but we (the Xen >> community) have already f*cked up once here, and this is doing so a >> second time. >> >> Nack. >> >> Fixing it anywhere other than Xen is simply not appropriate. (replying here, because the original mail doesn't seem to have made it into my inbox) I've said so to Sergey already on v1: The "fix" you want needs to be at or closer to the presentation layer. From Xen's perspective the request for information was _indeed_ denied. >> The reason for this (which ought to be obvious, but I guess only to >> those who actually do customer support) is basic human physiology. >> "denied" means something has gone wrong. It scares people, and causes >> them to seek help to change fix whatever is broken. > > This seems like a reasonable argument that "<denied>" causes issues. > But that doesn't change the fact that "" also causes issues. > > What about changing the string to "<build-id hidden>" or something like > that? That makes it more clear what would have been in that place, and > "hidden" is a lot less scary than "denied". I could live with this. But (judging from the picture that was provided earlier on) it would still require filtering at or close to the presentation layer, and by changing the prior <denied> to different and varying strings may make that job harder (albeit perhaps they could look for any <...>). Jan
On 10.01.20 16:56, Jan Beulich wrote: > On 10.01.2020 16:28, George Dunlap wrote: >> On 1/10/20 11:02 AM, Andrew Cooper wrote: >>> On 10/01/2020 10:37, Sergey Dyasli wrote: >>>> Hide the following information that can help identify the running Xen >>>> binary version: XENVER_extraversion, XENVER_compile_info, XENVER_changeset. >>>> Add explicit cases for XENVER_commandline and XENVER_build_id as well. >>>> >>>> Introduce xsm_filter_denied() to hvmloader to remove "<denied>" string >>>> from guest's DMI tables that otherwise would be shown in tools like >>>> dmidecode. >>>> >>>> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> >>>> --- >>>> v1 --> v2: >>>> - Added xsm_filter_denied() to hvmloader instead of modifying xen_deny() >>>> - Made behaviour the same for both Release and Debug builds >>>> - XENVER_capabilities is no longer hided >>>> >>>> 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> >>> >>> I realise there are arguments over how to fix this, but we (the Xen >>> community) have already f*cked up once here, and this is doing so a >>> second time. >>> >>> Nack. >>> >>> Fixing it anywhere other than Xen is simply not appropriate. > > (replying here, because the original mail doesn't seem to have > made it into my inbox) > > I've said so to Sergey already on v1: The "fix" you want needs to > be at or closer to the presentation layer. From Xen's perspective > the request for information was _indeed_ denied. > >>> The reason for this (which ought to be obvious, but I guess only to >>> those who actually do customer support) is basic human physiology. >>> "denied" means something has gone wrong. It scares people, and causes >>> them to seek help to change fix whatever is broken. >> >> This seems like a reasonable argument that "<denied>" causes issues. >> But that doesn't change the fact that "" also causes issues. >> >> What about changing the string to "<build-id hidden>" or something like >> that? That makes it more clear what would have been in that place, and >> "hidden" is a lot less scary than "denied". > > I could live with this. But (judging from the picture that was > provided earlier on) it would still require filtering at or close > to the presentation layer, and by changing the prior <denied> to > different and varying strings may make that job harder (albeit > perhaps they could look for any <...>). I'd go with "<hidden>" or "<NIL>". "<build-id hidden>" as value for the build-id is similar to "LCD-display". And it will certainly not be the correct value for other hidden items. :-) Juergen
On 1/10/20 4:45 PM, Jürgen Groß wrote: > On 10.01.20 16:56, Jan Beulich wrote: >> On 10.01.2020 16:28, George Dunlap wrote: >>> On 1/10/20 11:02 AM, Andrew Cooper wrote: >>>> On 10/01/2020 10:37, Sergey Dyasli wrote: >>>>> Hide the following information that can help identify the running Xen >>>>> binary version: XENVER_extraversion, XENVER_compile_info, >>>>> XENVER_changeset. >>>>> Add explicit cases for XENVER_commandline and XENVER_build_id as well. >>>>> >>>>> Introduce xsm_filter_denied() to hvmloader to remove "<denied>" string >>>>> from guest's DMI tables that otherwise would be shown in tools like >>>>> dmidecode. >>>>> >>>>> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> >>>>> --- >>>>> v1 --> v2: >>>>> - Added xsm_filter_denied() to hvmloader instead of modifying >>>>> xen_deny() >>>>> - Made behaviour the same for both Release and Debug builds >>>>> - XENVER_capabilities is no longer hided >>>>> >>>>> 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> >>>> >>>> I realise there are arguments over how to fix this, but we (the Xen >>>> community) have already f*cked up once here, and this is doing so a >>>> second time. >>>> >>>> Nack. >>>> >>>> Fixing it anywhere other than Xen is simply not appropriate. >> >> (replying here, because the original mail doesn't seem to have >> made it into my inbox) >> >> I've said so to Sergey already on v1: The "fix" you want needs to >> be at or closer to the presentation layer. From Xen's perspective >> the request for information was _indeed_ denied. >> >>>> The reason for this (which ought to be obvious, but I guess only to >>>> those who actually do customer support) is basic human physiology. >>>> "denied" means something has gone wrong. It scares people, and causes >>>> them to seek help to change fix whatever is broken. >>> >>> This seems like a reasonable argument that "<denied>" causes issues. >>> But that doesn't change the fact that "" also causes issues. >>> >>> What about changing the string to "<build-id hidden>" or something like >>> that? That makes it more clear what would have been in that place, and >>> "hidden" is a lot less scary than "denied". >> >> I could live with this. But (judging from the picture that was >> provided earlier on) it would still require filtering at or close >> to the presentation layer, and by changing the prior <denied> to >> different and varying strings may make that job harder (albeit >> perhaps they could look for any <...>). > > I'd go with "<hidden>" or "<NIL>". "<build-id hidden>" as value for the > build-id is similar to "LCD-display". And it will certainly not be the > correct value for other hidden items. :-) The idea is that in a log, if you see a buildid in context, you can usually figure out what it is just by looking at it; i.e..: Xen 4.13.1 8a2a24f4e1 So which is better? 1. Xen 4.13.1 2. Xen 4.13.1 <hidden> 3 Xen 4.13.1 <buildid hidden> 4 Xen 4.13.1 <NIL> I don't like 1 or 4. I could live with 2 I guess. -George
On 1/10/20 9:28 AM, George Dunlap wrote: > On 1/10/20 11:02 AM, Andrew Cooper wrote: >> On 10/01/2020 10:37, Sergey Dyasli wrote: >>> Hide the following information that can help identify the running Xen >>> binary version: XENVER_extraversion, XENVER_compile_info, XENVER_changeset. >>> Add explicit cases for XENVER_commandline and XENVER_build_id as well. >>> >>> Introduce xsm_filter_denied() to hvmloader to remove "<denied>" string >>> from guest's DMI tables that otherwise would be shown in tools like >>> dmidecode. >>> >>> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> >>> --- >>> v1 --> v2: >>> - Added xsm_filter_denied() to hvmloader instead of modifying xen_deny() >>> - Made behaviour the same for both Release and Debug builds >>> - XENVER_capabilities is no longer hided >>> >>> 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> >> >> I realise there are arguments over how to fix this, but we (the Xen >> community) have already f*cked up once here, and this is doing so a >> second time. >> >> Nack. >> >> Fixing it anywhere other than Xen is simply not appropriate. >> >> The reason for this (which ought to be obvious, but I guess only to >> those who actually do customer support) is basic human physiology. >> "denied" means something has gone wrong. It scares people, and causes >> them to seek help to change fix whatever is broken. > > This seems like a reasonable argument that "<denied>" causes issues. > But that doesn't change the fact that "" also causes issues. > I'd be curious to hear the case where the empty string causes issues. -- Doug
On 1/10/20 4:37 AM, Sergey Dyasli wrote: > Hide the following information that can help identify the running Xen > binary version: XENVER_extraversion, XENVER_compile_info, XENVER_changeset. > Add explicit cases for XENVER_commandline and XENVER_build_id as well. > > Introduce xsm_filter_denied() to hvmloader to remove "<denied>" string > from guest's DMI tables that otherwise would be shown in tools like > dmidecode. > > Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> > --- > v1 --> v2: > - Added xsm_filter_denied() to hvmloader instead of modifying xen_deny() So 100% this version of the patch won't fly with the various downstreams that run the v1 of this patch. Those various consumers will stick with v1. If the goal of this is to reduce the burden of the downstreams and their customers to carry a patch against Xen then I wouldn't even bother with this version. -- Doug
> On Jan 11, 2020, at 4:02 AM, Doug Goldstein <cardoe@cardoe.com> wrote: > > > > On 1/10/20 4:37 AM, Sergey Dyasli wrote: >> Hide the following information that can help identify the running Xen >> binary version: XENVER_extraversion, XENVER_compile_info, XENVER_changeset. >> Add explicit cases for XENVER_commandline and XENVER_build_id as well. >> Introduce xsm_filter_denied() to hvmloader to remove "<denied>" string >> from guest's DMI tables that otherwise would be shown in tools like >> dmidecode. >> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> >> --- >> v1 --> v2: >> - Added xsm_filter_denied() to hvmloader instead of modifying xen_deny() > > So 100% this version of the patch won't fly with the various downstreams that run the v1 of this patch. Those various consumers will stick with v1. > > If the goal of this is to reduce the burden of the downstreams and their customers to carry a patch against Xen then I wouldn't even bother with this version. If the goal is to come up with a solution that works for everyone, it would be helpful if you said *why* “various consumers” would find this patch unacceptable; and also what they might think about the alternate solutions proposed (and why). -George
> On Jan 11, 2020, at 3:55 AM, Doug Goldstein <cardoe@cardoe.com> wrote: > > > > On 1/10/20 9:28 AM, George Dunlap wrote: >> On 1/10/20 11:02 AM, Andrew Cooper wrote: >>> On 10/01/2020 10:37, Sergey Dyasli wrote: >>>> Hide the following information that can help identify the running Xen >>>> binary version: XENVER_extraversion, XENVER_compile_info, XENVER_changeset. >>>> Add explicit cases for XENVER_commandline and XENVER_build_id as well. >>>> >>>> Introduce xsm_filter_denied() to hvmloader to remove "<denied>" string >>>> from guest's DMI tables that otherwise would be shown in tools like >>>> dmidecode. >>>> >>>> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> >>>> --- >>>> v1 --> v2: >>>> - Added xsm_filter_denied() to hvmloader instead of modifying xen_deny() >>>> - Made behaviour the same for both Release and Debug builds >>>> - XENVER_capabilities is no longer hided >>>> >>>> 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> >>> >>> I realise there are arguments over how to fix this, but we (the Xen >>> community) have already f*cked up once here, and this is doing so a >>> second time. >>> >>> Nack. >>> >>> Fixing it anywhere other than Xen is simply not appropriate. >>> >>> The reason for this (which ought to be obvious, but I guess only to >>> those who actually do customer support) is basic human physiology. >>> "denied" means something has gone wrong. It scares people, and causes >>> them to seek help to change fix whatever is broken. >> This seems like a reasonable argument that "<denied>" causes issues. >> But that doesn't change the fact that "" also causes issues. > > I'd be curious to hear the case where the empty string causes issues. This was mentioned in the previous version of this thread. To mirror Andy’s (rather inflammatory) formulation: The reason for this (which ought to be obvious, but I guess only to those who actually [use computer interfaces]) is basic [user interface design]. When you ask an interface to do something for you, it should either do the thing you asked it to do, or it should tell you why it couldn’t. Or, at *very* least, it should tell you *that* it didn’t. Imagine someone who writes code to get the buildid and store it in a log somewhere. Everything compiles and runs great. And then a little ways down the road, they look in their log, and find “Xen 4.13.1”, with no buildid! So they spend an afternoon trying to figure out what went wrong. Did they screw up the build of Xen? No, `xl info` reports the bulidid. Did they grab they compile against the wrong header? Did they screw up the call somehow? Did they accidentally read it into the wrong buffer? Imagine their rage when eventually, after hours of tracing through code they’ve seen a dozen times, they figure out that THE INTERFACE IS SILENTLY FAILING. Or imagine someone who sells something that runs inside of Xen guests and uses Xen-specific features. All of her test systems have versions of Xen which expose the buildid to guests. At some point one of her customers has an issue, so she asks them to get the buildid by clicking here here and here, and getting the string. But when the customer reports the string, there’s no buildid, nothing! So again she spends hours trying to “debug” the “why do I not have a buildid” issue, looking like an idiot in front of her customer, only to discover that their customer is running a version of Xen where the buildid is simply empty, with no indication that anything has been hidden. Silently returning a result which is indistinguishable from “no buildid exists” is just plain rude to any programmer who uses this interface. I completely understand that having “<denied>” show up in a GUI might cause customer support headaches. But fundamentally it’s not the hypervisor’s job to coddle timid end-users; that’s what UIs are for. I don’t want to be dogmatic about that principle, so I’m happy to entertain changing the string to be something more user-friendly (or some other suitable change in the interface). But I do want to be dogmatic about interfaces not silently failing. -George
On 1/11/20 3:02 AM, George Dunlap wrote: > > >> On Jan 11, 2020, at 4:02 AM, Doug Goldstein <cardoe@cardoe.com> wrote: >> >> >> >> On 1/10/20 4:37 AM, Sergey Dyasli wrote: >>> Hide the following information that can help identify the running Xen >>> binary version: XENVER_extraversion, XENVER_compile_info, XENVER_changeset. >>> Add explicit cases for XENVER_commandline and XENVER_build_id as well. >>> Introduce xsm_filter_denied() to hvmloader to remove "<denied>" string >>> from guest's DMI tables that otherwise would be shown in tools like >>> dmidecode. >>> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> >>> --- >>> v1 --> v2: >>> - Added xsm_filter_denied() to hvmloader instead of modifying xen_deny() >> >> So 100% this version of the patch won't fly with the various downstreams that run the v1 of this patch. Those various consumers will stick with v1. >> >> If the goal of this is to reduce the burden of the downstreams and their customers to carry a patch against Xen then I wouldn't even bother with this version. > > If the goal is to come up with a solution that works for everyone, it would be helpful if you said *why* “various consumers” would find this patch unacceptable; and also what they might think about the alternate solutions proposed (and why). > > -George > I'd be happy if we had a Kconfig option behind what the string is. Give me a blank as an option but default it to whatever string like "<hidden>" that you'd like. Every shipping Xen distro I've worked on has had its own v1 variant of the patch and none of them authored by me. Xen is a bit unique in the software world as most pieces of software aren't run in an "adversarial" environment. Look at any multi-tenant cloud provider. There's continually bad actors that are creating VMs and probing your system configurations and attempting to build a fingerprinting technique to identify exploitable systems vs not exploitable systems. Many security issues are dropped on providers without adequate time to patch all the systems prior to a disclosure. Look at systems like OpenXT, SecureView and Qubes where the users of these systems don't necessarily update to the latest fix immediately. Now I know someone is going to read this and say "Look at Doug and him advocating for security through obscurity". But that's simply not the case. The point is anything that can be used to fingerprint a system easily and target an attack against that system is very different from saying my interfaces are secure because I don't publish the spec. When attackers are forced to probe a system it results in an opportunity to identity that behavior and take action. I'll just end saying that stripping information in dom0 from the domU has not been considered acceptable in various circles because it changes the stance from "It is not possible to leak this data" to "This data cannot leak if action X happens correctly". Which then requires tests and documentation to show that it is not possible to leak. Ultimately my point is if the goal of this patch is to upstream a patch that's carried by various downstreams, why not actually listen to what caused them to write the patch? In your other email you talk about developers being concerned about tracing the build of Xen or if they built it wrong. In the cases I'm talking about there's literally 0 concern for that. The build of Xen is captured very well as an artifact of the deployment and certification of that build. The developers of that build of Xen know exactly the revision that the specific system is using and when they receive information they can go right to that revision. -- Doug
On 10/01/2020 11:02, Andrew Cooper wrote: > On 10/01/2020 10:37, Sergey Dyasli wrote: >> Hide the following information that can help identify the running Xen >> binary version: XENVER_extraversion, XENVER_compile_info, XENVER_changeset. >> Add explicit cases for XENVER_commandline and XENVER_build_id as well. >> >> Introduce xsm_filter_denied() to hvmloader to remove "<denied>" string >> from guest's DMI tables that otherwise would be shown in tools like >> dmidecode. >> >> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> >> --- >> v1 --> v2: >> - Added xsm_filter_denied() to hvmloader instead of modifying xen_deny() >> - Made behaviour the same for both Release and Debug builds >> - XENVER_capabilities is no longer hided >> >> 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> > > I realise there are arguments over how to fix this, but we (the Xen > community) have already f*cked up once here, and this is doing so a > second time. > > Nack. > > Fixing it anywhere other than Xen is simply not appropriate. > > The reason for this (which ought to be obvious, but I guess only to > those who actually do customer support) is basic human physiology. > "denied" means something has gone wrong. It scares people, and causes > them to seek help to change fix whatever is broken. But the patch takes care of that by removing "denied" from DMI tables. Functionally it should have the same effect as v1 to ordinary guests. > It is not appropriate for it to find its way into the guest in the first > place, and that includes turning up in `dmesg` and other logs, and > expecting guest runtime to filter for it is complete nonsense. `dmesg` will have only Xen major version (e.g. Xen 4.13) with this patch applied. Even if there exists a tool which uses xen_version hypercall for information gathering, it would show you "<denied>" for fields like commandline and build_id already (without the patch). So extending this behaviour for other sensitive fields is not a regression IMHO. > As said several times before, the empty string is completely fine ABI > wise, doesn't confuse customers, and really really does work in practice. I agree with the other opinion that returning an empty string is too ambiguous. I'd prefer to retain the current behaviour with (whatever) non-empty descriptive string. -- Thanks, Sergey
On 1/12/20 6:26 PM, Doug Goldstein wrote: > On 1/11/20 3:02 AM, George Dunlap wrote: >> >> >>> On Jan 11, 2020, at 4:02 AM, Doug Goldstein <cardoe@cardoe.com> wrote: >>> >>> >>> >>> On 1/10/20 4:37 AM, Sergey Dyasli wrote: >>>> Hide the following information that can help identify the running Xen >>>> binary version: XENVER_extraversion, XENVER_compile_info, >>>> XENVER_changeset. >>>> Add explicit cases for XENVER_commandline and XENVER_build_id as well. >>>> Introduce xsm_filter_denied() to hvmloader to remove "<denied>" string >>>> from guest's DMI tables that otherwise would be shown in tools like >>>> dmidecode. >>>> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> >>>> --- >>>> v1 --> v2: >>>> - Added xsm_filter_denied() to hvmloader instead of modifying >>>> xen_deny() >>> >>> So 100% this version of the patch won't fly with the various >>> downstreams that run the v1 of this patch. Those various consumers >>> will stick with v1. >>> >>> If the goal of this is to reduce the burden of the downstreams and >>> their customers to carry a patch against Xen then I wouldn't even >>> bother with this version. >> >> If the goal is to come up with a solution that works for everyone, it >> would be helpful if you said *why* “various consumers” would find this >> patch unacceptable; and also what they might think about the alternate >> solutions proposed (and why). >> >> -George >> > [snip] > Now I know someone is going to read this and say "Look at Doug and him > advocating for security through obscurity". FWIW I'd be the first person to contradict them, and say you were practicing "defense in depth". :-) > Ultimately my point is if the goal of this patch is to upstream a patch > that's carried by various downstreams, why not actually listen to what > caused them to write the patch? Right, that's what I'm trying to do; but I don't seem to be making much progress. Here's my summary of the situation and arguments so far: 1. The xen_version hypercall can return strings for a number of different values, including XENVER_extraversion, which gives the point release and build id. 2. The XSM dummy module has code to filter which of these are allowed for unprivileged guests. When access to a given value is filtered, no error is returned; rather, the string "<denied>" is returned. 3. Knowledge about the specific instantiation of Xen on which they are running makes it easier for attackers to know how to attack t he system; the XENVER_extraversion provides little value to legitimate users, but a lot of value to attackers. As a defense-in-depth measure, it's important to be able to hide this information. 4. There's currently a patch carried by many downstreams, which changes the XSM dummy module to deny XENVER_extraversion to unprivileged guests. 5. However, this caused "<denied>" to show up in various user-visible places, which caused customer support headaches. So this out-of-tree patch also replaced the string returned when denying access to "" instead. Note that this is not *only* for XENVER_extraversion; with that patch, *any* time the value requested in xen_version is denied by policy, "" will be returned. 6. Silently returning an empty string is considered bad interface design by several developers. So Sergey's second patch: - Still denies XENVER_extraversion at the hypervisor level - Leaves the value returned by the hypervisor as "<denied>" - Filters the "<denied>" string at the hvmloader level, to prevent it leaking into a GUI and scaring customers. Now we get to Andy's objection on the 10th: --- The reason for this (which ought to be obvious, but I guess only to those who actually do customer support) is basic human physiology. "denied" means something has gone wrong. It scares people, and causes them to seek help to change fix whatever is broken. It is not appropriate for it to find its way into the guest in the first place, and that includes turning up in `dmesg` and other logs, and expecting guest runtime to filter for it is complete nonsense. --- Basically, Andy says that *anywhere* it might show up is way too scary, even a guest dmesg log. Well, I disagree; I look in "dmesg" and I see loads of "scary" things. But if "<denied>" is too scary, then we can try "<hidden>". Then we come to your mail. You spend two paragraphs justifying why we need to do #4 (hide the value from unprivileged guests), basically reiterating point #3 and dealing with potential objections. But nobody objects to #4, or disagrees with #3. You then have a paragraph arguing why it's important that information be stripped at the hypervisor rather than in the toolstack. But Sergey's v2 patch *does* strip the information at the hypervisor. His patch makes it so that XENVER_extraversion returns "<denied>". The code which converts "<denied>" to "" in hvmloader is purely a UI thing, so that people looking in their Windows System Info don't get scary messages. > I'd be happy if we had a Kconfig option behind what the string is. Give > me a blank as an option but default it to whatever string like > "<hidden>" that you'd like. Every shipping Xen distro I've worked on has > had its own v1 variant of the patch and none of them authored by me. OK, so with this we have four proposed options: 1. Block XENVER_extraversion at the hypervisor level. Change the xen_deny() string to "". (This is v1 of sergey's patch.) 2. Block XENVER_extraversion at the hypervisor level. Leave xen_deny() as returning "<denied>", but replace "<denied>" with "" in hvmloader so it doesn't show up in the System Info and scare users. 3. Block XENVER_extraversion at the hypervisor level. Change xen_deny() to return a more benign string like "<hidden>". (Perhaps also filter it in hvmloader, just for good measure.) 4. Block XENVER_extraversion at the hypervisor level. Make the xen_deny() string configurable in KConfig. Fundamentally I have no objection to #4. But I still don't know what your objections are to #2 and #3. -George
Hi George, Thank you for summarising the possibility. One question below. On 13/01/2020 12:51, George Dunlap wrote: > 2. Block XENVER_extraversion at the hypervisor level. Leave xen_deny() > as returning "<denied>", but replace "<denied>" with "" in hvmloader so > it doesn't show up in the System Info and scare users. > > 3. Block XENVER_extraversion at the hypervisor level. Change xen_deny() > to return a more benign string like "<hidden>". (Perhaps also filter it > in hvmloader, just for good measure.) My knowledge of live migration on x86 is a bit limited, but if I understand correctly those two options would require a guest to reboot in order to pick up the changes. Am I correct? Cheers,
On 13/01/2020 13:39, Julien Grall wrote: > Hi George, > > Thank you for summarising the possibility. One question below. > > On 13/01/2020 12:51, George Dunlap wrote: >> 2. Block XENVER_extraversion at the hypervisor level. Leave xen_deny() >> as returning "<denied>", but replace "<denied>" with "" in hvmloader so >> it doesn't show up in the System Info and scare users. >> >> 3. Block XENVER_extraversion at the hypervisor level. Change xen_deny() >> to return a more benign string like "<hidden>". (Perhaps also filter it >> in hvmloader, just for good measure.) > > My knowledge of live migration on x86 is a bit limited, but if I > understand correctly those two options would require a guest to reboot > in order to pick up the changes. Am I correct? Not in the slightest. The content returned changes whenever the hypervisor changes. ~Andrew
Doug Goldstein writes ("Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests"):
> I'd be happy if we had a Kconfig option behind what the string is. Give
> me a blank as an option but default it to whatever string like
> "<hidden>" that you'd like. Every shipping Xen distro I've worked on has
> had its own v1 variant of the patch and none of them authored by me.
Firstly: I generally agree with George's comments about not liking
silent failure, and I disagree with Andrew. My inclination would be
to have this string be "<hidden>" (or similar) - and to not filter it
in the DMI tables, so it would be "<hidden>" everywhere.
Doug, I can't figure out from your messages whether that would meet
your needs ? Is George right that the reason for filtering what used
to be "<denied>" is simply to reduce support burden due to the
negative connotations of "denied" ? Would "hidden" fix that ?
If I am wrong, what is the reason for the filtering ?
If we have to have this string be configurable then so be it but I
would rather see if we can find one thing that meets downstreams'
needs.
FTR I agree with many of the points you make and I understand why you
think this is frustrating. I hope we can clear this up.
Ian.
On 1/13/20 2:01 PM, Andrew Cooper wrote: > On 13/01/2020 13:39, Julien Grall wrote: >> Hi George, >> >> Thank you for summarising the possibility. One question below. >> >> On 13/01/2020 12:51, George Dunlap wrote: >>> 2. Block XENVER_extraversion at the hypervisor level. Leave xen_deny() >>> as returning "<denied>", but replace "<denied>" with "" in hvmloader so >>> it doesn't show up in the System Info and scare users. >>> >>> 3. Block XENVER_extraversion at the hypervisor level. Change xen_deny() >>> to return a more benign string like "<hidden>". (Perhaps also filter it >>> in hvmloader, just for good measure.) >> >> My knowledge of live migration on x86 is a bit limited, but if I >> understand correctly those two options would require a guest to reboot >> in order to pick up the changes. Am I correct? > > Not in the slightest. The content returned changes whenever the > hypervisor changes. I guess Julien is talking about the filtering done in hvmloader. That filtering is about what's in the guest's ACPI tables; and *that* happens only once at guest boot; so whatever the scary message is in the Windows System Information page (or wherever it is) would stay there until the guest reboots, regardless of which option we go with. -George
On 13/01/2020 14:07, George Dunlap wrote: > On 1/13/20 2:01 PM, Andrew Cooper wrote: >> On 13/01/2020 13:39, Julien Grall wrote: >>> Hi George, >>> >>> Thank you for summarising the possibility. One question below. >>> >>> On 13/01/2020 12:51, George Dunlap wrote: >>>> 2. Block XENVER_extraversion at the hypervisor level. Leave xen_deny() >>>> as returning "<denied>", but replace "<denied>" with "" in hvmloader so >>>> it doesn't show up in the System Info and scare users. >>>> >>>> 3. Block XENVER_extraversion at the hypervisor level. Change xen_deny() >>>> to return a more benign string like "<hidden>". (Perhaps also filter it >>>> in hvmloader, just for good measure.) >>> >>> My knowledge of live migration on x86 is a bit limited, but if I >>> understand correctly those two options would require a guest to reboot >>> in order to pick up the changes. Am I correct? >> >> Not in the slightest. The content returned changes whenever the >> hypervisor changes. > > I guess Julien is talking about the filtering done in hvmloader. That > filtering is about what's in the guest's ACPI tables; and *that* happens > only once at guest boot; so whatever the scary message is in the Windows > System Information page (or wherever it is) would stay there until the > guest reboots, regardless of which option we go with. Yes, I was speaking about the filtering done in hvmloader. Thank you both for the explanation. Cheers,
On 13/01/2020 12:51, George Dunlap wrote: > On 1/12/20 6:26 PM, Doug Goldstein wrote: >> On 1/11/20 3:02 AM, George Dunlap wrote: >>> >>>> On Jan 11, 2020, at 4:02 AM, Doug Goldstein <cardoe@cardoe.com> wrote: >>>> >>>> >>>> >>>> On 1/10/20 4:37 AM, Sergey Dyasli wrote: >>>>> Hide the following information that can help identify the running Xen >>>>> binary version: XENVER_extraversion, XENVER_compile_info, >>>>> XENVER_changeset. >>>>> Add explicit cases for XENVER_commandline and XENVER_build_id as well. >>>>> Introduce xsm_filter_denied() to hvmloader to remove "<denied>" string >>>>> from guest's DMI tables that otherwise would be shown in tools like >>>>> dmidecode. >>>>> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> >>>>> --- >>>>> v1 --> v2: >>>>> - Added xsm_filter_denied() to hvmloader instead of modifying >>>>> xen_deny() >>>> So 100% this version of the patch won't fly with the various >>>> downstreams that run the v1 of this patch. Those various consumers >>>> will stick with v1. >>>> >>>> If the goal of this is to reduce the burden of the downstreams and >>>> their customers to carry a patch against Xen then I wouldn't even >>>> bother with this version. >>> If the goal is to come up with a solution that works for everyone, it >>> would be helpful if you said *why* “various consumers” would find this >>> patch unacceptable; and also what they might think about the alternate >>> solutions proposed (and why). >>> >>> -George >>> > [snip] > >> Now I know someone is going to read this and say "Look at Doug and him >> advocating for security through obscurity". > FWIW I'd be the first person to contradict them, and say you were > practicing "defense in depth". :-) > >> Ultimately my point is if the goal of this patch is to upstream a patch >> that's carried by various downstreams, why not actually listen to what >> caused them to write the patch? > Right, that's what I'm trying to do; but I don't seem to be making much > progress. > > Here's my summary of the situation and arguments so far: > > 1. The xen_version hypercall can return strings for a number of > different values, including XENVER_extraversion, which gives the point > release and build id. > > 2. The XSM dummy module has code to filter which of these are allowed > for unprivileged guests. When access to a given value is filtered, no > error is returned; rather, the string "<denied>" is returned. Given an ABI which is capable of expressing -EPERM, it is perhaps worth noting that this behaviour is because some drivers will BSOD if the call fails... > 3. Knowledge about the specific instantiation of Xen on which they are > running makes it easier for attackers to know how to attack t he system; > the XENVER_extraversion provides little value to legitimate users, but a > lot of value to attackers. As a defense-in-depth measure, it's > important to be able to hide this information. > > 4. There's currently a patch carried by many downstreams, which changes > the XSM dummy module to deny XENVER_extraversion to unprivileged guests. > > 5. However, this caused "<denied>" to show up in various user-visible > places, which caused customer support headaches. So this out-of-tree > patch also replaced the string returned when denying access to "" > instead. Note that this is not *only* for XENVER_extraversion; with > that patch, *any* time the value requested in xen_version is denied by > policy, "" will be returned. > > 6. Silently returning an empty string is considered bad interface design > by several developers. Several others disagree with this claim. Not least because it is very common signal for "no information". This is why several downstreams really have gotten away with using the empty string, for products which have been in the field for years. > So Sergey's second patch: > - Still denies XENVER_extraversion at the hypervisor level > - Leaves the value returned by the hypervisor as "<denied>" > - Filters the "<denied>" string at the hvmloader level, to prevent it > leaking into a GUI and scaring customers. The SMBios table isn't the only way XENVER_extraversion leaks up into the UI. XENVER_extraversion isn't the only source of redacted information leaking up into the UI. Linux for example exports it all via sysfs. The windows drivers put XENVER_extraversion into several other logs. > > Now we get to Andy's objection on the 10th: > > --- > The reason for this (which ought to be obvious, but I guess only to > those who actually do customer support) is basic human physiology. > "denied" means something has gone wrong. It scares people, and causes > them to seek help to change fix whatever is broken. > > It is not appropriate for it to find its way into the guest in the first > place, and that includes turning up in `dmesg` and other logs, and > expecting guest runtime to filter for it is complete nonsense. > --- > > Basically, Andy says that *anywhere* it might show up is way too scary, > even a guest dmesg log. > > Well, I disagree; I look in "dmesg" and I see loads of "scary" things. Just because dmesg is not an example of a good UI, doesn't mean its ok for us to make: Xen version: 4.14<denied> (preserve-AD) the default thing shown in a Xen system. > But if "<denied>" is too scary, then we can try "<hidden>". From an abstract PoV, hidden is less bad than denied, but: Xen version: 4.14<hidden> (preserve-AD) isn't ok either. > > Then we come to your mail. > > You spend two paragraphs justifying why we need to do #4 (hide the value > from unprivileged guests), basically reiterating point #3 and dealing > with potential objections. But nobody objects to #4, or disagrees with #3. > > You then have a paragraph arguing why it's important that information be > stripped at the hypervisor rather than in the toolstack. > > But Sergey's v2 patch *does* strip the information at the hypervisor. > His patch makes it so that XENVER_extraversion returns "<denied>". The > code which converts "<denied>" to "" in hvmloader is purely a UI thing, > so that people looking in their Windows System Info don't get scary > messages. > >> I'd be happy if we had a Kconfig option behind what the string is. Give >> me a blank as an option but default it to whatever string like >> "<hidden>" that you'd like. Every shipping Xen distro I've worked on has >> had its own v1 variant of the patch and none of them authored by me. > > OK, so with this we have four proposed options: > > 1. Block XENVER_extraversion at the hypervisor level. Change the > xen_deny() string to "". (This is v1 of sergey's patch.) > > 2. Block XENVER_extraversion at the hypervisor level. Leave xen_deny() > as returning "<denied>", but replace "<denied>" with "" in hvmloader so > it doesn't show up in the System Info and scare users. > > 3. Block XENVER_extraversion at the hypervisor level. Change xen_deny() > to return a more benign string like "<hidden>". (Perhaps also filter it > in hvmloader, just for good measure.) > defence in depth argument is quite compelling. > > > >> -George > 4. Block XENVER_extraversion at the hypervisor level. Make the > xen_deny() string configurable in KConfig. > > Fundamentally I have no objection to #4. But I still don't know what > your objections are to #2 and #3. Because they don't fix the other bugs introduced by the patch. I don't think there is any argument over redacting more information than is currently redacted - the security arguments for doing so are entirely fine. The issues are all over how the redaction occurs, and in particular, how redacting new information interacts with the current redaction scheme to cause new problems. It is physically possible to build a hypervisor which has empty strings in all of this information, without even editing the source code. (Getting the compiler string to be empty would be a little tricky, but not not impossible. Everything else is easy.) It is also possible to build a hypervisor which has the literal "<denied>", which would then appear even to privileged domains, in this form. There is no such thing as a silently failure in the interface, because there is no way to distinguish an elided value from a real value, in either its "" or "<denied>" form. What "" gets you that nothing else does is sensible looking logging with no guest changes necessary. ~Andrew
Hi, On 13/01/2020 12:51, George Dunlap wrote: > On 1/12/20 6:26 PM, Doug Goldstein wrote: >> On 1/11/20 3:02 AM, George Dunlap wrote: > 1. Block XENVER_extraversion at the hypervisor level. Change the > xen_deny() string to "". (This is v1 of sergey's patch.) > > 2. Block XENVER_extraversion at the hypervisor level. Leave xen_deny() > as returning "<denied>", but replace "<denied>" with "" in hvmloader so > it doesn't show up in the System Info and scare users. > > 3. Block XENVER_extraversion at the hypervisor level. Change xen_deny() > to return a more benign string like "<hidden>". (Perhaps also filter it > in hvmloader, just for good measure.) > > 4. Block XENVER_extraversion at the hypervisor level. Make the > xen_deny() string configurable in KConfig. A Kconfig option is indeed ideal as some of the stakeholder may want to keep control of the string exposed. But if we go the Kconfig route, then maybe we want to allow each bits (extraversion, compiler...) to be separatly configurable. I would be more than happy to help writing such a patch if there is an interest for it. Cheers,
On 13/01/2020 14:40, Andrew Cooper wrote: > On 13/01/2020 12:51, George Dunlap wrote: >> So Sergey's second patch: >> - Still denies XENVER_extraversion at the hypervisor level >> - Leaves the value returned by the hypervisor as "<denied>" >> - Filters the "<denied>" string at the hvmloader level, to prevent it >> leaking into a GUI and scaring customers. > > The SMBios table isn't the only way XENVER_extraversion leaks up into > the UI. > > XENVER_extraversion isn't the only source of redacted information > leaking up into the UI. > > Linux for example exports it all via sysfs. The windows drivers put > XENVER_extraversion into several other logs. I've found that /sys/hypervisor/version/extra returns "<denied>". "<hidded>" would have looked better there. >> Now we get to Andy's objection on the 10th: >> >> --- >> The reason for this (which ought to be obvious, but I guess only to >> those who actually do customer support) is basic human physiology. >> "denied" means something has gone wrong. It scares people, and causes >> them to seek help to change fix whatever is broken. >> >> It is not appropriate for it to find its way into the guest in the first >> place, and that includes turning up in `dmesg` and other logs, and >> expecting guest runtime to filter for it is complete nonsense. >> --- >> >> Basically, Andy says that *anywhere* it might show up is way too scary, >> even a guest dmesg log. >> >> Well, I disagree; I look in "dmesg" and I see loads of "scary" things. > > Just because dmesg is not an example of a good UI, doesn't mean its ok > for us to make: > > Xen version: 4.14<denied> (preserve-AD) And the above is indeed found in dmesg of PV domains (they have no SMbios). "<hidden>" is not appropriate here indeed. It should be either "" or generic ".0" IMHO. -- Thanks, Sergey
diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c index 598a226278..e760ed5fa6 100644 --- a/tools/firmware/hvmloader/hvmloader.c +++ b/tools/firmware/hvmloader/hvmloader.c @@ -147,6 +147,7 @@ static void init_hypercalls(void) /* Print version information. */ cpuid(base + 1, &eax, &ebx, &ecx, &edx); hypercall_xen_version(XENVER_extraversion, extraversion); + xsm_filter_denied(extraversion, sizeof(extraversion)); printf("Detected Xen v%u.%u%s\n", eax >> 16, eax & 0xffff, extraversion); } diff --git a/tools/firmware/hvmloader/smbios.c b/tools/firmware/hvmloader/smbios.c index 97a054e9e3..1ba352ed2c 100644 --- a/tools/firmware/hvmloader/smbios.c +++ b/tools/firmware/hvmloader/smbios.c @@ -275,6 +275,7 @@ hvm_write_smbios_tables( xen_minor_version = (uint16_t) xen_version; hypercall_xen_version(XENVER_extraversion, xen_extra_version); + xsm_filter_denied(xen_extra_version, sizeof(xen_extra_version)); /* build up human-readable Xen version string */ p = xen_version_str; diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c index 0c3f2d24cd..09e355fa3d 100644 --- a/tools/firmware/hvmloader/util.c +++ b/tools/firmware/hvmloader/util.c @@ -995,6 +995,12 @@ void hvmloader_acpi_build_tables(struct acpi_config *config, hvm_param_set(HVM_PARAM_VM_GENERATION_ID_ADDR, config->vm_gid_addr); } +void xsm_filter_denied(char *str, size_t len) +{ + if ( strcmp(str, "<denied>") == 0 ) + memset(str, 0, len); +} + /* * Local variables: * mode: C diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h index 7bca6418d2..f7d907ca00 100644 --- a/tools/firmware/hvmloader/util.h +++ b/tools/firmware/hvmloader/util.h @@ -286,6 +286,8 @@ struct acpi_config; void hvmloader_acpi_build_tables(struct acpi_config *config, unsigned int physical); +void xsm_filter_denied(char *str, size_t len); + #endif /* __HVMLOADER_UTIL_H__ */ /* diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index b8e185e6fa..d15b078f10 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -750,14 +750,17 @@ 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_changeset: + case XENVER_commandline: + case XENVER_build_id: default: return xsm_default_action(XSM_PRIV, current->domain, NULL); }
Hide the following information that can help identify the running Xen binary version: XENVER_extraversion, XENVER_compile_info, XENVER_changeset. Add explicit cases for XENVER_commandline and XENVER_build_id as well. Introduce xsm_filter_denied() to hvmloader to remove "<denied>" string from guest's DMI tables that otherwise would be shown in tools like dmidecode. Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> --- v1 --> v2: - Added xsm_filter_denied() to hvmloader instead of modifying xen_deny() - Made behaviour the same for both Release and Debug builds - XENVER_capabilities is no longer hided 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> --- tools/firmware/hvmloader/hvmloader.c | 1 + tools/firmware/hvmloader/smbios.c | 1 + tools/firmware/hvmloader/util.c | 6 ++++++ tools/firmware/hvmloader/util.h | 2 ++ xen/include/xsm/dummy.h | 9 ++++++--- 5 files changed, 16 insertions(+), 3 deletions(-)