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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

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

I made it now only return '<denied>' for the XENVER_commandline.

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

I am all confused now.

There are two parts here:
 a) The XSM checks - which allow the XENVER_version..XENVER_guest_handle
   without any hinderance. For XENVER_commandline and XENVER_buildid
   they are evaluated.

 b) Acting on the XSM check. For most of them we cannot actually return
   -EFAULT and MUST return either an valid value or some form of a string.
   
   The ones for which we could return '<denied>' were changeset, compile_info,
   commandline, extraversion. To make it simpler we only do it for
   commandline.

In essence we have an XSM check which is ignored by all XENVER_ subops
except commandline (and build_id in later patch).

I think both of you are OK with that?

> 
> >> > @@ -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

Right. Sorry about that - the patch I had sent still includes the \0.

> strlen() returns on that string
> 
> Jan
> 

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