Re: [Xen-devel] [PATCH v2 1/3] xsm/xen_version: Add XSM for the xen_version hypercall.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.01.16 at 18:41, <konrad.wilk@xxxxxxxxxx> wrote:
> On Tue, Nov 10, 2015 at 05:29:35AM -0700, Jan Beulich wrote:
>> >>> On 06.11.15 at 20:36, <konrad.wilk@xxxxxxxxxx> wrote:
>> > All of XENVER_* have now an XSM check.
>> > 
>> > The subops for XENVER_[compile_info|changeset|commandline|
>> > extraversion] are now priviliged operations. To not break
>> > guests we still return an string - but it is just '<denied>'.
>> 
>> And I continue to question at least the extraversion part.
> 
> How about I remove the extraversion part from this patch and we can
> discuss putting 'extraversion' in the blacklist around another patch.

Yes, that'd be a step towards me agreeing with the change. I'm
not necessarily saying that's going to be enough, since I said "at
least", i.e. I continue to wonder how relevant it really is to hide
changeset and compile info (fwiw I agree with hiding the command
line).

>> > The rest: XENVER_[version|capabilities|
>> > parameters|get_features|page_size|guest_handle] behave
>> > as before - allowed by default for all guests.
>> > 
>> > This is with the XSM default policy and with the dummy ones.
>> 
>> And with a non-default policy you now ignore one of the latter
>> ops to also get denied.
> 
> No, but that is due to the 'deny' being only checked for certain subops.

To me this reply seems contradictory in itself: The "no" doesn't
seem to match up with the rest.

> I think what you are saying is that for XENVER_[version|capabilities|
> parameters|get_features|page_size|guest_handle] we should not have any
> XSM checks as they serve no purpose (which is what I had in the earlier
> versions of this patch). However Andrew mentioned that he would
> like _ALL_ of the sub-ops to be checked for.

And I agree with Andrew, hence my earlier comment above (with
the reply I can't really make sense of).

>> > @@ -354,10 +356,17 @@ DO(xen_version)(int cmd, 
>> > XEN_GUEST_HANDLE_PARAM(void) arg)
>> >          return 0;
>> >  
>> >      case XENVER_commandline:
>> > -        if ( copy_to_guest(arg, saved_cmdline, ARRAY_SIZE(saved_cmdline)) 
>> > )
>> > +    {
>> > +        size_t len = ARRAY_SIZE(saved_cmdline);
>> > +
>> > +        if ( deny )
>> > +            len = strlen(xen_deny());
>> 
>> +1 (or else you fail to nul-terminate the output).
> 
> Nice spotting!
> Perhaps modifying xen_deny() to be:
> 
> const char *xen_deny(void)
> {
>     return "<denied>\0";
> }
> 
> Would be better?

This would just add a second NUL at the end, without altering what
strlen() returns on that string.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel