[PATCH 0/4] virbitmap: Fix buffer overflow when shrinking a bitmap and copying it
See patches for rationale. Peter Krempa (4): virBitmapNewCopy: Honor sizes of either bitmap when doing memcpy() virbitmap: Extract and reuse buffer size calculation into a function util: virbitmap: Extract clearing of unused bits at the end of the last unit util: bitmap: Rewrite virBitmapShrink using new helpers src/util/virbitmap.c | 84 +++++++++++++++++++++----------------------- 1 file changed, 40 insertions(+), 44 deletions(-) -- 2.47.0
'virBitmapNewCopy()' allocates a new bitmap with the same number of bits but uses the internal allocation length as argument for the memcpy() operation to copy the bits. Due to bugs in other code these may not be the same resulting into a buffer overflow if the source is over-allocated. Use the buffer length of the target bitmap instead. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virbitmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index b8d0352bb1..a1a8c5d126 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -582,7 +582,7 @@ virBitmapNewCopy(virBitmap *src) { virBitmap *dst = virBitmapNew(src->nbits); - memcpy(dst->map, src->map, src->map_len * sizeof(src->map[0])); + memcpy(dst->map, src->map, dst->map_len * sizeof(src->map[0])); return dst; } -- 2.47.0
Calculating the number of element can come handy in multiple places, extract it from virBitmapNew. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virbitmap.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index a1a8c5d126..7dc63da6db 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -49,6 +49,22 @@ struct _virBitmap { #define VIR_BITMAP_BIT(b) (1UL << VIR_BITMAP_BIT_OFFSET(b)) +/** + * Calculates and returns the number of elements in the bitmap buffer to fit @bits. + */ +static size_t +virBitmapBuffsize(size_t nbits) +{ + if (SIZE_MAX - VIR_BITMAP_BITS_PER_UNIT < nbits) { + /* VIR_DIV_UP would overflow, let's overallocate by 1 entry instead of + * the potential overflow */ + return (nbits / VIR_BITMAP_BITS_PER_UNIT) + 1; + } + + return VIR_DIV_UP(nbits, VIR_BITMAP_BITS_PER_UNIT); +} + + /** * virBitmapNew: * @size: number of bits @@ -61,15 +77,7 @@ virBitmap * virBitmapNew(size_t size) { virBitmap *bitmap; - size_t sz; - - if (SIZE_MAX - VIR_BITMAP_BITS_PER_UNIT < size) { - /* VIR_DIV_UP would overflow, let's overallocate by 1 entry instead of - * the potential overflow */ - sz = (size / VIR_BITMAP_BITS_PER_UNIT) + 1; - } else { - sz = VIR_DIV_UP(size, VIR_BITMAP_BITS_PER_UNIT); - } + size_t sz = virBitmapBuffsize(size); bitmap = g_new0(virBitmap, 1); @@ -133,7 +141,7 @@ static void virBitmapExpand(virBitmap *map, size_t b) { - size_t new_len = VIR_DIV_UP(b + 1, VIR_BITMAP_BITS_PER_UNIT); + size_t new_len = virBitmapBuffsize(b + 1); /* resize the memory if necessary */ if (map->map_len < new_len) { -- 2.47.0
Extract the clearing of the traling bits from 'virBitmapSetAll' into a new helper. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virbitmap.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 7dc63da6db..35cf729a22 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -757,6 +757,19 @@ virBitmapSize(virBitmap *bitmap) } +/** + * Internal helper that clears the unused bits at the end of the last bitmap unit. + */ +static void +virBitmapClearTail(virBitmap *bitmap) +{ + size_t tail = bitmap->nbits % VIR_BITMAP_BITS_PER_UNIT; + + if (tail) + bitmap->map[bitmap->map_len - 1] &= -1UL >> (VIR_BITMAP_BITS_PER_UNIT - tail); +} + + /** * virBitmapSetAll: * @bitmap: the bitmap @@ -765,15 +778,10 @@ virBitmapSize(virBitmap *bitmap) */ void virBitmapSetAll(virBitmap *bitmap) { - int tail = bitmap->nbits % VIR_BITMAP_BITS_PER_UNIT; - memset(bitmap->map, 0xff, bitmap->map_len * (VIR_BITMAP_BITS_PER_UNIT / CHAR_BIT)); - /* Ensure tail bits are clear. */ - if (tail) - bitmap->map[bitmap->map_len - 1] &= - -1UL >> (VIR_BITMAP_BITS_PER_UNIT - tail); + virBitmapClearTail(bitmap); } -- 2.47.0
Rather than reimplement everything manually use virBitmapBuffsize to find the current number of units, realloc the buffer and clear the tail using virBitmapClearTail(). This fixes a corner case where the buffer would be over-allocated by one unit when shrinking to the boundary of the unit size. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virbitmap.c | 34 +++++++--------------------------- 1 file changed, 7 insertions(+), 27 deletions(-) diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 35cf729a22..138c1ac5af 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -1187,33 +1187,13 @@ void virBitmapShrink(virBitmap *map, size_t b) { - size_t toremove; - size_t nl = 0; - size_t nb = 0; - - if (!map) - return; - - if (map->nbits >= b) - map->nbits = b; - - nl = map->nbits / VIR_BITMAP_BITS_PER_UNIT; - nb = map->nbits % VIR_BITMAP_BITS_PER_UNIT; - - /* If we're at the end of the allocation the attempt to clear 'map->nbit' - * and further would be beyond the end of the bitmap */ - if (nl >= map->map_alloc) + if (!map || + map->nbits <= b) return; - map->map[nl] &= ((1UL << nb) - 1); - - toremove = map->map_alloc - (nl + 1); - - if (toremove == 0) - return; - - VIR_SHRINK_N(map->map, map->map_alloc, toremove); - - /* length needs to be fixed as well */ - map->map_len = map->map_alloc; + map->nbits = b; + map->map_len = virBitmapBuffsize(b); + map->map = g_renew(unsigned long, map->map, map->map_len); + map->map_alloc = map->map_len; + virBitmapClearTail(map); } -- 2.47.0
On Thu, Oct 17, 2024 at 14:09:08 +0200, Peter Krempa wrote:
See patches for rationale.
Peter Krempa (4): virBitmapNewCopy: Honor sizes of either bitmap when doing memcpy() virbitmap: Extract and reuse buffer size calculation into a function util: virbitmap: Extract clearing of unused bits at the end of the last unit util: bitmap: Rewrite virBitmapShrink using new helpers
src/util/virbitmap.c | 84 +++++++++++++++++++++----------------------- 1 file changed, 40 insertions(+), 44 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
participants (2)
-
Jiri Denemark -
Peter Krempa