[Xen-devel] [PATCH] xsm: hide detailed Xen version from unprivileged guests

[Xen-devel] [PATCH] xsm: hide detailed Xen version from unprivileged guests

[Sergey Dyasli]

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(-)

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);
     }
 }
 
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xsm: hide detailed Xen version from unprivileged guests

[Jan Beulich]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xsm: hide detailed Xen version from unprivileged guests

[Sergey Dyasli]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xsm: hide detailed Xen version from unprivileged guests

[Jan Beulich]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xsm: hide detailed Xen version from unprivileged guests

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xsm: hide detailed Xen version from unprivileged guests

[Andrew Cooper]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xsm: hide detailed Xen version from unprivileged guests

[Andrew Cooper]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xsm: hide detailed Xen version from unprivileged guests

[Jan Beulich]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xsm: hide detailed Xen version from unprivileged guests

[George Dunlap]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xsm: hide detailed Xen version from unprivileged guests

[Sergey Dyasli]

[-- Attachment #1: Type: text/plain, Size: 1974 bytes --]

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
> 

[-- Attachment #2: xen_msinfo.png --]
[-- Type: image/png, Size: 58846 bytes --]

[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xsm: hide detailed Xen version from unprivileged guests

[Jan Beulich]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xsm: hide detailed Xen version from unprivileged guests

[Sergey Dyasli]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xsm: hide detailed Xen version from unprivileged guests

[Jan Beulich]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel