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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.03.2023 10:22, Roger Pau Monné wrote:
> 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..."

Hmm, yes, my attention was caught too much by the Dom0 statement. While ...

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

... explicitly mentioning Dom0 is fine, I still think this needs wording
differently here, because Dom0 doesn't actively do anything with this
value (and, as said, it can't even obtain it other than by probing how
many mappings it can create).

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

Well, I mean specifically the Dom0 related statement.

Also to me "default maximum" reads odd (and slightly ambiguous). Would
"default upper bound on the number of ..." perhaps be a little better?

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

SGTM, and ...

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

... I think while what you say is true, it still is relevant to point
out for every change which may cause yet another way of running out of
resources.

Jan