| Message ID | 1503586226-12510-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 24/08/17 16:50, Andrew Cooper wrote: > This patch was originally a workaround for XSA-226. Since then, transitive > grants are believed to be functioning properly, and the defaults have changed > appropriately. > > However, for those people who chose to use the workaround (especially from an > attack surface mitigation point of view), retain the ability for the host > administrator to choose. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: George Dunlap <George.Dunlap@eu.citrix.com> > CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Tim Deegan <tim@xen.org> > CC: Wei Liu <wei.liu2@citrix.com> > --- > docs/misc/xen-command-line.markdown | 13 +++++++++++ > xen/common/grant_table.c | 44 +++++++++++++++++++++++++++++++++++-- > 2 files changed, 55 insertions(+), 2 deletions(-) > > diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown > index 4002eab..78c7b51 100644 > --- a/docs/misc/xen-command-line.markdown > +++ b/docs/misc/xen-command-line.markdown > @@ -868,6 +868,19 @@ Controls EPT related features. > > Specify which console gdbstub should use. See **console**. > > +### gnttab > +> `= List of [ max_ver:<integer>, transitive ]` > + > +> Default: `gnttab=max_ver:2,transitive` > + > +Control various aspects of the grant table behaviour available to guests. > + > +* `max_ver` Select the maximum grant table version to offer to guests. Valid > +version are 1 and 2. > +* `transitive` Permit or disallow the use of transitive grants. Note that the > +use of grant table v2 without transitive grants is an ABI breakage from the > +guests point of view. So shouldn't there be a way for the guest to query the support of transient grants? > + > ### gnttab\_max\_frames > > `= <integer>` > > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c > index 36895aa..f9c313d 100644 > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -50,6 +50,42 @@ integer_param("gnttab_max_nr_frames", max_nr_grant_frames); > unsigned int __read_mostly max_grant_frames; > integer_param("gnttab_max_frames", max_grant_frames); > > +static unsigned int __read_mostly opt_gnttab_max_version = 2; > +static bool __read_mostly opt_transitive_grants = true; > + > +static void __init parse_gnttab(char *s) Do you mind using: static int __init parse_gnttab(const char *s) in order to comply with my runtime parameter series? Juergen
On 24/08/17 16:01, Juergen Gross wrote: > On 24/08/17 16:50, Andrew Cooper wrote: >> This patch was originally a workaround for XSA-226. Since then, transitive >> grants are believed to be functioning properly, and the defaults have changed >> appropriately. >> >> However, for those people who chose to use the workaround (especially from an >> attack surface mitigation point of view), retain the ability for the host >> administrator to choose. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: George Dunlap <George.Dunlap@eu.citrix.com> >> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >> CC: Stefano Stabellini <sstabellini@kernel.org> >> CC: Tim Deegan <tim@xen.org> >> CC: Wei Liu <wei.liu2@citrix.com> >> --- >> docs/misc/xen-command-line.markdown | 13 +++++++++++ >> xen/common/grant_table.c | 44 +++++++++++++++++++++++++++++++++++-- >> 2 files changed, 55 insertions(+), 2 deletions(-) >> >> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown >> index 4002eab..78c7b51 100644 >> --- a/docs/misc/xen-command-line.markdown >> +++ b/docs/misc/xen-command-line.markdown >> @@ -868,6 +868,19 @@ Controls EPT related features. >> >> Specify which console gdbstub should use. See **console**. >> >> +### gnttab >> +> `= List of [ max_ver:<integer>, transitive ]` >> + >> +> Default: `gnttab=max_ver:2,transitive` >> + >> +Control various aspects of the grant table behaviour available to guests. >> + >> +* `max_ver` Select the maximum grant table version to offer to guests. Valid >> +version are 1 and 2. >> +* `transitive` Permit or disallow the use of transitive grants. Note that the >> +use of grant table v2 without transitive grants is an ABI breakage from the >> +guests point of view. > So shouldn't there be a way for the guest to query the support of > transient grants? Ideally yes, but how do you suggest doing this in a compatible way? All Xen downstreams which haven't backported the eventual transitive fixes will have this clobber in place, without any query-ability. The reason this workaround was so effective, and why several large users still wish to clobber them, is because nothing uses transitive grants. > >> + >> ### gnttab\_max\_frames >> > `= <integer>` >> >> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c >> index 36895aa..f9c313d 100644 >> --- a/xen/common/grant_table.c >> +++ b/xen/common/grant_table.c >> @@ -50,6 +50,42 @@ integer_param("gnttab_max_nr_frames", max_nr_grant_frames); >> unsigned int __read_mostly max_grant_frames; >> integer_param("gnttab_max_frames", max_grant_frames); >> >> +static unsigned int __read_mostly opt_gnttab_max_version = 2; >> +static bool __read_mostly opt_transitive_grants = true; >> + >> +static void __init parse_gnttab(char *s) > Do you mind using: > > static int __init parse_gnttab(const char *s) > > in order to comply with my runtime parameter series? If the result will still compile. What should the semantics be? (I've had a quick look through your series, but I can't see the semantics stated anywhere obvious) ~Andrew
On 24/08/17 17:16, Andrew Cooper wrote: > On 24/08/17 16:01, Juergen Gross wrote: >> On 24/08/17 16:50, Andrew Cooper wrote: >>> This patch was originally a workaround for XSA-226. Since then, transitive >>> grants are believed to be functioning properly, and the defaults have changed >>> appropriately. >>> >>> However, for those people who chose to use the workaround (especially from an >>> attack surface mitigation point of view), retain the ability for the host >>> administrator to choose. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> --- >>> CC: Jan Beulich <JBeulich@suse.com> >>> CC: George Dunlap <George.Dunlap@eu.citrix.com> >>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >>> CC: Stefano Stabellini <sstabellini@kernel.org> >>> CC: Tim Deegan <tim@xen.org> >>> CC: Wei Liu <wei.liu2@citrix.com> >>> --- >>> docs/misc/xen-command-line.markdown | 13 +++++++++++ >>> xen/common/grant_table.c | 44 +++++++++++++++++++++++++++++++++++-- >>> 2 files changed, 55 insertions(+), 2 deletions(-) >>> >>> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown >>> index 4002eab..78c7b51 100644 >>> --- a/docs/misc/xen-command-line.markdown >>> +++ b/docs/misc/xen-command-line.markdown >>> @@ -868,6 +868,19 @@ Controls EPT related features. >>> >>> Specify which console gdbstub should use. See **console**. >>> >>> +### gnttab >>> +> `= List of [ max_ver:<integer>, transitive ]` >>> + >>> +> Default: `gnttab=max_ver:2,transitive` >>> + >>> +Control various aspects of the grant table behaviour available to guests. >>> + >>> +* `max_ver` Select the maximum grant table version to offer to guests. Valid >>> +version are 1 and 2. >>> +* `transitive` Permit or disallow the use of transitive grants. Note that the >>> +use of grant table v2 without transitive grants is an ABI breakage from the >>> +guests point of view. >> So shouldn't there be a way for the guest to query the support of >> transient grants? > > Ideally yes, but how do you suggest doing this in a compatible way? An ELF note in the guest kernel indicating it is aware of that possibility in order to allow v2 grants for that guest without transient grants? > All Xen downstreams which haven't backported the eventual transitive > fixes will have this clobber in place, without any query-ability. So the only really compatible way would be to disallow v2 grants. OTOH I guess there aren't so many guests using v2 right now... > The reason this workaround was so effective, and why several large users > still wish to clobber them, is because nothing uses transitive grants. Right. And only very few guests use v2 grants. > >> >>> + >>> ### gnttab\_max\_frames >>> > `= <integer>` >>> >>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c >>> index 36895aa..f9c313d 100644 >>> --- a/xen/common/grant_table.c >>> +++ b/xen/common/grant_table.c >>> @@ -50,6 +50,42 @@ integer_param("gnttab_max_nr_frames", max_nr_grant_frames); >>> unsigned int __read_mostly max_grant_frames; >>> integer_param("gnttab_max_frames", max_grant_frames); >>> >>> +static unsigned int __read_mostly opt_gnttab_max_version = 2; >>> +static bool __read_mostly opt_transitive_grants = true; >>> + >>> +static void __init parse_gnttab(char *s) >> Do you mind using: >> >> static int __init parse_gnttab(const char *s) >> >> in order to comply with my runtime parameter series? > > If the result will still compile. What should the semantics be? (I've > had a quick look through your series, but I can't see the semantics > stated anywhere obvious) The parsing routine must not change the parameter string and should return an error (e.g. -EINVAL) in case of an illegal parameter. Juergen
On 24/08/17 18:45, Juergen Gross wrote: > On 24/08/17 17:16, Andrew Cooper wrote: >> On 24/08/17 16:01, Juergen Gross wrote: >>> On 24/08/17 16:50, Andrew Cooper wrote: >>>> This patch was originally a workaround for XSA-226. Since then, transitive >>>> grants are believed to be functioning properly, and the defaults have changed >>>> appropriately. >>>> >>>> However, for those people who chose to use the workaround (especially from an >>>> attack surface mitigation point of view), retain the ability for the host >>>> administrator to choose. >>>> >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> --- >>>> CC: Jan Beulich <JBeulich@suse.com> >>>> CC: George Dunlap <George.Dunlap@eu.citrix.com> >>>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >>>> CC: Stefano Stabellini <sstabellini@kernel.org> >>>> CC: Tim Deegan <tim@xen.org> >>>> CC: Wei Liu <wei.liu2@citrix.com> >>>> --- >>>> docs/misc/xen-command-line.markdown | 13 +++++++++++ >>>> xen/common/grant_table.c | 44 +++++++++++++++++++++++++++++++++++-- >>>> 2 files changed, 55 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown >>>> index 4002eab..78c7b51 100644 >>>> --- a/docs/misc/xen-command-line.markdown >>>> +++ b/docs/misc/xen-command-line.markdown >>>> @@ -868,6 +868,19 @@ Controls EPT related features. >>>> >>>> Specify which console gdbstub should use. See **console**. >>>> >>>> +### gnttab >>>> +> `= List of [ max_ver:<integer>, transitive ]` >>>> + >>>> +> Default: `gnttab=max_ver:2,transitive` >>>> + >>>> +Control various aspects of the grant table behaviour available to guests. >>>> + >>>> +* `max_ver` Select the maximum grant table version to offer to guests. Valid >>>> +version are 1 and 2. >>>> +* `transitive` Permit or disallow the use of transitive grants. Note that the >>>> +use of grant table v2 without transitive grants is an ABI breakage from the >>>> +guests point of view. >>> So shouldn't there be a way for the guest to query the support of >>> transient grants? >> Ideally yes, but how do you suggest doing this in a compatible way? > An ELF note in the guest kernel indicating it is aware of that > possibility in order to allow v2 grants for that guest without transient > grants? > >> All Xen downstreams which haven't backported the eventual transitive >> fixes will have this clobber in place, without any query-ability. > So the only really compatible way would be to disallow v2 grants. OTOH > I guess there aren't so many guests using v2 right now... > >> The reason this workaround was so effective, and why several large users >> still wish to clobber them, is because nothing uses transitive grants. > Right. And only very few guests use v2 grants. We tried disabling gnttab v2 by default first, which is why the max_ver: parameter is present in this patch. Some vendor versions of Linux refused to fall back from gnttab v2 to gnttab v1 on migrate, even though they are perfectly happy booting in a v1-only situation. Also https://github.com/xenserver/win-xenbus/blob/c2a60d437b42f2349361629f15275e4fabdcc22e/src/xenbus/gnttab.c#L566-L571 which was discovered out of the blue, when all windows guests installed on older versions of XenServer started turning blue. Leaving v2 active and creating an ABI breakage turned out to be the viable way to inhibit the use of transitive grants in cases which needed to run arbitrary guests. > >>>> + >>>> ### gnttab\_max\_frames >>>> > `= <integer>` >>>> >>>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c >>>> index 36895aa..f9c313d 100644 >>>> --- a/xen/common/grant_table.c >>>> +++ b/xen/common/grant_table.c >>>> @@ -50,6 +50,42 @@ integer_param("gnttab_max_nr_frames", max_nr_grant_frames); >>>> unsigned int __read_mostly max_grant_frames; >>>> integer_param("gnttab_max_frames", max_grant_frames); >>>> >>>> +static unsigned int __read_mostly opt_gnttab_max_version = 2; >>>> +static bool __read_mostly opt_transitive_grants = true; >>>> + >>>> +static void __init parse_gnttab(char *s) >>> Do you mind using: >>> >>> static int __init parse_gnttab(const char *s) >>> >>> in order to comply with my runtime parameter series? >> If the result will still compile. What should the semantics be? (I've >> had a quick look through your series, but I can't see the semantics >> stated anywhere obvious) > The parsing routine must not change the parameter string and should > return an error (e.g. -EINVAL) in case of an illegal parameter. Let me see what I can do. ~Andrew
>>> On 24.08.17 at 16:50, <andrew.cooper3@citrix.com> wrote: > --- a/docs/misc/xen-command-line.markdown > +++ b/docs/misc/xen-command-line.markdown > @@ -868,6 +868,19 @@ Controls EPT related features. > > Specify which console gdbstub should use. See **console**. > > +### gnttab > +> `= List of [ max_ver:<integer>, transitive ]` > + > +> Default: `gnttab=max_ver:2,transitive` > + > +Control various aspects of the grant table behaviour available to guests. > + > +* `max_ver` Select the maximum grant table version to offer to guests. Valid > +version are 1 and 2. > +* `transitive` Permit or disallow the use of transitive grants. Note that the > +use of grant table v2 without transitive grants is an ABI breakage from the > +guests point of view. Btw, with the need to use v2 on huge systems I'm no longer convinced providing an option to disable it is a good idea. Jan
>>> On 24.08.17 at 17:16, <andrew.cooper3@citrix.com> wrote: > On 24/08/17 16:01, Juergen Gross wrote: >> On 24/08/17 16:50, Andrew Cooper wrote: >>> --- a/docs/misc/xen-command-line.markdown >>> +++ b/docs/misc/xen-command-line.markdown >>> @@ -868,6 +868,19 @@ Controls EPT related features. >>> >>> Specify which console gdbstub should use. See **console**. >>> >>> +### gnttab >>> +> `= List of [ max_ver:<integer>, transitive ]` >>> + >>> +> Default: `gnttab=max_ver:2,transitive` >>> + >>> +Control various aspects of the grant table behaviour available to guests. >>> + >>> +* `max_ver` Select the maximum grant table version to offer to guests. Valid >>> +version are 1 and 2. >>> +* `transitive` Permit or disallow the use of transitive grants. Note that the >>> +use of grant table v2 without transitive grants is an ABI breakage from the >>> +guests point of view. >> So shouldn't there be a way for the guest to query the support of >> transient grants? > > Ideally yes, but how do you suggest doing this in a compatible way? > > All Xen downstreams which haven't backported the eventual transitive > fixes will have this clobber in place, without any query-ability. That workaround should not be used as an argument to not provide a way to query the capability. It was put in place knowing that it would cause problems for (hypothetical) guests using transitive grants. I'm not sure Jürgen's ELF note suggestion would be very useful though: I don't see how Xen knowing a guest kernel can deal with the situation would change anything - I don't think we should fail the loading of a kernel without such a note when transitive grants are disabled, not the least because we know of no kernels using them, and hence we'd pointlessly prevent the use of older kernels in such a case. What about a negative XENFEAT_*? New code could query it, and existing code is hosed anyway if run on such a system. Jan
On 25/08/17 10:49, Jan Beulich wrote: >>>> On 24.08.17 at 16:50, <andrew.cooper3@citrix.com> wrote: >> --- a/docs/misc/xen-command-line.markdown >> +++ b/docs/misc/xen-command-line.markdown >> @@ -868,6 +868,19 @@ Controls EPT related features. >> >> Specify which console gdbstub should use. See **console**. >> >> +### gnttab >> +> `= List of [ max_ver:<integer>, transitive ]` >> + >> +> Default: `gnttab=max_ver:2,transitive` >> + >> +Control various aspects of the grant table behaviour available to guests. >> + >> +* `max_ver` Select the maximum grant table version to offer to guests. Valid >> +version are 1 and 2. >> +* `transitive` Permit or disallow the use of transitive grants. Note that the >> +use of grant table v2 without transitive grants is an ABI breakage from the >> +guests point of view. > Btw, with the need to use v2 on huge systems I'm no longer > convinced providing an option to disable it is a good idea. "necessary to support larger systems" is not a valid reason to prevent smaller systems having the option to reduce their hypervisor attack surface. We have an unnecessarily large number of XSAs from hypervisor features which noone uses, and a similarly large number of XSAs from features which are only used in specialised usecases. Removing unused attack surfaces in common cases makes perfect sense for downstreams. ~Andrew
On 25/08/17 10:57, Jan Beulich wrote: >>>> On 24.08.17 at 17:16, <andrew.cooper3@citrix.com> wrote: >> On 24/08/17 16:01, Juergen Gross wrote: >>> On 24/08/17 16:50, Andrew Cooper wrote: >>>> --- a/docs/misc/xen-command-line.markdown >>>> +++ b/docs/misc/xen-command-line.markdown >>>> @@ -868,6 +868,19 @@ Controls EPT related features. >>>> >>>> Specify which console gdbstub should use. See **console**. >>>> >>>> +### gnttab >>>> +> `= List of [ max_ver:<integer>, transitive ]` >>>> + >>>> +> Default: `gnttab=max_ver:2,transitive` >>>> + >>>> +Control various aspects of the grant table behaviour available to guests. >>>> + >>>> +* `max_ver` Select the maximum grant table version to offer to guests. Valid >>>> +version are 1 and 2. >>>> +* `transitive` Permit or disallow the use of transitive grants. Note that the >>>> +use of grant table v2 without transitive grants is an ABI breakage from the >>>> +guests point of view. >>> So shouldn't there be a way for the guest to query the support of >>> transient grants? >> Ideally yes, but how do you suggest doing this in a compatible way? >> >> All Xen downstreams which haven't backported the eventual transitive >> fixes will have this clobber in place, without any query-ability. > That workaround should not be used as an argument to not > provide a way to query the capability. It was put in place knowing > that it would cause problems for (hypothetical) guests using > transitive grants. I am not objecting to introducing a mechanism if a suitable one can be found. However, the heritage of XSA-226 is a valid reason to not block this patch because a mechanism isn't present. > > I'm not sure Jürgen's ELF note suggestion would be very useful > though: I don't see how Xen knowing a guest kernel can deal with > the situation would change anything - I don't think we should > fail the loading of a kernel without such a note when transitive > grants are disabled, not the least because we know of no kernels > using them, and hence we'd pointlessly prevent the use of older > kernels in such a case. > > What about a negative XENFEAT_*? New code could query it, > and existing code is hosed anyway if run on such a system. Better yet, how about combining it with Juergens "xen: add new hypercall to get grant table limits"? We could have a features_available bitmap along with other gnttab related maxima. ~Andrew
On 25/08/17 14:10, Andrew Cooper wrote: > On 25/08/17 10:57, Jan Beulich wrote: >>>>> On 24.08.17 at 17:16, <andrew.cooper3@citrix.com> wrote: >>> On 24/08/17 16:01, Juergen Gross wrote: >>>> On 24/08/17 16:50, Andrew Cooper wrote: >>>>> --- a/docs/misc/xen-command-line.markdown >>>>> +++ b/docs/misc/xen-command-line.markdown >>>>> @@ -868,6 +868,19 @@ Controls EPT related features. >>>>> >>>>> Specify which console gdbstub should use. See **console**. >>>>> >>>>> +### gnttab >>>>> +> `= List of [ max_ver:<integer>, transitive ]` >>>>> + >>>>> +> Default: `gnttab=max_ver:2,transitive` >>>>> + >>>>> +Control various aspects of the grant table behaviour available to guests. >>>>> + >>>>> +* `max_ver` Select the maximum grant table version to offer to guests. Valid >>>>> +version are 1 and 2. >>>>> +* `transitive` Permit or disallow the use of transitive grants. Note that the >>>>> +use of grant table v2 without transitive grants is an ABI breakage from the >>>>> +guests point of view. >>>> So shouldn't there be a way for the guest to query the support of >>>> transient grants? >>> Ideally yes, but how do you suggest doing this in a compatible way? >>> >>> All Xen downstreams which haven't backported the eventual transitive >>> fixes will have this clobber in place, without any query-ability. >> That workaround should not be used as an argument to not >> provide a way to query the capability. It was put in place knowing >> that it would cause problems for (hypothetical) guests using >> transitive grants. > > I am not objecting to introducing a mechanism if a suitable one can be > found. > > However, the heritage of XSA-226 is a valid reason to not block this > patch because a mechanism isn't present. > >> >> I'm not sure Jürgen's ELF note suggestion would be very useful >> though: I don't see how Xen knowing a guest kernel can deal with >> the situation would change anything - I don't think we should >> fail the loading of a kernel without such a note when transitive >> grants are disabled, not the least because we know of no kernels >> using them, and hence we'd pointlessly prevent the use of older >> kernels in such a case. >> >> What about a negative XENFEAT_*? New code could query it, >> and existing code is hosed anyway if run on such a system. > > Better yet, how about combining it with Juergens "xen: add new hypercall > to get grant table limits"? I suspect this new hypercall has just been killed. > We could have a features_available bitmap along with other gnttab > related maxima. Feel free to recycle my patch then. :-) OTOH XENFEAT_* might be just the place where such information should be made available. Juergen
>>> On 25.08.17 at 14:05, <andrew.cooper3@citrix.com> wrote: > On 25/08/17 10:49, Jan Beulich wrote: >>>>> On 24.08.17 at 16:50, <andrew.cooper3@citrix.com> wrote: >>> --- a/docs/misc/xen-command-line.markdown >>> +++ b/docs/misc/xen-command-line.markdown >>> @@ -868,6 +868,19 @@ Controls EPT related features. >>> >>> Specify which console gdbstub should use. See **console**. >>> >>> +### gnttab >>> +> `= List of [ max_ver:<integer>, transitive ]` >>> + >>> +> Default: `gnttab=max_ver:2,transitive` >>> + >>> +Control various aspects of the grant table behaviour available to guests. >>> + >>> +* `max_ver` Select the maximum grant table version to offer to guests. Valid >>> +version are 1 and 2. >>> +* `transitive` Permit or disallow the use of transitive grants. Note that the >>> +use of grant table v2 without transitive grants is an ABI breakage from the >>> +guests point of view. >> Btw, with the need to use v2 on huge systems I'm no longer >> convinced providing an option to disable it is a good idea. > > "necessary to support larger systems" is not a valid reason to prevent > smaller systems having the option to reduce their hypervisor attack surface. Well, yes, one can view it that way. Two questions then, though: 1) If at some point someone comes up with a much better quality v3, how will your option syntax fit that (i.e. to exclude just v2)? 2) Switching between versions (post-migration) requires extra code in guests, albeit perhaps not very much. Is it wise to require OSes to be capable of switching back and forth? Jan
>>> On 25.08.17 at 14:10, <andrew.cooper3@citrix.com> wrote: > On 25/08/17 10:57, Jan Beulich wrote: >>>>> On 24.08.17 at 17:16, <andrew.cooper3@citrix.com> wrote: >>> On 24/08/17 16:01, Juergen Gross wrote: >>>> On 24/08/17 16:50, Andrew Cooper wrote: >>>>> --- a/docs/misc/xen-command-line.markdown >>>>> +++ b/docs/misc/xen-command-line.markdown >>>>> @@ -868,6 +868,19 @@ Controls EPT related features. >>>>> >>>>> Specify which console gdbstub should use. See **console**. >>>>> >>>>> +### gnttab >>>>> +> `= List of [ max_ver:<integer>, transitive ]` >>>>> + >>>>> +> Default: `gnttab=max_ver:2,transitive` >>>>> + >>>>> +Control various aspects of the grant table behaviour available to guests. >>>>> + >>>>> +* `max_ver` Select the maximum grant table version to offer to guests. Valid >>>>> +version are 1 and 2. >>>>> +* `transitive` Permit or disallow the use of transitive grants. Note that the >>>>> +use of grant table v2 without transitive grants is an ABI breakage from the >>>>> +guests point of view. >>>> So shouldn't there be a way for the guest to query the support of >>>> transient grants? >>> Ideally yes, but how do you suggest doing this in a compatible way? >>> >>> All Xen downstreams which haven't backported the eventual transitive >>> fixes will have this clobber in place, without any query-ability. >> That workaround should not be used as an argument to not >> provide a way to query the capability. It was put in place knowing >> that it would cause problems for (hypothetical) guests using >> transitive grants. > > I am not objecting to introducing a mechanism if a suitable one can be > found. > > However, the heritage of XSA-226 is a valid reason to not block this > patch because a mechanism isn't present. Code submission deadline for 4.10 isn't very far away; we shouldn't ship a major version with a partial workaround. >> I'm not sure Jürgen's ELF note suggestion would be very useful >> though: I don't see how Xen knowing a guest kernel can deal with >> the situation would change anything - I don't think we should >> fail the loading of a kernel without such a note when transitive >> grants are disabled, not the least because we know of no kernels >> using them, and hence we'd pointlessly prevent the use of older >> kernels in such a case. >> >> What about a negative XENFEAT_*? New code could query it, >> and existing code is hosed anyway if run on such a system. > > Better yet, how about combining it with Juergens "xen: add new hypercall > to get grant table limits"? > > We could have a features_available bitmap along with other gnttab > related maxima. That's certainly an option, if the introduction of that sub-op really continues to be necessary. Jan
On 25/08/17 14:29, Jan Beulich wrote: >>>> On 25.08.17 at 14:05, <andrew.cooper3@citrix.com> wrote: >> On 25/08/17 10:49, Jan Beulich wrote: >>>>>> On 24.08.17 at 16:50, <andrew.cooper3@citrix.com> wrote: >>>> --- a/docs/misc/xen-command-line.markdown >>>> +++ b/docs/misc/xen-command-line.markdown >>>> @@ -868,6 +868,19 @@ Controls EPT related features. >>>> >>>> Specify which console gdbstub should use. See **console**. >>>> >>>> +### gnttab >>>> +> `= List of [ max_ver:<integer>, transitive ]` >>>> + >>>> +> Default: `gnttab=max_ver:2,transitive` >>>> + >>>> +Control various aspects of the grant table behaviour available to guests. >>>> + >>>> +* `max_ver` Select the maximum grant table version to offer to guests. Valid >>>> +version are 1 and 2. >>>> +* `transitive` Permit or disallow the use of transitive grants. Note that the >>>> +use of grant table v2 without transitive grants is an ABI breakage from the >>>> +guests point of view. >>> Btw, with the need to use v2 on huge systems I'm no longer >>> convinced providing an option to disable it is a good idea. >> >> "necessary to support larger systems" is not a valid reason to prevent >> smaller systems having the option to reduce their hypervisor attack surface. > > Well, yes, one can view it that way. Two questions then, though: > 1) If at some point someone comes up with a much better quality > v3, how will your option syntax fit that (i.e. to exclude just v2)? > 2) Switching between versions (post-migration) requires extra code > in guests, albeit perhaps not very much. Is it wise to require OSes > to be capable of switching back and forth? BTW: the documentation of "transitive" could be better: does specifying "transitive" permit or disallow the use of transitive grants? Juergen
On 08/25/2017 01:31 PM, Jan Beulich wrote: >>>> On 25.08.17 at 14:10, <andrew.cooper3@citrix.com> wrote: >> On 25/08/17 10:57, Jan Beulich wrote: >>>>>> On 24.08.17 at 17:16, <andrew.cooper3@citrix.com> wrote: >>>> On 24/08/17 16:01, Juergen Gross wrote: >>>>> On 24/08/17 16:50, Andrew Cooper wrote: >>>>>> --- a/docs/misc/xen-command-line.markdown >>>>>> +++ b/docs/misc/xen-command-line.markdown >>>>>> @@ -868,6 +868,19 @@ Controls EPT related features. >>>>>> >>>>>> Specify which console gdbstub should use. See **console**. >>>>>> >>>>>> +### gnttab >>>>>> +> `= List of [ max_ver:<integer>, transitive ]` >>>>>> + >>>>>> +> Default: `gnttab=max_ver:2,transitive` >>>>>> + >>>>>> +Control various aspects of the grant table behaviour available to guests. >>>>>> + >>>>>> +* `max_ver` Select the maximum grant table version to offer to guests. Valid >>>>>> +version are 1 and 2. >>>>>> +* `transitive` Permit or disallow the use of transitive grants. Note that the >>>>>> +use of grant table v2 without transitive grants is an ABI breakage from the >>>>>> +guests point of view. >>>>> So shouldn't there be a way for the guest to query the support of >>>>> transient grants? >>>> Ideally yes, but how do you suggest doing this in a compatible way? >>>> >>>> All Xen downstreams which haven't backported the eventual transitive >>>> fixes will have this clobber in place, without any query-ability. >>> That workaround should not be used as an argument to not >>> provide a way to query the capability. It was put in place knowing >>> that it would cause problems for (hypothetical) guests using >>> transitive grants. >> >> I am not objecting to introducing a mechanism if a suitable one can be >> found. >> >> However, the heritage of XSA-226 is a valid reason to not block this >> patch because a mechanism isn't present. > > Code submission deadline for 4.10 isn't very far away; we shouldn't > ship a major version with a partial workaround. I'd say we shouldn't ship a major version with a risky, unused feature on by default. -George
On 25/08/17 18:21, George Dunlap wrote: > On 08/25/2017 01:31 PM, Jan Beulich wrote: >>>>> On 25.08.17 at 14:10, <andrew.cooper3@citrix.com> wrote: >>> On 25/08/17 10:57, Jan Beulich wrote: >>>>>>> On 24.08.17 at 17:16, <andrew.cooper3@citrix.com> wrote: >>>>> On 24/08/17 16:01, Juergen Gross wrote: >>>>>> On 24/08/17 16:50, Andrew Cooper wrote: >>>>>>> --- a/docs/misc/xen-command-line.markdown >>>>>>> +++ b/docs/misc/xen-command-line.markdown >>>>>>> @@ -868,6 +868,19 @@ Controls EPT related features. >>>>>>> >>>>>>> Specify which console gdbstub should use. See **console**. >>>>>>> >>>>>>> +### gnttab >>>>>>> +> `= List of [ max_ver:<integer>, transitive ]` >>>>>>> + >>>>>>> +> Default: `gnttab=max_ver:2,transitive` >>>>>>> + >>>>>>> +Control various aspects of the grant table behaviour available to guests. >>>>>>> + >>>>>>> +* `max_ver` Select the maximum grant table version to offer to guests. Valid >>>>>>> +version are 1 and 2. >>>>>>> +* `transitive` Permit or disallow the use of transitive grants. Note that the >>>>>>> +use of grant table v2 without transitive grants is an ABI breakage from the >>>>>>> +guests point of view. >>>>>> So shouldn't there be a way for the guest to query the support of >>>>>> transient grants? >>>>> Ideally yes, but how do you suggest doing this in a compatible way? >>>>> >>>>> All Xen downstreams which haven't backported the eventual transitive >>>>> fixes will have this clobber in place, without any query-ability. >>>> That workaround should not be used as an argument to not >>>> provide a way to query the capability. It was put in place knowing >>>> that it would cause problems for (hypothetical) guests using >>>> transitive grants. >>> >>> I am not objecting to introducing a mechanism if a suitable one can be >>> found. >>> >>> However, the heritage of XSA-226 is a valid reason to not block this >>> patch because a mechanism isn't present. >> >> Code submission deadline for 4.10 isn't very far away; we shouldn't >> ship a major version with a partial workaround. > > I'd say we shouldn't ship a major version with a risky, unused feature > on by default. You are aware that this "unused feature" is part of the public interface since about 8 years or so? Juergen
On 25/08/17 18:36, Juergen Gross wrote: > On 25/08/17 18:21, George Dunlap wrote: >> On 08/25/2017 01:31 PM, Jan Beulich wrote: >>>>>> On 25.08.17 at 14:10, <andrew.cooper3@citrix.com> wrote: >>>> On 25/08/17 10:57, Jan Beulich wrote: >>>>>>>> On 24.08.17 at 17:16, <andrew.cooper3@citrix.com> wrote: >>>>>> On 24/08/17 16:01, Juergen Gross wrote: >>>>>>> On 24/08/17 16:50, Andrew Cooper wrote: >>>>>>>> --- a/docs/misc/xen-command-line.markdown >>>>>>>> +++ b/docs/misc/xen-command-line.markdown >>>>>>>> @@ -868,6 +868,19 @@ Controls EPT related features. >>>>>>>> >>>>>>>> Specify which console gdbstub should use. See **console**. >>>>>>>> >>>>>>>> +### gnttab >>>>>>>> +> `= List of [ max_ver:<integer>, transitive ]` >>>>>>>> + >>>>>>>> +> Default: `gnttab=max_ver:2,transitive` >>>>>>>> + >>>>>>>> +Control various aspects of the grant table behaviour available to guests. >>>>>>>> + >>>>>>>> +* `max_ver` Select the maximum grant table version to offer to guests. Valid >>>>>>>> +version are 1 and 2. >>>>>>>> +* `transitive` Permit or disallow the use of transitive grants. Note that the >>>>>>>> +use of grant table v2 without transitive grants is an ABI breakage from the >>>>>>>> +guests point of view. >>>>>>> So shouldn't there be a way for the guest to query the support of >>>>>>> transient grants? >>>>>> Ideally yes, but how do you suggest doing this in a compatible way? >>>>>> >>>>>> All Xen downstreams which haven't backported the eventual transitive >>>>>> fixes will have this clobber in place, without any query-ability. >>>>> That workaround should not be used as an argument to not >>>>> provide a way to query the capability. It was put in place knowing >>>>> that it would cause problems for (hypothetical) guests using >>>>> transitive grants. >>>> I am not objecting to introducing a mechanism if a suitable one can be >>>> found. >>>> >>>> However, the heritage of XSA-226 is a valid reason to not block this >>>> patch because a mechanism isn't present. >>> Code submission deadline for 4.10 isn't very far away; we shouldn't >>> ship a major version with a partial workaround. >> I'd say we shouldn't ship a major version with a risky, unused feature >> on by default. > You are aware that this "unused feature" is part of the public interface > since about 8 years or so? Like so many things from Xen's past, it shouldn't have gone in in this state. gnttab v2 in particular suffers massively from second system syndrome, and is far more complicated for both Xen and guests to use than v1. The fact that no up-to-date guests use v2 is also a telling datapoint. As for this command line patch, I am going to insist on it being a 4.10 blocker, and all 4.$X.$N+1 releases. We shipped XSA-226 with this workaround, and several downstreams *really* *are* using it. Even with hindsight, shipping this patch was the correct thing to do. There wasn't a functioning fix for XSA-226 until a week after the embargo, when OSSTest discovered that the proposed proper fix for transitive grants broke all grant copy operations for 32bit guests. I'm also quite disappointed with how the post-embargo handling of XSA-226 has gone. Choosing to disable a feature (as opposed to fixing it) is always a tough decision, but the timeline speaks for itself. The decision wasn't taken lightly. For full transparency, the decision was taken by me (as the only person on the security team not on holidy, or working part time), 1 week into the 2 week embargo period, after having worked a full weekend (as well as some Amazon engineers, hence the special thanks in the CREDITS), trying to fix crash after crash to do with transitive grant race conditions, only to find that once the crashes were sorted, a reference count leak was still present. There was no obvious fix or end in sight, and the currently embargoed information was took what was believed to be a theoretical issue and turn it into a very real and easy-to-exploit heap corruption issue. The second part of the XSA-226 fix was only completed on the morning of public embargo, and still, testing eventually showed that the first part was actually buggy. At the moment, all of our downstreams which followed the embargoed advise will be using these command line options to mitigate the vulnerability. The fact that this patch wasn't committed to the stable trees is bad, because it will cause our downstreams to regress move to a newer version of Xen and the command line options they set up no longer have any effect. With a believed transitive fix now in place, there is certainly room for arguing over the default to avoid the ABI breakage. However, all downstreams who have mitigated the vulnerability or reduce their hypervisor attack surface by setting gnttab=max_ver:1 or gnttab=no-transitive need this to keep on working going forwards. ~Andrew
>>> On 25.08.17 at 20:43, <andrew.cooper3@citrix.com> wrote: > At the moment, all of our downstreams which followed the embargoed > advise will be using these command line options to mitigate the > vulnerability. The fact that this patch wasn't committed to the stable > trees is bad, because it will cause our downstreams to regress move to a > newer version of Xen and the command line options they set up no longer > have any effect. No-one should ever re-base onto a new major or stable version by dropping all locally carried patches. Jan
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown index 4002eab..78c7b51 100644 --- a/docs/misc/xen-command-line.markdown +++ b/docs/misc/xen-command-line.markdown @@ -868,6 +868,19 @@ Controls EPT related features. Specify which console gdbstub should use. See **console**. +### gnttab +> `= List of [ max_ver:<integer>, transitive ]` + +> Default: `gnttab=max_ver:2,transitive` + +Control various aspects of the grant table behaviour available to guests. + +* `max_ver` Select the maximum grant table version to offer to guests. Valid +version are 1 and 2. +* `transitive` Permit or disallow the use of transitive grants. Note that the +use of grant table v2 without transitive grants is an ABI breakage from the +guests point of view. + ### gnttab\_max\_frames > `= <integer>` diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index 36895aa..f9c313d 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -50,6 +50,42 @@ integer_param("gnttab_max_nr_frames", max_nr_grant_frames); unsigned int __read_mostly max_grant_frames; integer_param("gnttab_max_frames", max_grant_frames); +static unsigned int __read_mostly opt_gnttab_max_version = 2; +static bool __read_mostly opt_transitive_grants = true; + +static void __init parse_gnttab(char *s) +{ + char *ss; + + do { + ss = strchr(s, ','); + if ( ss ) + *ss = '\0'; + + if ( !strncmp(s, "max_ver:", 8) ) + { + long ver = simple_strtol(s + 8, NULL, 10); + + if ( ver >= 1 && ver <= 2 ) + opt_gnttab_max_version = ver; + } + else + { + bool val = !!strncmp(s, "no-", 3); + + if ( !val ) + s += 3; + + if ( !strcmp(s, "transitive") ) + opt_transitive_grants = val; + } + + s = ss + 1; + } while ( ss ); +} + +custom_param("gnttab", parse_gnttab); + /* The maximum number of grant mappings is defined as a multiplier of the * maximum number of grant table entries. This defines the multiplier used. * Pretty arbitrary. [POLICY] @@ -2505,7 +2541,8 @@ static int gnttab_copy_claim_buf(const struct gnttab_copy *op, current->domain->domain_id, buf->read_only, &buf->frame, &buf->page, - &buf->ptr.offset, &buf->len, true); + &buf->ptr.offset, &buf->len, + opt_transitive_grants); if ( rc != GNTST_okay ) goto out; buf->ptr.u.ref = ptr->u.ref; @@ -3237,7 +3274,10 @@ do_grant_table_op( break; case GNTTABOP_set_version: - rc = gnttab_set_version(guest_handle_cast(uop, gnttab_set_version_t)); + if ( opt_gnttab_max_version == 1 ) + rc = -ENOSYS; /* Behave as before set_version was introduced. */ + else + rc = gnttab_set_version(guest_handle_cast(uop, gnttab_set_version_t)); break; case GNTTABOP_get_status_frames:
This patch was originally a workaround for XSA-226. Since then, transitive grants are believed to be functioning properly, and the defaults have changed appropriately. However, for those people who chose to use the workaround (especially from an attack surface mitigation point of view), retain the ability for the host administrator to choose. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: George Dunlap <George.Dunlap@eu.citrix.com> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Tim Deegan <tim@xen.org> CC: Wei Liu <wei.liu2@citrix.com> --- docs/misc/xen-command-line.markdown | 13 +++++++++++ xen/common/grant_table.c | 44 +++++++++++++++++++++++++++++++++++-- 2 files changed, 55 insertions(+), 2 deletions(-)