Xen devel - Nov 2012 - [PATCH] xen: Fix xenctl_cpumap_to_cpumask buffer size check

xenctl_cpumap_to_cpumask incorrectly uses sizeof when checking whether
bits should be masked off from the input cpumap bitmap or not.

Fix by using the correct cpumask buffer size in place of sizeof.

Signed-off-by: Matthew Daley <mattjd@gmail.com>

diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index e153cb4..204e951 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -78,7 +78,7 @@ int xenctl_cpumap_to_cpumask(
     {
         if ( copy_from_guest(bytemap, xenctl_cpumap->bitmap, copy_bytes) )
             err = -EFAULT;
-        if ( (xenctl_cpumap->nr_cpus & 7) && (guest_bytes <=
sizeof(bytemap)) )
+        if ( (xenctl_cpumap->nr_cpus & 7) && (guest_bytes <=
(nr_cpu_ids + 7) / 8) )
             bytemap[guest_bytes-1] &= ~(0xff <<
(xenctl_cpumap->nr_cpus & 7));
     }
 
-- 
1.7.10.4

>>> On 13.11.12 at 07:04, Matthew Daley <mattjd@gmail.com> wrote:
> xenctl_cpumap_to_cpumask incorrectly uses sizeof when checking whether
> bits should be masked off from the input cpumap bitmap or not.
> 
> Fix by using the correct cpumask buffer size in place of sizeof.
> 
> Signed-off-by: Matthew Daley <mattjd@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>

However, I would have preferred the adjustment to be ...
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index e153cb4..204e951 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -78,7 +78,7 @@ int xenctl_cpumap_to_cpumask(
>      {
>          if ( copy_from_guest(bytemap, xenctl_cpumap->bitmap,
copy_bytes) )
>              err = -EFAULT;
> -        if ( (xenctl_cpumap->nr_cpus & 7) && (guest_bytes
<= sizeof(bytemap)) )
> +        if ( (xenctl_cpumap->nr_cpus & 7) && (guest_bytes
<= (nr_cpu_ids + 7) / 8) )
        if ( (xenctl_cpumap->nr_cpus & 7) && (guest_bytes <=
copy_bytes) )

or even (considering that guest_bytes >= copy_bytes due to the
way copy_bytes gets initialized)

        if ( (xenctl_cpumap->nr_cpus & 7) && (guest_bytes ==
copy_bytes) )

to make explicit when exactly the masking is necessary.

Further, the fact this is not security relevant (i.e. the bug could
not cause memory corruption) isn''t obvious either: It is implied
from _xmalloc() never returning chunks of data smaller than the
size of a pointer, i.e. even if sizeof(void*) > guest_bytes >
copy_bytes, the piece of memory erroneously written to would
still be inside the allocation done at the top of the function. I''d
suppose that would have been worth mentioning in the change
description.

And, for the record (and in order to determine backporting
needs), I caused this with c/s 23991:a7ccbc79fc17.

Jan
>              bytemap[guest_bytes-1] &= ~(0xff <<
(xenctl_cpumap->nr_cpus & 7));
>      }
>  
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel

On 13/11/2012 08:36, "Jan Beulich" <JBeulich@suse.com> wrote:
>> Fix by using the correct cpumask buffer size in place of sizeof.
>> 
>> Signed-off-by: Matthew Daley <mattjd@gmail.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> However, I would have preferred the adjustment to be ...
> 
>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
>> index e153cb4..204e951 100644
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -78,7 +78,7 @@ int xenctl_cpumap_to_cpumask(
>>      {
>>          if ( copy_from_guest(bytemap, xenctl_cpumap->bitmap,
copy_bytes) )
>>              err = -EFAULT;
>> -        if ( (xenctl_cpumap->nr_cpus & 7) &&
(guest_bytes <>> sizeof(bytemap)) )
>> +        if ( (xenctl_cpumap->nr_cpus & 7) &&
(guest_bytes <= (nr_cpu_ids +
>> 7) / 8) )
> 
>         if ( (xenctl_cpumap->nr_cpus & 7) && (guest_bytes
<= copy_bytes) )
> 
> or even (considering that guest_bytes >= copy_bytes due to the
> way copy_bytes gets initialized)
> 
>         if ( (xenctl_cpumap->nr_cpus & 7) && (guest_bytes ==
copy_bytes) )
> 
> to make explicit when exactly the masking is necessary.
Any of the three alternatives is fine by me.

Acked-by: Keir Fraser <keir@xen.org>

xenctl_cpumap_to_cpumask incorrectly uses sizeof when checking whether
bits should be masked off from the input cpumap bitmap or not.

Fix and make clearer by simply comparing the amount of bytes given in
the input cpumap to the amount actually copied; if equal, bits may need
to be masked off.

This does not have security impact: _xmalloc never returns allocations
smaller than the size of a pointer, hence the uncorrected buffer size
check would still not allow writes to unallocated memory.

Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
Jan: Agreed with both of your points. Here''s a v2.

diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index e153cb4..a7a6b9f 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -78,7 +78,7 @@ int xenctl_cpumap_to_cpumask(
     {
         if ( copy_from_guest(bytemap, xenctl_cpumap->bitmap, copy_bytes) )
             err = -EFAULT;
-        if ( (xenctl_cpumap->nr_cpus & 7) && (guest_bytes <=
sizeof(bytemap)) )
+        if ( (xenctl_cpumap->nr_cpus & 7) && (guest_bytes ==
copy_bytes) )
             bytemap[guest_bytes-1] &= ~(0xff <<
(xenctl_cpumap->nr_cpus & 7));
     }
 
-- 
1.7.10.4

On 13/11/2012 10:17, "Matthew Daley" <mattjd@gmail.com> wrote:
> xenctl_cpumap_to_cpumask incorrectly uses sizeof when checking whether
> bits should be masked off from the input cpumap bitmap or not.
> 
> Fix and make clearer by simply comparing the amount of bytes given in
> the input cpumap to the amount actually copied; if equal, bits may need
> to be masked off.
> 
> This does not have security impact: _xmalloc never returns allocations
> smaller than the size of a pointer, hence the uncorrected buffer size
> check would still not allow writes to unallocated memory.
> 
> Signed-off-by: Matthew Daley <mattjd@gmail.com>
Acked-by: Keir Fraser <keir@xen.org>
> ---
> Jan: Agreed with both of your points. Here''s a v2.
> 
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index e153cb4..a7a6b9f 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -78,7 +78,7 @@ int xenctl_cpumap_to_cpumask(
>      {
>          if ( copy_from_guest(bytemap, xenctl_cpumap->bitmap,
copy_bytes) )
>              err = -EFAULT;
> -        if ( (xenctl_cpumap->nr_cpus & 7) && (guest_bytes
<= sizeof(bytemap))
> )
> +        if ( (xenctl_cpumap->nr_cpus & 7) && (guest_bytes
== copy_bytes) )
>              bytemap[guest_bytes-1] &= ~(0xff <<
(xenctl_cpumap->nr_cpus &
> 7));
>      }
>