[Xen-devel] [PATCH 0/6] misc hardening and some cleanup

[Xen-devel] [PATCH 0/6] misc hardening and some cleanup

[Jan Beulich]

Ilja has reported a couple of issues which were on the boundary of
needing an XSA, due to some vagueness of the statements resulting
from XSA-77. The first 3 patches here address these reports, after
having settled within the Security Team that we can't find anyone /
anything actually being potentially affected in reality.

In the course of auditing for possible actual issues resulting from
the missing overflow check addressed by patch 3, a few more cleanup
opportunities were noticed, which the remaining 3 patches take care
of.

1: EFI: re-check {get,set}-variable name strings after copying in
2: EFI: don't leak heap contents through XEN_EFI_get_next_variable_name
3: xmalloc: guard against integer overflow
4: Arm/GICv2: don't needlessly use xzalloc_bytes()
5: sysctl: use xmalloc_array() for XEN_SYSCTL_page_offline_op
6: domctl/vNUMA: avoid arithmetic overflow

Jan

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

[Xen-devel] [PATCH 1/6] EFI: re-check {get, set}-variable name strings after copying in

[Jan Beulich]

A malicious guest given permission to invoke XENPF_efi_runtime_call may
play with the strings underneath Xen sizing them and copying them in.
Guard against this by re-checking the copyied in data for consistency
with the initial sizing. At the same time also check that the actual
copy-in is in fact successful, and switch to the lighter weight non-
checking flavor of the function.

Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
---
Note that this collides with XSA-257's patch 6.

--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -281,16 +281,6 @@ static int __init wstrncmp(const CHAR16
     return n ? *s1 - *s2 : 0;
 }
 
-static const CHAR16 *__init wmemchr(const CHAR16 *s, CHAR16 c, UINTN n)
-{
-    while ( n && *s != c )
-    {
-        --n;
-        ++s;
-    }
-    return n ? s : NULL;
-}
-
 static CHAR16 *__init s2w(union string *str)
 {
     const char *s = str->s;
--- a/xen/common/efi/efi.h
+++ b/xen/common/efi/efi.h
@@ -39,3 +39,5 @@ extern UINT64 efi_boot_max_var_store_siz
 
 extern UINT64 efi_apple_properties_addr;
 extern UINTN efi_apple_properties_len;
+
+const CHAR16 *wmemchr(const CHAR16 *s, CHAR16 c, UINTN n);
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -194,7 +194,18 @@ void efi_reset_system(bool warm)
 }
 
 #endif /* CONFIG_ARM */
-#endif
+
+const CHAR16 *wmemchr(const CHAR16 *s, CHAR16 c, UINTN n)
+{
+    while ( n && *s != c )
+    {
+        --n;
+        ++s;
+    }
+    return n ? s : NULL;
+}
+
+#endif /* COMPAT */
 
 #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
 int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
