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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Nov 10, 2015 at 05:29:35AM -0700, Jan Beulich wrote:
> >>> On 06.11.15 at 20:36, <konrad.wilk@oracle.com> 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.

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

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.

It feels like I am back to square one :-)

My feeling is that those subops I mentioned should have no XSM checks
at all so nobody can mess that up (thought it was fun messing that up).

However Andrew did not like that, but you seem to imply you like that.

Could you two hash out which way you would like this?

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

As this 'len' +1 would lead me to copy one byte extra from the .rodata
section where "<denied>" resides leaking to some information leak.

> Jan
>