All of lore.kernel.org
 help / color / mirror / Atom feed
* [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
@ 2020-01-10 10:37 Sergey Dyasli
  2020-01-10 11:02 ` Andrew Cooper
                   ` (2 more replies)
  0 siblings, 3 replies; 22+ messages in thread
From: Sergey Dyasli @ 2020-01-10 10:37 UTC (permalink / raw)
  To: xen-devel
  Cc: Sergey Dyasli, Stefano Stabellini, Julien Grall, Wei Liu,
	Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
	Jan Beulich, Daniel De Graaf

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

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


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

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
  2020-01-10 10:37 [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests Sergey Dyasli
@ 2020-01-10 11:02 ` Andrew Cooper
  2020-01-10 15:28   ` George Dunlap
  2020-01-13 11:01   ` Sergey Dyasli
  2020-01-10 11:09 ` Jan Beulich
  2020-01-11  4:02 ` Doug Goldstein
  2 siblings, 2 replies; 22+ messages in thread
From: Andrew Cooper @ 2020-01-10 11:02 UTC (permalink / raw)
  To: Sergey Dyasli, xen-devel
  Cc: Lars Kurth, Stefano Stabellini, Julien Grall, Wei Liu,
	Konrad Rzeszutek Wilk, George Dunlap, Ian Jackson, Jan Beulich,
	Daniel De Graaf

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

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
  2020-01-10 10:37 [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests Sergey Dyasli
  2020-01-10 11:02 ` Andrew Cooper
@ 2020-01-10 11:09 ` Jan Beulich
  2020-01-11  4:02 ` Doug Goldstein
  2 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2020-01-10 11:09 UTC (permalink / raw)
  To: Sergey Dyasli
  Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
	George Dunlap, Andrew Cooper, Ian Jackson, xen-devel,
	Daniel De Graaf

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

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
  2020-01-10 11:02 ` Andrew Cooper
@ 2020-01-10 15:28   ` George Dunlap
  2020-01-10 15:56     ` Jan Beulich
  2020-01-11  3:55     ` Doug Goldstein
  2020-01-13 11:01   ` Sergey Dyasli
  1 sibling, 2 replies; 22+ messages in thread
From: George Dunlap @ 2020-01-10 15:28 UTC (permalink / raw)
  To: Andrew Cooper, Sergey Dyasli, xen-devel
  Cc: Lars Kurth, Stefano Stabellini, Julien Grall, Wei Liu,
	Konrad Rzeszutek Wilk, George Dunlap, Ian Jackson, Jan Beulich,
	Daniel De Graaf

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

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
  2020-01-10 15:28   ` George Dunlap
@ 2020-01-10 15:56     ` Jan Beulich
  2020-01-10 16:45       ` Jürgen Groß
  2020-01-11  3:55     ` Doug Goldstein
  1 sibling, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2020-01-10 15:56 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Sergey Dyasli, Lars Kurth, Stefano Stabellini, Julien Grall,
	Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Ian Jackson,
	George Dunlap, xen-devel, Daniel De Graaf

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

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
  2020-01-10 15:56     ` Jan Beulich
@ 2020-01-10 16:45       ` Jürgen Groß
  2020-01-10 17:00         ` George Dunlap
  0 siblings, 1 reply; 22+ messages in thread
From: Jürgen Groß @ 2020-01-10 16:45 UTC (permalink / raw)
  To: Jan Beulich, Andrew Cooper
  Cc: Sergey Dyasli, Lars Kurth, Stefano Stabellini, Julien Grall,
	Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Ian Jackson,
	George Dunlap, xen-devel, Daniel De Graaf

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

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
  2020-01-10 16:45       ` Jürgen Groß
@ 2020-01-10 17:00         ` George Dunlap
  0 siblings, 0 replies; 22+ messages in thread
From: George Dunlap @ 2020-01-10 17:00 UTC (permalink / raw)
  To: Jürgen Groß, Jan Beulich, Andrew Cooper
  Cc: Sergey Dyasli, Lars Kurth, Stefano Stabellini, Julien Grall,
	Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Ian Jackson,
	xen-devel, Daniel De Graaf

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

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
  2020-01-10 15:28   ` George Dunlap
  2020-01-10 15:56     ` Jan Beulich
@ 2020-01-11  3:55     ` Doug Goldstein
  2020-01-11  9:35       ` George Dunlap
  1 sibling, 1 reply; 22+ messages in thread
From: Doug Goldstein @ 2020-01-11  3:55 UTC (permalink / raw)
  To: George Dunlap, Andrew Cooper, Sergey Dyasli, xen-devel
  Cc: Lars Kurth, Stefano Stabellini, Julien Grall, Wei Liu,
	Konrad Rzeszutek Wilk, George Dunlap, Ian Jackson, Jan Beulich,
	Daniel De Graaf



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

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
  2020-01-10 10:37 [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests Sergey Dyasli
  2020-01-10 11:02 ` Andrew Cooper
  2020-01-10 11:09 ` Jan Beulich
@ 2020-01-11  4:02 ` Doug Goldstein
  2020-01-11  9:02   ` George Dunlap
  2 siblings, 1 reply; 22+ messages in thread
From: Doug Goldstein @ 2020-01-11  4:02 UTC (permalink / raw)
  To: Sergey Dyasli, xen-devel
  Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
	George Dunlap, Andrew Cooper, Ian Jackson, Jan Beulich,
	Daniel De Graaf



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

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
  2020-01-11  4:02 ` Doug Goldstein
@ 2020-01-11  9:02   ` George Dunlap
  2020-01-12 18:26     ` Doug Goldstein
  0 siblings, 1 reply; 22+ messages in thread
From: George Dunlap @ 2020-01-11  9:02 UTC (permalink / raw)
  To: Doug Goldstein
  Cc: Sergey Dyasli, Stefano Stabellini, Julien Grall, Wei Liu,
	Konrad Rzeszutek Wilk, Andrew Cooper, George Dunlap, Xen-devel,
	Jan Beulich, Ian Jackson, Daniel De Graaf



> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
  2020-01-11  3:55     ` Doug Goldstein
@ 2020-01-11  9:35       ` George Dunlap
  0 siblings, 0 replies; 22+ messages in thread
From: George Dunlap @ 2020-01-11  9:35 UTC (permalink / raw)
  To: Doug Goldstein
  Cc: Lars Kurth, Stefano Stabellini, Julien Grall, Wei Liu,
	Sergey Dyasli, Andrew Cooper, Konrad Rzeszutek Wilk,
	George Dunlap, Xen-devel, Jan Beulich, Ian Jackson,
	Daniel De Graaf



> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
  2020-01-11  9:02   ` George Dunlap
@ 2020-01-12 18:26     ` Doug Goldstein
  2020-01-13 12:51       ` George Dunlap
  2020-01-13 14:01       ` Ian Jackson
  0 siblings, 2 replies; 22+ messages in thread
From: Doug Goldstein @ 2020-01-12 18:26 UTC (permalink / raw)
  To: George Dunlap
  Cc: Sergey Dyasli, Stefano Stabellini, Julien Grall, Wei Liu,
	Konrad Rzeszutek Wilk, Andrew Cooper, Xen-devel, Jan Beulich,
	Ian Jackson, Daniel De Graaf

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

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
  2020-01-10 11:02 ` Andrew Cooper
  2020-01-10 15:28   ` George Dunlap
@ 2020-01-13 11:01   ` Sergey Dyasli
  1 sibling, 0 replies; 22+ messages in thread
From: Sergey Dyasli @ 2020-01-13 11:01 UTC (permalink / raw)
  To: Andrew Cooper, xen-devel
  Cc: Lars Kurth, Stefano Stabellini, Julien Grall, Wei Liu,
	Konrad Rzeszutek Wilk, George Dunlap, Doug Goldstein,
	Ian Jackson, Jan Beulich, Daniel De Graaf,
	sergey.dyasli@citrix.com >> Sergey Dyasli

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

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
  2020-01-12 18:26     ` Doug Goldstein
@ 2020-01-13 12:51       ` George Dunlap
  2020-01-13 13:39         ` Julien Grall
                           ` (2 more replies)
  2020-01-13 14:01       ` Ian Jackson
  1 sibling, 3 replies; 22+ messages in thread
From: George Dunlap @ 2020-01-13 12:51 UTC (permalink / raw)
  To: Doug Goldstein
  Cc: Sergey Dyasli, Stefano Stabellini, Julien Grall, Wei Liu,
	Konrad Rzeszutek Wilk, Andrew Cooper, Xen-devel, Jan Beulich,
	Ian Jackson, Daniel De Graaf

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

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
  2020-01-13 12:51       ` George Dunlap
@ 2020-01-13 13:39         ` Julien Grall
  2020-01-13 14:01           ` Andrew Cooper
  2020-01-13 14:40         ` Andrew Cooper
  2020-01-13 14:52         ` Julien Grall
  2 siblings, 1 reply; 22+ messages in thread
From: Julien Grall @ 2020-01-13 13:39 UTC (permalink / raw)
  To: George Dunlap, Doug Goldstein
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu,
	Konrad Rzeszutek Wilk, Andrew Cooper, Xen-devel, Jan Beulich,
	Ian Jackson, Daniel De Graaf

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,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
  2020-01-13 13:39         ` Julien Grall
@ 2020-01-13 14:01           ` Andrew Cooper
  2020-01-13 14:07             ` George Dunlap
  0 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2020-01-13 14:01 UTC (permalink / raw)
  To: Julien Grall, George Dunlap, Doug Goldstein
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu,
	Konrad Rzeszutek Wilk, Xen-devel, Jan Beulich, Ian Jackson,
	Daniel De Graaf

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

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
  2020-01-12 18:26     ` Doug Goldstein
  2020-01-13 12:51       ` George Dunlap
@ 2020-01-13 14:01       ` Ian Jackson
  1 sibling, 0 replies; 22+ messages in thread
From: Ian Jackson @ 2020-01-13 14:01 UTC (permalink / raw)
  To: Doug Goldstein
  Cc: Sergey Dyasli, Stefano Stabellini, Julien Grall, Wei Liu,
	Konrad Rzeszutek Wilk, Andrew Cooper, George Dunlap, Xen-devel,
	Jan Beulich, Daniel De Graaf

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.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
  2020-01-13 14:01           ` Andrew Cooper
@ 2020-01-13 14:07             ` George Dunlap
  2020-01-13 14:28               ` Julien Grall
  0 siblings, 1 reply; 22+ messages in thread
From: George Dunlap @ 2020-01-13 14:07 UTC (permalink / raw)
  To: Andrew Cooper, Julien Grall, Doug Goldstein
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu,
	Konrad Rzeszutek Wilk, Xen-devel, Jan Beulich, Ian Jackson,
	Daniel De Graaf

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

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
  2020-01-13 14:07             ` George Dunlap
@ 2020-01-13 14:28               ` Julien Grall
  0 siblings, 0 replies; 22+ messages in thread
From: Julien Grall @ 2020-01-13 14:28 UTC (permalink / raw)
  To: George Dunlap, Andrew Cooper, Doug Goldstein
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu,
	Konrad Rzeszutek Wilk, Xen-devel, Jan Beulich, Ian Jackson,
	Daniel De Graaf



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,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
  2020-01-13 12:51       ` George Dunlap
  2020-01-13 13:39         ` Julien Grall
@ 2020-01-13 14:40         ` Andrew Cooper
  2020-01-14 10:19           ` Sergey Dyasli
  2020-01-13 14:52         ` Julien Grall
  2 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2020-01-13 14:40 UTC (permalink / raw)
  To: George Dunlap, Doug Goldstein
  Cc: Sergey Dyasli, Stefano Stabellini, Julien Grall, Wei Liu,
	Konrad Rzeszutek Wilk, Xen-devel, Jan Beulich, Ian Jackson,
	Daniel De Graaf

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

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
  2020-01-13 12:51       ` George Dunlap
  2020-01-13 13:39         ` Julien Grall
  2020-01-13 14:40         ` Andrew Cooper
@ 2020-01-13 14:52         ` Julien Grall
  2 siblings, 0 replies; 22+ messages in thread
From: Julien Grall @ 2020-01-13 14:52 UTC (permalink / raw)
  To: George Dunlap, Doug Goldstein
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu,
	Konrad Rzeszutek Wilk, Andrew Cooper, Paul Durrant, Xen-devel,
	Jan Beulich, Ian Jackson, Daniel De Graaf

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,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests
  2020-01-13 14:40         ` Andrew Cooper
@ 2020-01-14 10:19           ` Sergey Dyasli
  0 siblings, 0 replies; 22+ messages in thread
From: Sergey Dyasli @ 2020-01-14 10:19 UTC (permalink / raw)
  To: Andrew Cooper, George Dunlap, Doug Goldstein
  Cc: sergey.dyasli@citrix.com >> Sergey Dyasli,
	Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
	Xen-devel, Jan Beulich, Ian Jackson, Daniel De Graaf

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

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2020-01-14 10:19 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-10 10:37 [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests Sergey Dyasli
2020-01-10 11:02 ` Andrew Cooper
2020-01-10 15:28   ` George Dunlap
2020-01-10 15:56     ` Jan Beulich
2020-01-10 16:45       ` Jürgen Groß
2020-01-10 17:00         ` George Dunlap
2020-01-11  3:55     ` Doug Goldstein
2020-01-11  9:35       ` George Dunlap
2020-01-13 11:01   ` Sergey Dyasli
2020-01-10 11:09 ` Jan Beulich
2020-01-11  4:02 ` Doug Goldstein
2020-01-11  9:02   ` George Dunlap
2020-01-12 18:26     ` Doug Goldstein
2020-01-13 12:51       ` George Dunlap
2020-01-13 13:39         ` Julien Grall
2020-01-13 14:01           ` Andrew Cooper
2020-01-13 14:07             ` George Dunlap
2020-01-13 14:28               ` Julien Grall
2020-01-13 14:40         ` Andrew Cooper
2020-01-14 10:19           ` Sergey Dyasli
2020-01-13 14:52         ` Julien Grall
2020-01-13 14:01       ` Ian Jackson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.