| Message ID | 20171124200759.24733-1-npiggin@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | core/bitmap: fix bitmap iteration limit corruption | expand |
Nicholas Piggin <npiggin@gmail.com> writes: > The bitmap iterators did not reduce the number of bits to scan > when searching for the next bit, which would result in them > overruning their bitmap. > > These are only used in one place, in xive reset, and the effect > is that the xive reset code will keep zeroing memory until it > reaches a block of memory of MAX_EQ_COUNT >> 3 bits in length, > all zeroes. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > include/bitmap.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Nice! It seems that this test case modification would find it and check it's fixed too. With this patch (but without yours), a standard 'make check' would explode appropriately revealing the bug: ==17110== Invalid read of size 8 ==17110== at 0x400849: __bitmap_find_bit (bitmap.c:31) ==17110== by 0x400900: bitmap_find_zero_bit (bitmap.c:49) ==17110== by 0x400E50: main (run-bitmap.c:77) ==17110== Address 0x5221048 is 0 bytes after a block of size 8 alloc'd ==17110== at 0x4C2FB6B: malloc (vg_replace_malloc.c:299) ==17110== by 0x40093F: main (run-bitmap.c:24) diff --git a/core/test/run-bitmap.c b/core/test/run-bitmap.c index ceeb7d52f41e..7c68460a89bc 100644 --- a/core/test/run-bitmap.c +++ b/core/test/run-bitmap.c @@ -22,6 +22,7 @@ int main(void) { bitmap_t *map = malloc(sizeof(bitmap_elem_t)); + int i; memset(map, 0, sizeof(bitmap_elem_t)); assert(BITMAP_ELEMS(16) == (BITMAP_ELEMS(8))); @@ -73,6 +74,16 @@ int main(void) assert(*(unsigned long*)map == 0x00); assert(bitmap_tst_bit(*map, 8) == false); + bitmap_for_each_zero(*map, 7, i) { + bitmap_set_bit(*map, i); + } + + for (i = 0; i < 7; i++) + assert(bitmap_tst_bit(*map, i) == true); + + assert(bitmap_tst_bit(*map, 8) == false); + + free(map); return 0;
Stewart Smith <stewart@linux.vnet.ibm.com> writes: > Nicholas Piggin <npiggin@gmail.com> writes: >> The bitmap iterators did not reduce the number of bits to scan >> when searching for the next bit, which would result in them >> overruning their bitmap. >> >> These are only used in one place, in xive reset, and the effect >> is that the xive reset code will keep zeroing memory until it >> reaches a block of memory of MAX_EQ_COUNT >> 3 bits in length, >> all zeroes. >> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> --- >> include/bitmap.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) > > Nice! > > It seems that this test case modification would find it and check it's > fixed too. > > With this patch (but without yours), a standard 'make check' would > explode appropriately revealing the bug: > > ==17110== Invalid read of size 8 > ==17110== at 0x400849: __bitmap_find_bit (bitmap.c:31) > ==17110== by 0x400900: bitmap_find_zero_bit (bitmap.c:49) > ==17110== by 0x400E50: main (run-bitmap.c:77) > ==17110== Address 0x5221048 is 0 bytes after a block of size 8 alloc'd > ==17110== at 0x4C2FB6B: malloc (vg_replace_malloc.c:299) > ==17110== by 0x40093F: main (run-bitmap.c:24) Thanks! Merged to master as of 2df2407375963ab08bcb3c62eb7230c07e734687 with the test going in just after it as 2be4422dace9960b56f27cc611644ca5fdd292d9
diff --git a/include/bitmap.h b/include/bitmap.h index 12913ea00..f3510f725 100644 --- a/include/bitmap.h +++ b/include/bitmap.h @@ -59,11 +59,11 @@ extern int bitmap_find_one_bit(bitmap_t map, unsigned int start, #define bitmap_for_each_zero(map, size, bit) \ for (bit = bitmap_find_zero_bit(map, 0, size); \ bit >= 0; \ - bit = bitmap_find_zero_bit(map, bit + 1, size)) + bit = bitmap_find_zero_bit(map, (bit) + 1, (size) - (bit) - 1)) #define bitmap_for_each_one(map, size, bit) \ for (bit = bitmap_find_one_bit(map, 0, size); \ bit >= 0; \ - bit = bitmap_find_one_bit(map, bit + 1, size)) + bit = bitmap_find_one_bit(map, (bit) + 1, (size) - (bit) - 1)) #endif /* __BITMAP_H */
The bitmap iterators did not reduce the number of bits to scan when searching for the next bit, which would result in them overruning their bitmap. These are only used in one place, in xive reset, and the effect is that the xive reset code will keep zeroing memory until it reaches a block of memory of MAX_EQ_COUNT >> 3 bits in length, all zeroes. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- include/bitmap.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)