[PATCH] common/gnttab: Introduce command line feature controls

[PATCH] common/gnttab: Introduce command line feature controls

[Andrew Cooper]

This patch was originally released as part of XSA-226.  It retains the same
command line syntax (as various downstreams are mitigating XSA-226 using this
mechanism) but the defaults have been updated due to the revised XSA-226
patched, after which transitive grants are believed to functioning
properly.

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>

v2:
 * Rebase over command-line parsing changes.
---
 docs/misc/xen-command-line.markdown | 13 ++++++++++++
 xen/common/grant_table.c            | 42 +++++++++++++++++++++++++++++++++++--
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 9c10d3a..2bf2a8f 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -916,6 +916,19 @@ Controls EPT related features.
 
 Specify which console gdbstub should use. See **console**.
 
+### gnttab
+> `= List of [ max_ver:<integer>, transitive=<bool> ]`
+
+> 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 48c5479..3937af7 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -97,6 +97,40 @@ static unsigned int __read_mostly max_maptrack_frames =
                                                DEFAULT_MAX_MAPTRACK_FRAMES;
 integer_runtime_param("gnttab_max_maptrack_frames", max_maptrack_frames);
 
+static unsigned int __read_mostly opt_gnttab_max_version = 2;
+static bool __read_mostly opt_transitive_grants = true;
+
+static int __init parse_gnttab(const char *s)
+{
+    const char *ss;
+    int val, rc = 0;
+
+    do {
+        ss = strchr(s, ',');
+        if ( !ss )
+            ss = strchr(s, '\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
+                rc = -EINVAL;
+        }
+        else if ( (val = parse_boolean("transitive", s, ss)) >= 0 )
+            opt_transitive_grants = val;
+        else
+            rc = -EINVAL;
+
+        s = ss + 1;
+    } while ( *ss );
+
+    return rc;
+}
+custom_param("gnttab", parse_gnttab);
+
 /*
  * Note that the three values below are effectively part of the ABI, even if
  * we don't need to make them a formal part of it: A guest suspended for
@@ -2674,7 +2708,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;
@@ -3424,7 +3459,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:
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[Jan Beulich]

>>> On 01.02.18 at 15:38, <andrew.cooper3@citrix.com> wrote:
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -916,6 +916,19 @@ Controls EPT related features.
>  
>  Specify which console gdbstub should use. See **console**.
>  
> +### gnttab
> +> `= List of [ max_ver:<integer>, transitive=<bool> ]`

I realize you don't want to change this as people already use it, but
I'd still like to give my usual comment: I'd prefer if we could avoid
introducing further underscore-containing (sub)options. I really don't
understand why everyone does this: Dashes are easier to type on
all keyboards I'm aware of, and there's no need to mimic C identifier
names for command line options.

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -97,6 +97,40 @@ static unsigned int __read_mostly max_maptrack_frames =
>                                                 DEFAULT_MAX_MAPTRACK_FRAMES;
>  integer_runtime_param("gnttab_max_maptrack_frames", max_maptrack_frames);
>  
> +static unsigned int __read_mostly opt_gnttab_max_version = 2;
> +static bool __read_mostly opt_transitive_grants = true;
> +
> +static int __init parse_gnttab(const char *s)
> +{
> +    const char *ss;
> +    int val, rc = 0;
> +
> +    do {
> +        ss = strchr(s, ',');
> +        if ( !ss )
> +            ss = strchr(s, '\0');
> +
> +        if ( !strncmp(s, "max_ver:", 8) )
> +        {
> +            long ver = simple_strtol(s + 8, NULL, 10);

In particular with you already having determined the intended end
of the number, wouldn't it make sense to refuse non-number input,
by checking ss against what simple_strtol() would provide if the
middle parameter wasn't NULL?

> @@ -3424,7 +3459,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;

I can sort of see why you want it this way, but why do you mean to
penalize any guest simply setting the version to 1 regardless of its
current setting (like might e.g. be needed in kexec-like situations)?
Guests may assume the availability of set_version by looking at the
Xen version number (whether that's an entirely valid thing to do is
a separate question).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[Andrew Cooper]

On 02/02/18 08:51, Jan Beulich wrote:
>>>> On 01.02.18 at 15:38, <andrew.cooper3@citrix.com> wrote:
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -916,6 +916,19 @@ Controls EPT related features.
>>  
>>  Specify which console gdbstub should use. See **console**.
>>  
>> +### gnttab
>> +> `= List of [ max_ver:<integer>, transitive=<bool> ]`
> I realize you don't want to change this as people already use it, but
> I'd still like to give my usual comment: I'd prefer if we could avoid
> introducing further underscore-containing (sub)options. I really don't
> understand why everyone does this: Dashes are easier to type on
> all keyboards I'm aware of, and there's no need to mimic C identifier
> names for command line options.

I can introduce a max-ver alias if you insist, but dropping max_ver here
is going to break users who took this patch for XSA-226.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[Jan Beulich]

>>> On 05.02.18 at 12:55, <andrew.cooper3@citrix.com> wrote:
> On 02/02/18 08:51, Jan Beulich wrote:
>>>>> On 01.02.18 at 15:38, <andrew.cooper3@citrix.com> wrote:
>>> --- a/docs/misc/xen-command-line.markdown
>>> +++ b/docs/misc/xen-command-line.markdown
>>> @@ -916,6 +916,19 @@ Controls EPT related features.
>>>  
>>>  Specify which console gdbstub should use. See **console**.
>>>  
>>> +### gnttab
>>> +> `= List of [ max_ver:<integer>, transitive=<bool> ]`
>> I realize you don't want to change this as people already use it, but
>> I'd still like to give my usual comment: I'd prefer if we could avoid
>> introducing further underscore-containing (sub)options. I really don't
>> understand why everyone does this: Dashes are easier to type on
>> all keyboards I'm aware of, and there's no need to mimic C identifier
>> names for command line options.
> 
> I can introduce a max-ver alias if you insist, but dropping max_ver here
> is going to break users who took this patch for XSA-226.

Hence the way I've worded my reply - I don't mean to insist on
changing what you have, or the introduction of an alias. I merely
wanted to give the comment, in the hope that it helps to avoid
future underscores in command line option names.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[George Dunlap]

On 02/05/2018 12:56 PM, Jan Beulich wrote:
>>>> On 05.02.18 at 12:55, <andrew.cooper3@citrix.com> wrote:
>> On 02/02/18 08:51, Jan Beulich wrote:
>>>>>> On 01.02.18 at 15:38, <andrew.cooper3@citrix.com> wrote:
>>>> --- a/docs/misc/xen-command-line.markdown
>>>> +++ b/docs/misc/xen-command-line.markdown
>>>> @@ -916,6 +916,19 @@ Controls EPT related features.
>>>>  
>>>>  Specify which console gdbstub should use. See **console**.
>>>>  
>>>> +### gnttab
>>>> +> `= List of [ max_ver:<integer>, transitive=<bool> ]`
>>> I realize you don't want to change this as people already use it, but
>>> I'd still like to give my usual comment: I'd prefer if we could avoid
>>> introducing further underscore-containing (sub)options. I really don't
>>> understand why everyone does this: Dashes are easier to type on
>>> all keyboards I'm aware of, and there's no need to mimic C identifier
>>> names for command line options.
>>
>> I can introduce a max-ver alias if you insist, but dropping max_ver here
>> is going to break users who took this patch for XSA-226.
> 
> Hence the way I've worded my reply - I don't mean to insist on
> changing what you have, or the introduction of an alias. I merely
> wanted to give the comment, in the hope that it helps to avoid
> future underscores in command line option names.

FWIW I often end up looking at other options and name things similarly;
so making the documentation say "max-ver", but accepting both "max-ver"
and "max_ver", would probably make it more likely that future options
would start out as having a dash rather than an underscore.

But it's just a suggestion; I wouldn't push for it.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[Andrew Cooper]

On 05/02/18 13:14, George Dunlap wrote:
> On 02/05/2018 12:56 PM, Jan Beulich wrote:
>>>>> On 05.02.18 at 12:55, <andrew.cooper3@citrix.com> wrote:
>>> On 02/02/18 08:51, Jan Beulich wrote:
>>>>>>> On 01.02.18 at 15:38, <andrew.cooper3@citrix.com> wrote:
>>>>> --- a/docs/misc/xen-command-line.markdown
>>>>> +++ b/docs/misc/xen-command-line.markdown
>>>>> @@ -916,6 +916,19 @@ Controls EPT related features.
>>>>>  
>>>>>  Specify which console gdbstub should use. See **console**.
>>>>>  
>>>>> +### gnttab
>>>>> +> `= List of [ max_ver:<integer>, transitive=<bool> ]`
>>>> I realize you don't want to change this as people already use it, but
>>>> I'd still like to give my usual comment: I'd prefer if we could avoid
>>>> introducing further underscore-containing (sub)options. I really don't
>>>> understand why everyone does this: Dashes are easier to type on
>>>> all keyboards I'm aware of, and there's no need to mimic C identifier
>>>> names for command line options.
>>> I can introduce a max-ver alias if you insist, but dropping max_ver here
>>> is going to break users who took this patch for XSA-226.
>> Hence the way I've worded my reply - I don't mean to insist on
>> changing what you have, or the introduction of an alias. I merely
>> wanted to give the comment, in the hope that it helps to avoid
>> future underscores in command line option names.
> FWIW I often end up looking at other options and name things similarly;
> so making the documentation say "max-ver", but accepting both "max-ver"
> and "max_ver", would probably make it more likely that future options
> would start out as having a dash rather than an underscore.
>
> But it's just a suggestion; I wouldn't push for it.

So how to unblock this?  There are no concrete suggestions, and no
concrete objections to the patch in its current form.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[George Dunlap]

On 02/26/2018 06:02 PM, Andrew Cooper wrote:
> On 05/02/18 13:14, George Dunlap wrote:
>> On 02/05/2018 12:56 PM, Jan Beulich wrote:
>>>>>> On 05.02.18 at 12:55, <andrew.cooper3@citrix.com> wrote:
>>>> On 02/02/18 08:51, Jan Beulich wrote:
>>>>>>>> On 01.02.18 at 15:38, <andrew.cooper3@citrix.com> wrote:
>>>>>> --- a/docs/misc/xen-command-line.markdown
>>>>>> +++ b/docs/misc/xen-command-line.markdown
>>>>>> @@ -916,6 +916,19 @@ Controls EPT related features.
>>>>>>  
>>>>>>  Specify which console gdbstub should use. See **console**.
>>>>>>  
>>>>>> +### gnttab
>>>>>> +> `= List of [ max_ver:<integer>, transitive=<bool> ]`
>>>>> I realize you don't want to change this as people already use it, but
>>>>> I'd still like to give my usual comment: I'd prefer if we could avoid
>>>>> introducing further underscore-containing (sub)options. I really don't
>>>>> understand why everyone does this: Dashes are easier to type on
>>>>> all keyboards I'm aware of, and there's no need to mimic C identifier
>>>>> names for command line options.
>>>> I can introduce a max-ver alias if you insist, but dropping max_ver here
>>>> is going to break users who took this patch for XSA-226.
>>> Hence the way I've worded my reply - I don't mean to insist on
>>> changing what you have, or the introduction of an alias. I merely
>>> wanted to give the comment, in the hope that it helps to avoid
>>> future underscores in command line option names.
>> FWIW I often end up looking at other options and name things similarly;
>> so making the documentation say "max-ver", but accepting both "max-ver"
>> and "max_ver", would probably make it more likely that future options
>> would start out as having a dash rather than an underscore.
>>
>> But it's just a suggestion; I wouldn't push for it.
> 
> So how to unblock this?  There are no concrete suggestions, and no
> concrete objections to the patch in its current form.

From what I understand, both Jan and I are saying, "We won't block the
patch if you re-submit with max_ver but Jan's other comments addressed;
but we would prefer it if max-ver could be used instead."

Given that you want to keep things compatible with the securty patch, I
see two options forward for you:

1. Re-submit it using only max_ver

2. Make it accept both max-ver and max_ver, but only document  max-ver.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[Jan Beulich]

>>> On 27.02.18 at 12:38, <andrew.cooper3@citrix.com> wrote:
> This patch was originally released as part of XSA-226.  It retains the same
> command line syntax (as various downstreams are mitigating XSA-226 using this
> mechanism) but the defaults have been updated due to the revised XSA-226
> patched, after which transitive grants are believed to functioning
> properly.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH] common/gnttab: Introduce command line feature controls

[Andrew Cooper]

This patch was originally released as part of XSA-226.  It retains the same
command line syntax (as various downstreams are mitigating XSA-226 using this
mechanism) but the defaults have been updated due to the revised XSA-226
patched, after which transitive grants are believed to functioning
properly.

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>

v2:
 * Rebase over command-line parsing changes.
v3:
 * Switch to 'max-ver', leaving max_ver as an undocumented alias.
 * Check the end pointer from simple_strtol() to check for trailing characters.
 * Tollerate set_version(1) even if failing set_version(2) with -ENOSYS.
---
 docs/misc/xen-command-line.markdown | 13 ++++++++++++
 xen/common/grant_table.c            | 42 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 8317639..a95195f 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -920,6 +920,19 @@ Controls EPT related features.
 
 Specify which console gdbstub should use. See **console**.
 
+### gnttab
+> `= List of [ max-ver:<integer>, transitive=<bool> ]`
+
+> 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 48c5479..36a182e 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -97,6 +97,41 @@ static unsigned int __read_mostly max_maptrack_frames =
                                                DEFAULT_MAX_MAPTRACK_FRAMES;
 integer_runtime_param("gnttab_max_maptrack_frames", max_maptrack_frames);
 
+static unsigned int __read_mostly opt_gnttab_max_version = 2;
+static bool __read_mostly opt_transitive_grants = true;
+
+static int __init parse_gnttab(const char *s)
+{
+    const char *ss, *e;
+    int val, rc = 0;
+
+    do {
+        ss = strchr(s, ',');
+        if ( !ss )
+            ss = strchr(s, '\0');
+
+        if ( !strncmp(s, "max-ver:", 8) ||
+             !strncmp(s, "max_ver:", 8) ) /* Alias for original XSA-226 patch */
+        {
+            long ver = simple_strtol(s + 8, &e, 10);
+
+            if ( e == ss && ver >= 1 && ver <= 2 )
+                opt_gnttab_max_version = ver;
+            else
+                rc = -EINVAL;
+        }
+        else if ( (val = parse_boolean("transitive", s, ss)) >= 0 )
+            opt_transitive_grants = val;
+        else
+            rc = -EINVAL;
+
+        s = ss + 1;
+    } while ( *ss );
+
+    return rc;
+}
+custom_param("gnttab", parse_gnttab);
+
 /*
  * Note that the three values below are effectively part of the ABI, even if
  * we don't need to make them a formal part of it: A guest suspended for
@@ -2674,7 +2709,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;
@@ -2876,6 +2912,10 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
     if ( op.version != 1 && op.version != 2 )
         goto out;
 
+    res = -ENOSYS;
+    if ( op.version == 2 && opt_gnttab_max_version == 1 )
+        goto out; /* Behave as before set_version was introduced. */
+
     res = 0;
     if ( gt->gt_version == op.version )
         goto out;
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[Jan Beulich]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[Andrew Cooper]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[Juergen Gross]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[George Dunlap]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[Juergen Gross]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[Jan Beulich]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[Jan Beulich]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[Juergen Gross]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[Andrew Cooper]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[Andrew Cooper]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[Jan Beulich]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[Jan Beulich]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[Andrew Cooper]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[Juergen Gross]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[Andrew Cooper]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] common/gnttab: Introduce command line feature controls

[Juergen Gross]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH] common/gnttab: Introduce command line feature controls

[Andrew Cooper]

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.
+
 ### 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:
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel