1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] Handle copying bitmaps to larger data buffers

Allen, John posted 1 patch 5 years ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190415144245.26992-1-john.allen@amd.com
[libvirt] [PATCH] Handle copying bitmaps to larger data buffers
Posted by Allen, John 5 years ago 
If a bitmap of a shorter length than the data buffer is passed to
virBitmapToDataBuf, it will read off the end of the bitmap and copy junk
into the returned buffer. Add a check to only copy the length of the
bitmap to the buffer.

The problem can be observed after setting a vcpu affinity using the vcpupin
command on a system with a large number of cores:
  # virsh vcpupin example_domain 0 0
  # virsh vcpupin example_domain 0
     VCPU   CPU Affinity
    ---------------------------
     0      0,192,197-198,202

Signed-off-by: John Allen <john.allen@amd.com>
---
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
index d074f29e54..021174cbb3 100644
--- a/src/util/virbitmap.c
+++ b/src/util/virbitmap.c
@@ -824,11 +824,15 @@ virBitmapToDataBuf(virBitmapPtr bitmap,
                    unsigned char *bytes,
                    size_t len)
 {
+    size_t nbytes = bitmap->map_len * (VIR_BITMAP_BITS_PER_UNIT / CHAR_BIT);
     unsigned long *l;
     size_t i, j;
 
     memset(bytes, 0, len);
 
+    /* If bitmap and buffer differ in size, only fill to the smaller length */
+    len = MIN(len, nbytes);
+
     /* htole64 is not provided by gnulib, so we do the conversion by hand */
     l = bitmap->map;
     for (i = j = 0; i < len; i++, j++) {

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Handle copying bitmaps to larger data buffers
Posted by Allen, John 4 years, 12 months ago 
Gentle reminder. Any comments on this patch?

Thanks,
John

On Mon, Apr 15, 2019 at 09:43:07AM -0500, Allen, John wrote:
> If a bitmap of a shorter length than the data buffer is passed to
> virBitmapToDataBuf, it will read off the end of the bitmap and copy junk
> into the returned buffer. Add a check to only copy the length of the
> bitmap to the buffer.
> 
> The problem can be observed after setting a vcpu affinity using the vcpupin
> command on a system with a large number of cores:
>   # virsh vcpupin example_domain 0 0
>   # virsh vcpupin example_domain 0
>      VCPU   CPU Affinity
>     ---------------------------
>      0      0,192,197-198,202
> 
> Signed-off-by: John Allen <john.allen@amd.com>
> ---
> diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
> index d074f29e54..021174cbb3 100644
> --- a/src/util/virbitmap.c
> +++ b/src/util/virbitmap.c
> @@ -824,11 +824,15 @@ virBitmapToDataBuf(virBitmapPtr bitmap,
>                     unsigned char *bytes,
>                     size_t len)
>  {
> +    size_t nbytes = bitmap->map_len * (VIR_BITMAP_BITS_PER_UNIT / CHAR_BIT);
>      unsigned long *l;
>      size_t i, j;
>  
>      memset(bytes, 0, len);
>  
> +    /* If bitmap and buffer differ in size, only fill to the smaller length */
> +    len = MIN(len, nbytes);
> +
>      /* htole64 is not provided by gnulib, so we do the conversion by hand */
>      l = bitmap->map;
>      for (i = j = 0; i < len; i++, j++) {

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Handle copying bitmaps to larger data buffers
Posted by Pavel Hrdina 4 years, 11 months ago 
On Mon, Apr 15, 2019 at 02:43:07PM +0000, Allen, John wrote:
> If a bitmap of a shorter length than the data buffer is passed to
> virBitmapToDataBuf, it will read off the end of the bitmap and copy junk
> into the returned buffer. Add a check to only copy the length of the
> bitmap to the buffer.
> 
> The problem can be observed after setting a vcpu affinity using the vcpupin
> command on a system with a large number of cores:
>   # virsh vcpupin example_domain 0 0
>   # virsh vcpupin example_domain 0
>      VCPU   CPU Affinity
>     ---------------------------
>      0      0,192,197-198,202
> 
> Signed-off-by: John Allen <john.allen@amd.com>
> ---

Thanks for the patch, pushed now.

> diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
> index d074f29e54..021174cbb3 100644
> --- a/src/util/virbitmap.c
> +++ b/src/util/virbitmap.c
> @@ -824,11 +824,15 @@ virBitmapToDataBuf(virBitmapPtr bitmap,
>                     unsigned char *bytes,
>                     size_t len)
>  {
> +    size_t nbytes = bitmap->map_len * (VIR_BITMAP_BITS_PER_UNIT / CHAR_BIT);
>      unsigned long *l;
>      size_t i, j;
>  
>      memset(bytes, 0, len);
>  
> +    /* If bitmap and buffer differ in size, only fill to the smaller length */
> +    len = MIN(len, nbytes);
> +

I would probably store the min back to nbytes and used that but it
doesn't matter.

>      /* htole64 is not provided by gnulib, so we do the conversion by hand */
>      l = bitmap->map;
>      for (i = j = 0; i < len; i++, j++) {

Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Handle copying bitmaps to larger data buffers
Posted by Cole Robinson 4 years, 11 months ago 
On 4/25/19 5:42 AM, Pavel Hrdina wrote:
> On Mon, Apr 15, 2019 at 02:43:07PM +0000, Allen, John wrote:
>> If a bitmap of a shorter length than the data buffer is passed to
>> virBitmapToDataBuf, it will read off the end of the bitmap and copy junk
>> into the returned buffer. Add a check to only copy the length of the
>> bitmap to the buffer.
>>
>> The problem can be observed after setting a vcpu affinity using the vcpupin
>> command on a system with a large number of cores:
>>   # virsh vcpupin example_domain 0 0
>>   # virsh vcpupin example_domain 0
>>      VCPU   CPU Affinity
>>     ---------------------------
>>      0      0,192,197-198,202
>>
>> Signed-off-by: John Allen <john.allen@amd.com>
>> ---
> 
> Thanks for the patch, pushed now.
> 
>> diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
>> index d074f29e54..021174cbb3 100644
>> --- a/src/util/virbitmap.c
>> +++ b/src/util/virbitmap.c
>> @@ -824,11 +824,15 @@ virBitmapToDataBuf(virBitmapPtr bitmap,
>>                     unsigned char *bytes,
>>                     size_t len)
>>  {
>> +    size_t nbytes = bitmap->map_len * (VIR_BITMAP_BITS_PER_UNIT / CHAR_BIT);
>>      unsigned long *l;
>>      size_t i, j;
>>  
>>      memset(bytes, 0, len);
>>  
>> +    /* If bitmap and buffer differ in size, only fill to the smaller length */
>> +    len = MIN(len, nbytes);
>> +
> 
> I would probably store the min back to nbytes and used that but it
> doesn't matter.
> 
>>      /* htole64 is not provided by gnulib, so we do the conversion by hand */
>>      l = bitmap->map;
>>      for (i = j = 0; i < len; i++, j++) {
> 

ACK to the patch as well. I hit a similar issue in my attempts to test
this, thought they had a different root cause but I was wrong, so ignore
my message attached to the test driver patch.

FWIW if someone wants to manually reproduce, grab my test driver patch
posted earlier and attached, and the testdriver.xml attached. Revert
this patch, build and reproduce with:

$ ./tools/virsh --connect test://`pwd`/testdriver.xml "vcpupin test 0 4;
vcpuinfo test"

Similarly this gives wrong results too, only first 13 host cpus should
be online:

$ ./tools/virsh --connect test://`pwd`/testdriver.xml nodecpumap

The latter is due to some bad virBitmapToData usage, but it's separate.

Thanks,
Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.