[PATCH 0/2] tools/libxc: Fixes to cpuid logic

[PATCH 0/2] tools/libxc: Fixes to cpuid logic

[Andrew Cooper]

Andrew Cooper (2):
  tools/libxc: Fix issues with libxc and Xen having different featureset lengths
  tools/libxc: Fix error handling in get_cpuid_domain_info()

 tools/libxc/xc_cpuid_x86.c | 56 ++++++++++++++++++++++++++++------------------
 tools/misc/xen-cpuid.c     |  2 +-
 2 files changed, 35 insertions(+), 23 deletions(-)

-- 
2.1.4


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

[PATCH 1/2] tools/libxc: Fix issues with libxc and Xen having different featureset lengths

[Andrew Cooper]

In almost all cases, Xen and libxc will agree on the featureset length,
because they are built from the same source.

However, there are circumstances (e.g. security hotfixes) where the featureset
gets longer and dom0 will, after installing updates, be running with an old
Xen but new libxc.  Despite writing the code with this scenario in mind, there
were some bugs.

First, xen-cpuid's get_featureset() erroneously allocates a buffer based on
Xen's featureset length, but records libxc's length, which is longer.

The hypercall bounce buffer code reads/writes the recorded length, which is
beyond the end of the allocated object, and a later free() encounters corrupt
heap metadata.  Fix this by recording the same length that we allocate.

Secondly, get_cpuid_domain_info() has a related bug when the passed-in
featureset is a different length to libxc's.

A large amount of the libxc cpuid functionality depends on info->featureset
being as long as expected, and it is allocated appropriately.  However, in the
case that a shorter external featureset is passed in, the logic to check for
trailing nonzero bits may read off the end of it.  Rework the logic to use the
correct upper bound.

In addition, leave a comment next to the fields in struct cpuid_domain_info
explaining the relationship between the various lengths, and how to cope with
different lengths.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 tools/libxc/xc_cpuid_x86.c | 23 +++++++++++++++++++++--
 tools/misc/xen-cpuid.c     |  2 +-
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 9e47fc8..13862b9 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -239,6 +239,18 @@ struct cpuid_domain_info
     bool hvm;
     uint64_t xfeature_mask;
 
+    /*
+     * Careful with featureset lengths.
+     *
+     * Code in this file requires featureset to have at least
+     * xc_get_cpu_featureset_size() entries.  This is a libxc compiletime
+     * constant.
+     *
+     * The featureset length used by the hypervisor may be different.  If the
+     * hypervisor version is longer, XEN_SYSCTL_get_cpu_featureset will fail
+     * with -ENOBUFS, and libxc really does need rebuilding.  If the
+     * hypervisor version is shorter, it is safe to zero-extend.
+     */
     uint32_t *featureset;
     unsigned int nr_features;
 
@@ -309,11 +321,18 @@ static int get_cpuid_domain_info(xc_interface *xch, uint32_t domid,
 
     if ( featureset )
     {
+        /*
+         * The user supplied featureset may be shorter or longer than
+         * host_nr_features.  Shorter is fine, and we will zero-extend.
+         * Longer is fine, so long as it only padded with zeros.
+         */
+        unsigned int fslen = min(host_nr_features, nr_features);
+
         memcpy(info->featureset, featureset,
-               min(host_nr_features, nr_features) * sizeof(*info->featureset));
+               fslen * sizeof(*info->featureset));
 
         /* Check for truncated set bits. */
-        for ( i = nr_features; i < host_nr_features; ++i )
+        for ( i = fslen; i < nr_features; ++i )
             if ( featureset[i] != 0 )
                 return -EOPNOTSUPP;
     }
diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index 04b11d7..6e7ca8b 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -231,7 +231,7 @@ static void get_featureset(xc_interface *xch, unsigned int idx)
 {
     struct fsinfo *f = &featuresets[idx];
 
-    f->len = xc_get_cpu_featureset_size();
+    f->len = nr_features;
     f->fs = calloc(nr_features, sizeof(*f->fs));
 
     if ( !f->fs )
-- 
2.1.4


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

[PATCH 2/2] tools/libxc: Fix error handling in get_cpuid_domain_info()

[Andrew Cooper]

get_cpuid_domain_info() has two conflicting return styles - either -error for
local failures, or -1/errno for hypercall failures.  Switch to consistently
use -error.

While fixing the xc_get_cpu_featureset(), take the opportunity to remove the
redundancy and move it to be adjacent to the other featureset handling.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 tools/libxc/xc_cpuid_x86.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 13862b9..098affe 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -336,13 +336,22 @@ static int get_cpuid_domain_info(xc_interface *xch, uint32_t domid,
             if ( featureset[i] != 0 )
                 return -EOPNOTSUPP;
     }
+    else
+    {
+        rc = xc_get_cpu_featureset(xch, (info->hvm
+                                         ? XEN_SYSCTL_cpu_featureset_hvm
+                                         : XEN_SYSCTL_cpu_featureset_pv),
+                                   &host_nr_features, info->featureset);
+        if ( rc )
+            return -errno;
+    }
 
     /* Get xstate information. */
     domctl.cmd = XEN_DOMCTL_getvcpuextstate;
     domctl.domain = domid;
     rc = do_domctl(xch, &domctl);
     if ( rc )
-        return rc;
+        return -errno;
 
     info->xfeature_mask = domctl.u.vcpuextstate.xfeature_mask;
 
@@ -352,23 +361,15 @@ static int get_cpuid_domain_info(xc_interface *xch, uint32_t domid,
 
         rc = xc_hvm_param_get(xch, domid, HVM_PARAM_PAE_ENABLED, &val);
         if ( rc )
-            return rc;
+            return -errno;
 
         info->pae = !!val;
 
         rc = xc_hvm_param_get(xch, domid, HVM_PARAM_NESTEDHVM, &val);
         if ( rc )
-            return rc;
+            return -errno;
 
         info->nestedhvm = !!val;
-
-        if ( !featureset )
-        {
-            rc = xc_get_cpu_featureset(xch, XEN_SYSCTL_cpu_featureset_hvm,
-                                       &host_nr_features, info->featureset);
-            if ( rc )
-                return rc;
-        }
     }
     else
     {
@@ -376,17 +377,9 @@ static int get_cpuid_domain_info(xc_interface *xch, uint32_t domid,
 
         rc = xc_domain_get_guest_width(xch, domid, &width);
         if ( rc )
-            return rc;
+            return -errno;
 
         info->pv64 = (width == 8);
-
-        if ( !featureset )
-        {
-            rc = xc_get_cpu_featureset(xch, XEN_SYSCTL_cpu_featureset_pv,
-                                       &host_nr_features, info->featureset);
-            if ( rc )
-                return rc;
-        }
     }
 
     return 0;
-- 
2.1.4


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

Re: [PATCH 1/2] tools/libxc: Fix issues with libxc and Xen having different featureset lengths

[Jan Beulich]

>>> On 29.11.18 at 20:20, <andrew.cooper3@citrix.com> wrote:
> In almost all cases, Xen and libxc will agree on the featureset length,
> because they are built from the same source.
> 
> However, there are circumstances (e.g. security hotfixes) where the featureset
> gets longer and dom0 will, after installing updates, be running with an old
> Xen but new libxc.  Despite writing the code with this scenario in mind, there
> were some bugs.
> 
> First, xen-cpuid's get_featureset() erroneously allocates a buffer based on
> Xen's featureset length, but records libxc's length, which is longer.

"... which may be longer", seeing that nr_features gets initialized from
xc_get_cpu_featureset_size()'s return value, and its subsequent
updating (through xc_get_cpu_featureset()) is only done in certain
cases.

> The hypercall bounce buffer code reads/writes the recorded length, which is
> beyond the end of the allocated object, and a later free() encounters corrupt
> heap metadata.  Fix this by recording the same length that we allocate.
> 
> Secondly, get_cpuid_domain_info() has a related bug when the passed-in
> featureset is a different length to libxc's.
> 
> A large amount of the libxc cpuid functionality depends on info->featureset
> being as long as expected, and it is allocated appropriately.  However, in the
> case that a shorter external featureset is passed in, the logic to check for
> trailing nonzero bits may read off the end of it.  Rework the logic to use the
> correct upper bound.
> 
> In addition, leave a comment next to the fields in struct cpuid_domain_info
> explaining the relationship between the various lengths, and how to cope with
> different lengths.
> 
> 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

Re: [PATCH 2/2] tools/libxc: Fix error handling in get_cpuid_domain_info()

[Jan Beulich]

>>> On 29.11.18 at 20:20, <andrew.cooper3@citrix.com> wrote:
> get_cpuid_domain_info() has two conflicting return styles - either -error for
> local failures, or -1/errno for hypercall failures.  Switch to consistently
> use -error.
> 
> While fixing the xc_get_cpu_featureset(), take the opportunity to remove the
> redundancy and move it to be adjacent to the other featureset handling.

Having noticed the redundancy before, I was assuming this was done
to prevent altering host_nr_features early, just in case future code
additions would want to use its original value. The way it is now, the
code folding looks correct.

> 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

Re: [PATCH 1/2] tools/libxc: Fix issues with libxc and Xen having different featureset lengths

[Andrew Cooper]

On 30/11/2018 09:18, Jan Beulich wrote:
>>>> On 29.11.18 at 20:20, <andrew.cooper3@citrix.com> wrote:
>> In almost all cases, Xen and libxc will agree on the featureset length,
>> because they are built from the same source.
>>
>> However, there are circumstances (e.g. security hotfixes) where the featureset
>> gets longer and dom0 will, after installing updates, be running with an old
>> Xen but new libxc.  Despite writing the code with this scenario in mind, there
>> were some bugs.
>>
>> First, xen-cpuid's get_featureset() erroneously allocates a buffer based on
>> Xen's featureset length, but records libxc's length, which is longer.
> "... which may be longer", seeing that nr_features gets initialized from
> xc_get_cpu_featureset_size()'s return value, and its subsequent
> updating (through xc_get_cpu_featureset()) is only done in certain
> cases.

Ah yes - very true.  I'll also tweak the following sentence to start
with "In this situation, the hypercall bounce ..."

>
>> The hypercall bounce buffer code reads/writes the recorded length, which is
>> beyond the end of the allocated object, and a later free() encounters corrupt
>> heap metadata.  Fix this by recording the same length that we allocate.
>>
>> Secondly, get_cpuid_domain_info() has a related bug when the passed-in
>> featureset is a different length to libxc's.
>>
>> A large amount of the libxc cpuid functionality depends on info->featureset
>> being as long as expected, and it is allocated appropriately.  However, in the
>> case that a shorter external featureset is passed in, the logic to check for
>> trailing nonzero bits may read off the end of it.  Rework the logic to use the
>> correct upper bound.
>>
>> In addition, leave a comment next to the fields in struct cpuid_domain_info
>> explaining the relationship between the various lengths, and how to cope with
>> different lengths.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks,

~Andrew

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

Re: [PATCH 2/2] tools/libxc: Fix error handling in get_cpuid_domain_info()

[Andrew Cooper]

On 30/11/2018 09:23, Jan Beulich wrote:
>>>> On 29.11.18 at 20:20, <andrew.cooper3@citrix.com> wrote:
>> get_cpuid_domain_info() has two conflicting return styles - either -error for
>> local failures, or -1/errno for hypercall failures.  Switch to consistently
>> use -error.
>>
>> While fixing the xc_get_cpu_featureset(), take the opportunity to remove the
>> redundancy and move it to be adjacent to the other featureset handling.
> Having noticed the redundancy before, I was assuming this was done
> to prevent altering host_nr_features early, just in case future code
> additions would want to use its original value. The way it is now, the
> code folding looks correct.

I don't recall a deliberate intention to do that, but it was a long time
ago now.

>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

~Andrew

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

Re: [PATCH 0/2] tools/libxc: Fixes to cpuid logic

[Wei Liu]

On Thu, Nov 29, 2018 at 07:20:00PM +0000, Andrew Cooper wrote:
> Andrew Cooper (2):
>   tools/libxc: Fix issues with libxc and Xen having different featureset lengths
>   tools/libxc: Fix error handling in get_cpuid_domain_info()

With Jan's comments addressed:

Acked-by: Wei Liu <wei.liu2@citrix.com>

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