Re: [PATCH for-4.16 v3] gnttab: allow setting max version per-domain

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Oct 28, 2021 at 01:12:31PM +0100, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH for-4.16 v3] gnttab: allow setting max 
> version per-domain"):
> > diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> > index 55c4881205..21a39adb70 100644
> > --- a/docs/man/xl.cfg.5.pod.in
> > +++ b/docs/man/xl.cfg.5.pod.in
> > @@ -580,6 +580,12 @@ to have. This value controls how many pages of foreign 
> > domains can be accessed
> >  via the grant mechanism by this domain. The default value is settable via
> >  L<xl.conf(5)>.
> >  
> > +=item B<max_grant_version=NUMBER>
> > +
> > +Specify the maximum grant table version the domain is allowed to use. 
> > Current
> > +supported versions are 1 and 2. The default value is settable via
> > +L<xl.conf(5)>.
> 
> 
> Firstly, review from my maintainer hat:
> 
> In the lower levels of this stack, I'm not sure why you chose -1 for
> "default", when 0 is not (and never will be) used ?

No, I actually had plans to use 0 to signal no grant table support for
the guest, see:

https://lore.kernel.org/xen-devel/20210922082123.54374-7-roger.pau@xxxxxxxxxx/

> > diff --git a/tools/helpers/init-xenstore-domain.c 
> > b/tools/helpers/init-xenstore-domain.c
> > index 6836002f0b..7cd1aa8f7c 100644
> > --- a/tools/helpers/init-xenstore-domain.c
> > +++ b/tools/helpers/init-xenstore-domain.c
> > @@ -88,6 +88,7 @@ static int build(xc_interface *xch)
> >           */
> >          .max_grant_frames = 4,
> >          .max_maptrack_frames = 128,
> > +        .grant_opts = 1,
> >      };
> 
> I think this sets the max gnttab version for xenstore stub domains to
> 1 ?  That's not mentioned in your commit message or your release
> discussion.

Indeed. AFAICT MiniOS only supports grant table v1, and the grant
related parameters max_maptrack_frames and max_grant_frames are
already set based on DEFAULT_MAX_GRANTS and NR_GRANT_FRAMES
respectively as defined in MiniOS code.

Would you like me to this to the commit message:

"xenstored stubdomains are limited to grant table v1 because the
current MiniOS code used to build them only has support for grants v1.
There are existing limits set for xenstored stubdomains at creation
time that already match the defaults in MiniOS."

> 
> Secondly, the release question:
> 
> > Release rationale:
> > 
> > We have had a bunch of security issues involving grant table v2 (382,
> > 379, 268, 255) which could have been avoided by limiting the grant
> > table version available to guests. This can be currently done using a
> > global host parameter, but it's certainly more helpful to be able to
> > do it on a per domain basis from the toolstack.
> 
> Let me think this through.
> 
> Upside:
> 
> So the advantage is to have a mitigation for possible (but, perhaps,
> likely) future security bugs.  We don't change the default here, so
> the default configuration is still vulnerable.
> 
> We have this mitigation already but only on a per-host command line
> basis, so you (1) have to reboot (2) can't use it if you have any
> guests that need v2.  (2) is less of a problem than it sounds because
> even with the selective mitigation you will remain vulnerable to those
> guests.  So the main advantage is having to reboot the guests but not
> the hosts.
> 
> Downside:
> 
> > Changes to the hypervisor by this patch are fairly minimal, as there
> > are already checks for the max grant table version allowed, so the
> > main change there is moving the max grant table version limit inside
> > the domain struct and plumbing it through the toolstrack.
> 
> Right.
> 
> > I think the risk here is quite low for libxl/xl, because it's
> > extensively tested by osstest, so the main risk would be breaking the
> > Ocaml stubs, which could go unnoticed as those are not actually tested
> > by osstest.
> 
> I will want a review by ocaml folks.  Christian ?
> 
> In particular, I would like an opinion from the ocaml tooling
> maintainers about whether there is a risk of this feature breaking
> things *if it's not used*.
> 
> 
> I am not sure about the implications for migration.  Might using this
> cause migration to fail for some guests ?
> 
>    Note that when using the default grant version the specific max
>    version in use by the domain is not migrated. Any guests should be
>    able to cope with the max grant version changing across migrations,
>    and if a specific guest relies on a maximum grant version being
>    unconditionally available it should be specified on the configuration
>    file.
> 
> Only if the feature is *not* used, I guess.  Ie, this is the status
> quo.   So I don't think there is any release risk there.

This was raised by Jan in a previous version, the discussion can be
found here:

https://lore.kernel.org/xen-devel/0b58667f-b6bc-d5b5-2dd1-0c8996367319@xxxxxxxx/

The issue could arise if a guest that strictly needs grant v2 is
migrated from a host that has v2 as the default max version to another
box that has v1 as the max version. If the guest config file doesn't
explicitly specify that the guest requires grant v2 migration will
succeed, but the guest will likely fail to resume properly.

This is already the current behavior if a guest is migrated from a
host not having gnttab=max-ver set to one having gnttab=max-ver:1.

> 
> If this change does cause problems, it is deep throughout the stack,
> but not entangled with anything else, so I think we could revert it ?

I think so, unless we start piling a lot of other toolstack changes on
top it's unlikely to be a problem to revert.

> If we can get good answers to all of this, ideally I would like to see
> this committed by the end of tomorrow.  I plan to cut RC1 on Monday.

Thanks, Roger.