[Xen-devel] [PATCH] xen: clamp bitmaps to correct number of bits

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

# HG changeset patch
# User Ian Campbell <ian.campbell@xxxxxxxxxx>
# Date 1346932601 -3600
# Node ID 22d5e100131d1576d455780b9fdf36eacd453445
# Parent  7db66f2a0242c2dca66dca9165cb58a98d92dba9
xen: clamp bitmaps to correct number of bits

Valgrind running xl create reports:
 ==24777== Invalid read of size 4
 ==24777==    at 0x4072805: libxl__get_numa_candidate (libxl_numa.c:203)
 ==24777==    by 0x40680B6: libxl__build_pre (libxl_dom.c:166)
 ==24777==    by 0x405B82E: libxl__domain_build (libxl_create.c:323)
 ==24777==    by 0x405BB9C: domcreate_bootloader_done (libxl_create.c:747)
 ==24777==    by 0x407AD27: bootloader_local_detached_cb 
(libxl_bootloader.c:281)
 ==24777==    by 0x40508D8: local_device_detach_cb (libxl.c:2470)
 ==24777==    by 0x4052B10: libxl__device_disk_local_initiate_detach 
(libxl.c:2445)
 ==24777==    by 0x407AE9F: bootloader_callback (libxl_bootloader.c:265)
 ==24777==    by 0x407C69A: libxl__bootloader_run (libxl_bootloader.c:392)
 ==24777==    by 0x405CB24: do_domain_create (libxl_create.c:687)
 ==24777==    by 0x405CC5E: libxl_domain_create_new (libxl_create.c:1177)
 ==24777==    by 0x805BDE2: create_domain (xl_cmdimpl.c:1812)
 ==24777==  Address 0x42dbdd8 is 8 bytes after a block of size 48 alloc'd
 ==24777==    at 0x4023340: calloc (vg_replace_malloc.c:593)
 ==24777==    by 0x406D479: libxl__zalloc (libxl_internal.c:88)
 ==24777==    by 0x404EF38: libxl_get_cpu_topology (libxl.c:3707)
 ==24777==    by 0x4072232: libxl__get_numa_candidate (libxl_numa.c:314)
 ==24777==    by 0x40680B6: libxl__build_pre (libxl_dom.c:166)
 ==24777==    by 0x405B82E: libxl__domain_build (libxl_create.c:323)
 ==24777==    by 0x405BB9C: domcreate_bootloader_done (libxl_create.c:747)
 ==24777==    by 0x407AD27: bootloader_local_detached_cb 
(libxl_bootloader.c:281)
 ==24777==    by 0x40508D8: local_device_detach_cb (libxl.c:2470)
 ==24777==    by 0x4052B10: libxl__device_disk_local_initiate_detach 
(libxl.c:2445)
 ==24777==    by 0x407AE9F: bootloader_callback (libxl_bootloader.c:265)
 ==24777==    by 0x407C69A: libxl__bootloader_run (libxl_bootloader.c:392)

This is because with nr_cpus=4 the bitmask returned from Xen
contains 0xff rather than 0x0f bit our bitmap walking routines (e.g.
libxl_for_each_set_bit) round up to the next byte (so it iterates
e.g. 8 times not 4). This then causes us to access of the end of
whatever array we are walking through each set bit for.

The principal of least surprise suggests that these bits ought not to
be set and this is not a hot path so fix this at the hypervisor layer
by clamping the bits in the returned bitmap to the correct limit.

Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
---

The impact of this seems to be fairly benign in practice, so I'm not
sure about it for 4.2.0. Could leave to 4.2.1?

Dario,

Any chance you could look at the libxl bitmap functions in 4.3 and
make them use the right limit (i.e. nr_bits not (nr_bits+7)/8?

Thanks,
Ian.

diff -r 7db66f2a0242 -r 22d5e100131d xen/common/bitmap.c
--- a/xen/common/bitmap.c       Thu Sep 06 10:44:46 2012 +0100
+++ b/xen/common/bitmap.c       Thu Sep 06 12:56:41 2012 +0100
@@ -38,6 +38,17 @@
  * for the best explanations of this ordering.
  */
 
+/* Ensure that the last byte is zero from nbits onwards. */
+static void clamp_last_byte(uint8_t *bp, int nbits)
+{
+       int lastbyte = nbits/8;
+       int remainder = nbits % 8;
+       int mask = ((1U<<remainder)-1)&0xff;
+
+       if (remainder)
+               bp[lastbyte] &= mask;
+}
+
 int __bitmap_empty(const unsigned long *bitmap, int bits)
 {
        int k, lim = bits/BITS_PER_LONG;
@@ -485,6 +496,7 @@ void bitmap_long_to_byte(uint8_t *bp, co
                        nbits -= 8;
                }
        }
+       clamp_last_byte(bp, nbits);
 }
 
 void bitmap_byte_to_long(unsigned long *lp, const uint8_t *bp, int nbits)
@@ -507,6 +519,7 @@ void bitmap_byte_to_long(unsigned long *
 void bitmap_long_to_byte(uint8_t *bp, const unsigned long *lp, int nbits)
 {
        memcpy(bp, lp, (nbits+7)/8);
+       clamp_last_byte(bp, nbits);
 }
 
 void bitmap_byte_to_long(unsigned long *lp, const uint8_t *bp, int nbits)

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