Re: [PATCH] xen/grants: repurpose command line max options

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Mar 13, 2023 at 05:55:09PM +0100, Jan Beulich wrote:
> On 13.03.2023 13:16, Roger Pau Monne wrote:
> > --- a/docs/misc/xen-command-line.pandoc
> > +++ b/docs/misc/xen-command-line.pandoc
> > @@ -1232,9 +1232,8 @@ The usage of gnttab v2 is not security supported on ARM platforms.
> >  
> >  > Can be modified at runtime
> >  
> > -Specify the maximum number of frames which any domain may use as part
> > -of its grant table. This value is an upper boundary of the per-domain
> > -value settable via Xen tools.
> > +Specify the default maximum number of frames which any domain may use as part
> > +of its grant table unless a different value is specified at domain creation.
> >  
> >  Dom0 is using this value for sizing its grant table.
> 
> dom0less DomU-s do as well, at the very least, also ...
> 
> > @@ -1245,9 +1244,10 @@ Dom0 is using this value for sizing its grant table.
> >  
> >  > Can be modified at runtime
> >  
> > -Specify the maximum number of frames to use as part of a domains
> > -maptrack array. This value is an upper boundary of the per-domain
> > -value settable via Xen tools.
> > +Specify the default maximum number of frames to use as part of a domains
> > +maptrack array unless a different value is specified at domain creation.
> > +
> > +Dom0 is using this value for sizing its maptrack array.
> 
> ... here. And even ordinary DomU-s appear to default to that in the
> absence of a specific value in the guest config. IOW at the very least
> the info you add should not be misleading. Better would be if the pre-
> existing info was adjusted at the same time.

Aren't domUs already clearly covered by the sentence:

"Specify the default maximum number of frames to use as part of a domains..."

IMO dom0 needs to be explicitly mentioned because in that case the
value provided is not the one used by default, but rather the one that
gets used.

> I also wonder about the specific wording down here: While the max grant
> table size can indeed be queried, this isn't the case for the maptrack
> array. A domain also doesn't need to know its size, so maybe "This value
> is used to size all domains' maptrack arrays, unless overridden by their
> guest config"?

I think the wording I've added already conveys this meaning:

"Specify the default maximum number of frames to use as part of a domains
maptrack array unless a different value is specified at domain creation."

> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -1956,18 +1956,15 @@ int grant_table_init(struct domain *d, int max_grant_frames,
> >          return -EINVAL;
> >      }
> >  
> > -    /* Default to maximum value if no value was specified */
> > +    /* Apply defaults if no value was specified */
> >      if ( max_grant_frames < 0 )
> >          max_grant_frames = opt_max_grant_frames;
> >      if ( max_maptrack_frames < 0 )
> >          max_maptrack_frames = opt_max_maptrack_frames;
> >  
> > -    if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
> > -         max_grant_frames > opt_max_grant_frames ||
> > -         max_maptrack_frames > opt_max_maptrack_frames )
> > +    if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES )
> >      {
> > -        dprintk(XENLOG_INFO, "Bad grant table sizes: grant %u, maptrack %u\n",
> > -                max_grant_frames, max_maptrack_frames);
> > +        dprintk(XENLOG_INFO, "Bad grant table size %u\n", max_grant_frames);
> >          return -EINVAL;
> >      }
> 
> I think I agree with the relaxation done here, but I also think this not
> introducing security concerns wants spelling out in the description: My
> understanding is that even in disaggregated environments we assume only
> fully privileged entities can create domains.

Yes, that's my understanding, as domain creation can only be done by
privileged domains.  Of course when using a custom XSM policy
the permissions can be changed, but it's then the job of the user to
asses the security implications in that case, and there are existing
paths to cause resource exhausting when having access to the domain
create hypercall. I can add:

"The relaxation in the logic for the maximum size of the grant and
maptrack table sizes doesn't change the fact that domain creation
hypercall can cause resource exhausting, so disaggregated setups
should take it into account."

But domain creation for example also allows creating a domain that has
MSR relaxed, at which point it could also be vulnerable to other
issues.

Thanks, Roger.