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

[Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>]

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