@@ -465,7 +476,12 @@ int efi_runtime_call(struct xenpf_efi_ru
         name = xmalloc_array(CHAR16, ++len);
         if ( !name )
            return -ENOMEM;
-        __copy_from_guest(name, op->u.get_variable.name, len);
+        if ( __copy_from_guest(name, op->u.get_variable.name, len) ||
+             wmemchr(name, 0, len) != name + len - 1 )
+        {
+            xfree(name);
+            return -EIO;
+        }
 
         size = op->u.get_variable.size;
         if ( size )
@@ -513,7 +529,12 @@ int efi_runtime_call(struct xenpf_efi_ru
         name = xmalloc_array(CHAR16, ++len);
         if ( !name )
            return -ENOMEM;
-        __copy_from_guest(name, op->u.set_variable.name, len);
+        if ( __copy_from_guest(name, op->u.set_variable.name, len) ||
+             wmemchr(name, 0, len) != name + len - 1 )
+        {
+            xfree(name);
+            return -EIO;
+        }
 
         data = xmalloc_bytes(op->u.set_variable.size);
         if ( !data )


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

[Xen-devel] [PATCH 2/6] EFI: don't leak heap contents through XEN_EFI_get_next_variable_name

[Jan Beulich]

Commit 1f4eb9d27d0e ("EFI: fix getting EFI variable list on some
systems") switched to using the caller provided size for the copy-out
without making sure the copied buffer is properly scrubbed.

Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>

--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -571,7 +571,7 @@ int efi_runtime_call(struct xenpf_efi_ru
             return -EINVAL;
 
         size = op->u.get_next_variable_name.size;
-        name.raw = xmalloc_bytes(size);
+        name.raw = xzalloc_bytes(size);
         if ( !name.raw )
             return -ENOMEM;
         if ( copy_from_guest(name.raw, op->u.get_next_variable_name.name,


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

[Xen-devel] [PATCH 3/6] xmalloc: guard against integer overflow

[Jan Beulich]

There are hypercall handling paths (EFI ones are what this was found
with) needing to allocate buffers of a caller specified size. This is
generally fine, as our page allocator enforces an upper bound on all
allocations. However, certain extremely large sizes could, when adding
in allocator overhead, result in an apparently tiny allocation size,
which would typically result in either a successful allocation, but a
severe buffer overrun when using that memory block, or in a crash right
in the allocator code.

Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>

--- a/xen/common/xmalloc_tlsf.c
+++ b/xen/common/xmalloc_tlsf.c
@@ -378,7 +378,17 @@ void *xmem_pool_alloc(unsigned long size
     int fl, sl;
     unsigned long tmp_size;
 
-    size = (size < MIN_BLOCK_SIZE) ? MIN_BLOCK_SIZE : ROUNDUP_SIZE(size);
+    if ( size < MIN_BLOCK_SIZE )
+        size = MIN_BLOCK_SIZE;
+    else
+    {
+        tmp_size = ROUNDUP_SIZE(size);
+        /* Guard against overflow. */
+        if ( tmp_size < size )
+            return NULL;
+        size = tmp_size;
+    }
+
     /* Rounding up the requested size and calculating fl and sl */
 
     spin_lock(&pool->lock);
@@ -594,6 +604,10 @@ void *_xmalloc(unsigned long size, unsig
         align = MEM_ALIGN;
     size += align - MEM_ALIGN;
 
+    /* Guard against overflow. */
+    if ( size < align - MEM_ALIGN )
+        return NULL;
+
     if ( !xenpool )
         tlsf_init();
 
@@ -646,6 +660,10 @@ void *_xrealloc(void *ptr, unsigned long
         unsigned long tmp_size = size + align - MEM_ALIGN;
         const struct bhdr *b;
 
+        /* Guard against overflow. */
+        if ( tmp_size < size )
+            return NULL;
+
         if ( tmp_size < PAGE_SIZE )
             tmp_size = (tmp_size < MIN_BLOCK_SIZE) ? MIN_BLOCK_SIZE :
                 ROUNDUP_SIZE(tmp_size);


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

[Xen-devel] [PATCH 4/6] Arm/GICv2: don't needlessly use xzalloc_bytes()

[Jan Beulich]

... when plain xzalloc() (which is more type safe) does.

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

--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -969,7 +969,7 @@ static void gicv2_add_v2m_frame_to_list(
               nr_spis, V2M_MAX_SPI - V2M_MIN_SPI + 1);
 
     /* Allocate an entry to record new v2m frame information. */
-    v2m_data = xzalloc_bytes(sizeof(struct v2m_data));
+    v2m_data = xzalloc(struct v2m_data);
     if ( !v2m_data )
         panic("GICv2: Cannot allocate memory for v2m frame\n");
 

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

Re: [Xen-devel] [PATCH 4/6] Arm/GICv2: don't needlessly use xzalloc_bytes()

[Julien Grall]

Hi Jan,

On 05/02/2020 13:16, Jan Beulich wrote:
> ... when plain xzalloc() (which is more type safe) does.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Julien Grall <julien@xen.org>

Cheers,

> 
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -969,7 +969,7 @@ static void gicv2_add_v2m_frame_to_list(
>                 nr_spis, V2M_MAX_SPI - V2M_MIN_SPI + 1);
>   
>       /* Allocate an entry to record new v2m frame information. */
> -    v2m_data = xzalloc_bytes(sizeof(struct v2m_data));
> +    v2m_data = xzalloc(struct v2m_data);
>       if ( !v2m_data )
>           panic("GICv2: Cannot allocate memory for v2m frame\n");
>   
> 

-- 
Julien Grall

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

[Xen-devel] [PATCH 4/6] sysctl: use xmalloc_array() for XEN_SYSCTL_page_offline_op

[Jan Beulich]

This is more robust than the raw xmalloc_bytes().

Also add a sanity check on the input page range.

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

--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -187,13 +187,17 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xe
         uint32_t *status, *ptr;
         mfn_t mfn;
 
+        ret = -EINVAL;
+        if ( op->u.page_offline.end < op->u.page_offline.start )
+            break;
+
         ret = xsm_page_offline(XSM_HOOK, op->u.page_offline.cmd);
         if ( ret )
             break;
 
-        ptr = status = xmalloc_bytes( sizeof(uint32_t) *
-                                (op->u.page_offline.end -
-                                  op->u.page_offline.start + 1));
+        ptr = status = xmalloc_array(uint32_t,
+                                     (op->u.page_offline.end -
+                                      op->u.page_offline.start + 1));
         if ( !status )
         {
             dprintk(XENLOG_WARNING, "Out of memory for page offline op\n");


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

Re: [Xen-devel] [PATCH 4/6] sysctl: use xmalloc_array() for XEN_SYSCTL_page_offline_op

[Julien Grall]

Hi Jan,

On 05/02/2020 13:16, Jan Beulich wrote:
> This is more robust than the raw xmalloc_bytes().
> 
> Also add a sanity check on the input page range.

It feels to me that the commit message/title should focus on the sanity 
check. The xmalloc_array() is just a cleanup is "less important".

Argually the clean-up should be in a separate patch but I can appreciate 
they are somewhat related.

> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -187,13 +187,17 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xe
>           uint32_t *status, *ptr;
>           mfn_t mfn;
>   
> +        ret = -EINVAL;
> +        if ( op->u.page_offline.end < op->u.page_offline.start )
> +            break;
> +
>           ret = xsm_page_offline(XSM_HOOK, op->u.page_offline.cmd);
>           if ( ret )
>               break;
>   
> -        ptr = status = xmalloc_bytes( sizeof(uint32_t) *
> -                                (op->u.page_offline.end -
> -                                  op->u.page_offline.start + 1));
> +        ptr = status = xmalloc_array(uint32_t,
> +                                     (op->u.page_offline.end -
> +                                      op->u.page_offline.start + 1));
>           if ( !status )
>           {
>               dprintk(XENLOG_WARNING, "Out of memory for page offline op\n");
> 

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 4/6] sysctl: use xmalloc_array() for XEN_SYSCTL_page_offline_op

[Jan Beulich]

On 05.02.2020 15:34, Julien Grall wrote:
> On 05/02/2020 13:16, Jan Beulich wrote:
>> This is more robust than the raw xmalloc_bytes().
>>
>> Also add a sanity check on the input page range.
> 
> It feels to me that the commit message/title should focus on the sanity 
> check. The xmalloc_array() is just a cleanup is "less important".

But it not being there would generally just result in -ENOMEM
due to the xmalloc_...() failing (leaving aside overflow not
accounted for in the old code), which by the new check just
gets changed into the more applicable -EINVAL. I view the
changed called out in the title as more important.

Jan

>> --- a/xen/common/sysctl.c
>> +++ b/xen/common/sysctl.c
>> @@ -187,13 +187,17 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xe
>>           uint32_t *status, *ptr;
>>           mfn_t mfn;
>>   
>> +        ret = -EINVAL;
>> +        if ( op->u.page_offline.end < op->u.page_offline.start )
>> +            break;
>> +
>>           ret = xsm_page_offline(XSM_HOOK, op->u.page_offline.cmd);
>>           if ( ret )
>>               break;
>>   
>> -        ptr = status = xmalloc_bytes( sizeof(uint32_t) *
>> -                                (op->u.page_offline.end -
>> -                                  op->u.page_offline.start + 1));
>> +        ptr = status = xmalloc_array(uint32_t,
>> +                                     (op->u.page_offline.end -
>> +                                      op->u.page_offline.start + 1));
>>           if ( !status )
>>           {
>>               dprintk(XENLOG_WARNING, "Out of memory for page offline op\n");
>>
> 


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

Re: [Xen-devel] [PATCH 4/6] sysctl: use xmalloc_array() for XEN_SYSCTL_page_offline_op

[Julien Grall]

Hi Jan,

On 05/02/2020 16:38, Jan Beulich wrote:
> On 05.02.2020 15:34, Julien Grall wrote:
>> On 05/02/2020 13:16, Jan Beulich wrote:
>>> This is more robust than the raw xmalloc_bytes().
>>>
>>> Also add a sanity check on the input page range.
>>
>> It feels to me that the commit message/title should focus on the sanity
>> check. The xmalloc_array() is just a cleanup is "less important".
> 
> But it not being there would generally just result in -ENOMEM
> due to the xmalloc_...() failing (leaving aside overflow not
> accounted for in the old code), which by the new check just
> gets changed into the more applicable -EINVAL. I view the
> changed called out in the title as more important.

None of the commit message really explain this. So the sanity check did 
feel more important.

You probably want to reword the commit message to explain why the sanity 
check is added (i.e ENOMEM vs EINVAL).

Cheers,

-- 
Julien Grall

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

[Xen-devel] [PATCH 6/6] domctl/vNUMA: avoid arithmetic overflow

[Jan Beulich]

Checking the result of a multiplication against a certain limit has no
sufficient implication on the original value's range. In the case here
it is in particular problematic that while handling the domctl we do

    if ( copy_from_guest(info->vdistance, uinfo->vdistance,
                         nr_vnodes * nr_vnodes) )
        goto vnuma_fail;

which means copying sizeof(unsigned int) * (nr_vnodes * nr_vnodes)
bytes, and the handling of XENMEM_get_vnumainfo similarly has

        tmp.vdistance = xmalloc_array(unsigned int, dom_vnodes * dom_vnodes);

which means allocating sizeof(unsigned int) * (dom_vnodes * dom_vnodes)
bytes, whereas in then goes on doing this:

        memcpy(tmp.vdistance, d->vnuma->vdistance,
               sizeof(*d->vnuma->vdistance) * dom_vnodes * dom_vnodes);

Note the lack of parentheses in the multiplication expression.

Adjust the overflow check, moving the must-not-be-zero one right next to
it to avoid questions on whether there might be division by zero.

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

--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -263,7 +263,8 @@ static struct vnuma_info *vnuma_alloc(un
      * Check if any of the allocations are bigger than PAGE_SIZE.
      * See XSA-77.
      */
-    if ( nr_vnodes * nr_vnodes > (PAGE_SIZE / sizeof(*vnuma->vdistance)) ||
+    if ( nr_vnodes == 0 ||
+         nr_vnodes > (PAGE_SIZE / sizeof(*vnuma->vdistance) / nr_vnodes) ||
          nr_ranges > (PAGE_SIZE / sizeof(*vnuma->vmemrange)) )
         return ERR_PTR(-EINVAL);
 
@@ -302,7 +303,7 @@ static struct vnuma_info *vnuma_init(con
 
     nr_vnodes = uinfo->nr_vnodes;
 
-    if ( nr_vnodes == 0 || uinfo->nr_vcpus != d->max_vcpus || uinfo->pad != 0 )
+    if ( uinfo->nr_vcpus != d->max_vcpus || uinfo->pad != 0 )
         return ERR_PTR(ret);
 
     info = vnuma_alloc(nr_vnodes, uinfo->nr_vmemranges, d->max_vcpus);


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

Re: [Xen-devel] [PATCH 6/6] domctl/vNUMA: avoid arithmetic overflow

[Wei Liu]

On Wed, Feb 05, 2020 at 02:17:02PM +0100, Jan Beulich wrote:
> Checking the result of a multiplication against a certain limit has no
> sufficient implication on the original value's range. In the case here
> it is in particular problematic that while handling the domctl we do
> 
>     if ( copy_from_guest(info->vdistance, uinfo->vdistance,
>                          nr_vnodes * nr_vnodes) )
>         goto vnuma_fail;
> 
> which means copying sizeof(unsigned int) * (nr_vnodes * nr_vnodes)
> bytes, and the handling of XENMEM_get_vnumainfo similarly has
> 
>         tmp.vdistance = xmalloc_array(unsigned int, dom_vnodes * dom_vnodes);
> 
> which means allocating sizeof(unsigned int) * (dom_vnodes * dom_vnodes)
> bytes, whereas in then goes on doing this:
> 
>         memcpy(tmp.vdistance, d->vnuma->vdistance,
>                sizeof(*d->vnuma->vdistance) * dom_vnodes * dom_vnodes);
> 
> Note the lack of parentheses in the multiplication expression.
> 
> Adjust the overflow check, moving the must-not-be-zero one right next to
> it to avoid questions on whether there might be division by zero.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Wei Liu <wl@xen.org>

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

Re: [Xen-devel] [PATCH 0/6] misc hardening and some cleanup

[Jan Beulich]

On 05.02.2020 14:11, Jan Beulich wrote:
> Ilja has reported a couple of issues which were on the boundary of
> needing an XSA, due to some vagueness of the statements resulting
> from XSA-77. The first 3 patches here address these reports, after
> having settled within the Security Team that we can't find anyone /
> anything actually being potentially affected in reality.
> 
> In the course of auditing for possible actual issues resulting from
> the missing overflow check addressed by patch 3, a few more cleanup
> opportunities were noticed, which the remaining 3 patches take care
> of.
> 
> 1: EFI: re-check {get,set}-variable name strings after copying in
> 2: EFI: don't leak heap contents through XEN_EFI_get_next_variable_name
> 3: xmalloc: guard against integer overflow

Since these three patches have been suitably ack-ed, and since
they also aren't new to the majority of the REST maintainers,
I'm intending to commit them no later than tomorrow, perhaps
even before I leave today. Unless, of course, I hear objections.

Jan

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