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

[Ian Jackson <iwj@xxxxxxxxxxxxxx>]

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 ?

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


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.


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 ?


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.

I don't currently want to rule out allowing this to go in early next
week but that is getting considerably less desirable.


Thanks,
Ian.