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

[Julien Grall <julien@xxxxxxx>]

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