From 54f1c17bbe728b0fbef51bb6c6601746e51d2fbf Mon Sep 17 00:00:00 2001 From: Dana Robinson <43805+derobins@users.noreply.github.com> Date: Tue, 25 Apr 2023 10:03:41 -0700 Subject: Harden the v1 B-tree and local heap cache clients (#2803) * Hardens v1 B-tree deserialize function * Harden the H5HL deserialize functionality --- src/H5Bcache.c | 83 +++++++++++++++++++++++++------------------------------- src/H5Bpkg.h | 22 +++++++-------- src/H5Bprivate.h | 17 ++++++------ src/H5HLcache.c | 64 +++++++++++++++++++++++-------------------- 4 files changed, 90 insertions(+), 96 deletions(-) diff --git a/src/H5Bcache.c b/src/H5Bcache.c index cd0a0ba..437bc1b 100644 --- a/src/H5Bcache.c +++ b/src/H5Bcache.c @@ -13,10 +13,8 @@ /*------------------------------------------------------------------------- * * Created: H5Bcache.c - * Oct 31 2005 - * Quincey Koziol * - * Purpose: Implement B-tree metadata cache methods. + * Purpose: Implement B-tree metadata cache methods * *------------------------------------------------------------------------- */ @@ -83,13 +81,9 @@ const H5AC_class_t H5AC_BT[1] = {{ /*------------------------------------------------------------------------- * Function: H5B__cache_get_initial_load_size * - * Purpose: Compute the size of the data structure on disk. - * - * Return: Non-negative on success/Negative on failure - * - * Programmer: Quincey Koziol - * May 18, 2010 + * Purpose: Compute the size of the data structure on disk * + * Return: SUCCEED/FAIL *------------------------------------------------------------------------- */ static herr_t @@ -117,24 +111,20 @@ H5B__cache_get_initial_load_size(void *_udata, size_t *image_len) /*------------------------------------------------------------------------- * Function: H5B__cache_deserialize * - * Purpose: Deserialize the data structure from disk. - * - * Return: Success: Pointer to a new B-tree node. - * Failure: NULL - * - * Programmer: Quincey Koziol - * Mar 24, 2008 + * Purpose: Deserialize the data structure from disk * + * Return: Success: Pointer to a new B-tree node + * Failure: NULL *------------------------------------------------------------------------- */ static void * -H5B__cache_deserialize(const void *_image, size_t H5_ATTR_UNUSED len, void *_udata, - hbool_t H5_ATTR_UNUSED *dirty) +H5B__cache_deserialize(const void *_image, size_t len, void *_udata, hbool_t H5_ATTR_UNUSED *dirty) { H5B_t *bt = NULL; /* Pointer to the deserialized B-tree node */ H5B_cache_ud_t *udata = (H5B_cache_ud_t *)_udata; /* User data for callback */ H5B_shared_t *shared; /* Pointer to shared B-tree info */ const uint8_t *image = (const uint8_t *)_image; /* Pointer into image buffer */ + const uint8_t *p_end = image + len - 1; /* End of image buffer */ uint8_t *native; /* Pointer to native keys */ unsigned u; /* Local index variable */ H5B_t *ret_value = NULL; /* Return value */ @@ -156,7 +146,8 @@ H5B__cache_deserialize(const void *_image, size_t H5_ATTR_UNUSED len, void *_uda /* Get a pointer to the shared info, for convenience */ shared = (H5B_shared_t *)H5UC_GET_OBJ(bt->rc_shared); - HDassert(shared); + if (NULL == shared) + HGOTO_ERROR(H5E_BTREE, H5E_CANTGET, NULL, "can't get a pointer to shared data") /* Allocate space for the native keys and child addresses */ if (NULL == (bt->native = H5FL_BLK_MALLOC(native_block, shared->sizeof_keys))) @@ -164,49 +155,61 @@ H5B__cache_deserialize(const void *_image, size_t H5_ATTR_UNUSED len, void *_uda if (NULL == (bt->child = H5FL_SEQ_MALLOC(haddr_t, (size_t)shared->two_k))) HGOTO_ERROR(H5E_BTREE, H5E_CANTALLOC, NULL, "can't allocate buffer for child addresses") - /* magic number */ + /* Magic number */ + if (H5_IS_BUFFER_OVERFLOW(image, H5_SIZEOF_MAGIC, p_end)) + HGOTO_ERROR(H5E_BTREE, H5E_OVERFLOW, NULL, "ran off end of input buffer while decoding"); if (HDmemcmp(image, H5B_MAGIC, (size_t)H5_SIZEOF_MAGIC) != 0) HGOTO_ERROR(H5E_BTREE, H5E_BADVALUE, NULL, "wrong B-tree signature") image += H5_SIZEOF_MAGIC; - /* node type and level */ + /* Node type and level */ + if (H5_IS_BUFFER_OVERFLOW(image, 2, p_end)) + HGOTO_ERROR(H5E_BTREE, H5E_OVERFLOW, NULL, "ran off end of input buffer while decoding"); if (*image++ != (uint8_t)udata->type->id) HGOTO_ERROR(H5E_BTREE, H5E_CANTLOAD, NULL, "incorrect B-tree node type") bt->level = *image++; - /* entries used */ + /* Entries used */ + if (H5_IS_BUFFER_OVERFLOW(image, 2, p_end)) + HGOTO_ERROR(H5E_BTREE, H5E_OVERFLOW, NULL, "ran off end of input buffer while decoding"); UINT16DECODE(image, bt->nchildren); /* Check if bt->nchildren is greater than two_k */ if (bt->nchildren > shared->two_k) HGOTO_ERROR(H5E_BTREE, H5E_BADVALUE, NULL, "number of children is greater than maximum") - /* sibling pointers */ + /* Sibling pointers */ + if (H5_IS_BUFFER_OVERFLOW(image, H5F_sizeof_addr(udata->f), p_end)) + HGOTO_ERROR(H5E_BTREE, H5E_OVERFLOW, NULL, "ran off end of input buffer while decoding"); H5F_addr_decode(udata->f, (const uint8_t **)&image, &(bt->left)); + + if (H5_IS_BUFFER_OVERFLOW(image, H5F_sizeof_addr(udata->f), p_end)) + HGOTO_ERROR(H5E_BTREE, H5E_OVERFLOW, NULL, "ran off end of input buffer while decoding"); H5F_addr_decode(udata->f, (const uint8_t **)&image, &(bt->right)); - /* the child/key pairs */ + /* Child/key pairs */ native = bt->native; for (u = 0; u < bt->nchildren; u++) { /* Decode native key value */ + if (H5_IS_BUFFER_OVERFLOW(image, shared->sizeof_rkey, p_end)) + HGOTO_ERROR(H5E_BTREE, H5E_OVERFLOW, NULL, "ran off end of input buffer while decoding"); if ((udata->type->decode)(shared, image, native) < 0) HGOTO_ERROR(H5E_BTREE, H5E_CANTDECODE, NULL, "unable to decode key") image += shared->sizeof_rkey; native += udata->type->sizeof_nkey; /* Decode address value */ + if (H5_IS_BUFFER_OVERFLOW(image, H5F_sizeof_addr(udata->f), p_end)) + HGOTO_ERROR(H5E_BTREE, H5E_OVERFLOW, NULL, "ran off end of input buffer while decoding"); H5F_addr_decode(udata->f, (const uint8_t **)&image, bt->child + u); - } /* end for */ + } - /* Decode final key */ + /* Final key */ if (bt->nchildren > 0) { /* Decode native key value */ if ((udata->type->decode)(shared, image, native) < 0) HGOTO_ERROR(H5E_BTREE, H5E_CANTDECODE, NULL, "unable to decode key") - } /* end if */ - - /* Sanity check */ - HDassert((size_t)((const uint8_t *)image - (const uint8_t *)_image) <= len); + } /* Set return value */ ret_value = bt; @@ -224,11 +227,7 @@ done: * * Purpose: Compute the size of the data structure on disk. * - * Return: Non-negative on success/Negative on failure - * - * Programmer: Quincey Koziol - * May 20, 2010 - * + * Return: SUCCEED/FAIL *------------------------------------------------------------------------- */ static herr_t @@ -256,13 +255,9 @@ H5B__cache_image_len(const void *_thing, size_t *image_len) /*------------------------------------------------------------------------- * Function: H5B__cache_serialize * - * Purpose: Serialize the data structure for writing to disk. - * - * Return: Non-negative on success/Negative on failure - * - * Programmer: Quincey Koziol - * Mar 24, 2008 + * Purpose: Serialize the data structure for writing to disk * + * Return: SUCCEED/FAIL *------------------------------------------------------------------------- */ static herr_t @@ -341,11 +336,7 @@ done: * * Purpose: Destroy/release an "in core representation" of a data structure * - * Return: Non-negative on success/Negative on failure - * - * Programmer: Quincey Koziol - * Mar 26, 2008 - * + * Return: SUCCEED/FAIL *------------------------------------------------------------------------- */ static herr_t diff --git a/src/H5Bpkg.h b/src/H5Bpkg.h index ef9f56e..f50bd48 100644 --- a/src/H5Bpkg.h +++ b/src/H5Bpkg.h @@ -44,21 +44,21 @@ /* The B-tree node as stored in memory... */ typedef struct H5B_t { H5AC_info_t cache_info; /* Information for H5AC cache functions */ - /* _must_ be first field in structure */ - H5UC_t *rc_shared; /*ref-counted shared info */ - unsigned level; /*node level */ - unsigned nchildren; /*number of child pointers */ - haddr_t left; /*address of left sibling */ - haddr_t right; /*address of right sibling */ - uint8_t *native; /*array of keys in native format */ - haddr_t *child; /*2k child pointers */ + /* MUST be first field in structure */ + H5UC_t *rc_shared; /* Ref-counted shared info */ + unsigned level; /* Node level */ + unsigned nchildren; /* Number of child pointers */ + haddr_t left; /* Address of left sibling */ + haddr_t right; /* Address of right sibling */ + uint8_t *native; /* Array of keys in native format */ + haddr_t *child; /* 2k child pointers */ } H5B_t; /* Callback info for loading a B-tree node into the cache */ typedef struct H5B_cache_ud_t { - H5F_t *f; /* File that B-tree node is within */ - const struct H5B_class_t *type; /* Type of tree */ - H5UC_t *rc_shared; /* Ref-counted shared info */ + H5F_t *f; /* File that B-tree node is within */ + const struct H5B_class_t *type; /* Type of tree */ + H5UC_t *rc_shared; /* Ref-counted shared info */ } H5B_cache_ud_t; /*****************************/ diff --git a/src/H5Bprivate.h b/src/H5Bprivate.h index 49e400c..0017c43 100644 --- a/src/H5Bprivate.h +++ b/src/H5Bprivate.h @@ -82,16 +82,16 @@ typedef int (*H5B_operator_t)(H5F_t *f, const void *_lt_key, haddr_t addr, const * the instances of nodes in that B-tree. */ typedef struct H5B_shared_t { - const struct H5B_class_t *type; /* Type of tree */ - unsigned two_k; /* 2*"K" value for tree's nodes */ - size_t sizeof_rkey; /* Size of raw (disk) key */ - size_t sizeof_rnode; /* Size of raw (disk) node */ - size_t sizeof_keys; /* Size of native (memory) key node */ - size_t sizeof_addr; /* Size of file address (in bytes) */ - size_t sizeof_len; /* Size of file lengths (in bytes) */ + const struct H5B_class_t *type; /* Type of tree */ + unsigned two_k; /* 2*"K" value for tree's nodes */ + size_t sizeof_rkey; /* Size of raw (disk) key */ + size_t sizeof_rnode; /* Size of raw (disk) node */ + size_t sizeof_keys; /* Size of native (memory) key node */ + size_t sizeof_addr; /* Size of file address (in bytes) */ + size_t sizeof_len; /* Size of file lengths (in bytes) */ uint8_t *page; /* Disk page */ size_t *nkey; /* Offsets of each native key in native key buffer */ - void *udata; /* 'Local' info for a B-tree */ + void *udata; /* 'Local' info for a B-tree */ } H5B_shared_t; /* @@ -101,7 +101,6 @@ typedef struct H5B_shared_t { * has an array of K values indexed by the `id' class field below. The * array is initialized with the HDF5_BTREE_K_DEFAULT macro. */ - typedef struct H5B_class_t { H5B_subid_t id; /*id as found in file*/ size_t sizeof_nkey; /*size of native (memory) key*/ diff --git a/src/H5HLcache.c b/src/H5HLcache.c index 72af9b4..c04efb6 100644 --- a/src/H5HLcache.c +++ b/src/H5HLcache.c @@ -13,10 +13,8 @@ /*------------------------------------------------------------------------- * * Created: H5HLcache.c - * Feb 5 2008 - * Quincey Koziol * - * Purpose: Implement local heap metadata cache methods. + * Purpose: Implement local heap metadata cache methods * *------------------------------------------------------------------------- */ @@ -81,7 +79,8 @@ static herr_t H5HL__cache_datablock_notify(H5C_notify_action_t action, void *_th static herr_t H5HL__cache_datablock_free_icr(void *thing); /* Header deserialization */ -static herr_t H5HL__hdr_deserialize(H5HL_t *heap, const uint8_t *image, H5HL_cache_prfx_ud_t *udata); +static herr_t H5HL__hdr_deserialize(H5HL_t *heap, const uint8_t *image, size_t len, + H5HL_cache_prfx_ud_t *udata); /* Free list de/serialization */ static herr_t H5HL__fl_deserialize(H5HL_t *heap); @@ -137,38 +136,39 @@ const H5AC_class_t H5AC_LHEAP_DBLK[1] = {{ /*------------------------------------------------------------------------- * Function: H5HL__hdr_deserialize() * - * Purpose: Decode a local heap's header - * - * Return: Success: SUCCEED - * Failure: FAIL - * - * Programmer: Quincey Koziol - * December 15, 2016 + * Purpose: Decode a local heap's header * + * Return: SUCCEED/FAIL *------------------------------------------------------------------------- */ static herr_t -H5HL__hdr_deserialize(H5HL_t *heap, const uint8_t *image, H5HL_cache_prfx_ud_t *udata) +H5HL__hdr_deserialize(H5HL_t *heap, const uint8_t *image, size_t len, H5HL_cache_prfx_ud_t *udata) { - herr_t ret_value = SUCCEED; /* Return value */ + const uint8_t *p_end = image + len - 1; /* End of image buffer */ + herr_t ret_value = SUCCEED; FUNC_ENTER_PACKAGE - /* Sanity checks */ HDassert(heap); HDassert(image); HDassert(udata); - /* Check magic number */ + /* Magic number */ + if (H5_IS_BUFFER_OVERFLOW(image, H5_SIZEOF_MAGIC, p_end)) + HGOTO_ERROR(H5E_HEAP, H5E_OVERFLOW, FAIL, "ran off end of input buffer while decoding"); if (HDmemcmp(image, H5HL_MAGIC, (size_t)H5_SIZEOF_MAGIC) != 0) HGOTO_ERROR(H5E_HEAP, H5E_BADVALUE, FAIL, "bad local heap signature") image += H5_SIZEOF_MAGIC; /* Version */ + if (H5_IS_BUFFER_OVERFLOW(image, 1, p_end)) + HGOTO_ERROR(H5E_HEAP, H5E_OVERFLOW, FAIL, "ran off end of input buffer while decoding"); if (H5HL_VERSION != *image++) HGOTO_ERROR(H5E_HEAP, H5E_VERSION, FAIL, "wrong version number in local heap") /* Reserved */ + if (H5_IS_BUFFER_OVERFLOW(image, 3, p_end)) + HGOTO_ERROR(H5E_HEAP, H5E_OVERFLOW, FAIL, "ran off end of input buffer while decoding"); image += 3; /* Store the prefix's address & length */ @@ -176,14 +176,20 @@ H5HL__hdr_deserialize(H5HL_t *heap, const uint8_t *image, H5HL_cache_prfx_ud_t * heap->prfx_size = udata->sizeof_prfx; /* Heap data size */ + if (H5_IS_BUFFER_OVERFLOW(image, udata->sizeof_size, p_end)) + HGOTO_ERROR(H5E_HEAP, H5E_OVERFLOW, FAIL, "ran off end of input buffer while decoding"); H5F_DECODE_LENGTH_LEN(image, heap->dblk_size, udata->sizeof_size); /* Free list head */ + if (H5_IS_BUFFER_OVERFLOW(image, udata->sizeof_size, p_end)) + HGOTO_ERROR(H5E_HEAP, H5E_OVERFLOW, FAIL, "ran off end of input buffer while decoding"); H5F_DECODE_LENGTH_LEN(image, heap->free_block, udata->sizeof_size); if (heap->free_block != H5HL_FREE_NULL && heap->free_block >= heap->dblk_size) HGOTO_ERROR(H5E_HEAP, H5E_BADVALUE, FAIL, "bad heap free list") /* Heap data address */ + if (H5_IS_BUFFER_OVERFLOW(image, udata->sizeof_addr, p_end)) + HGOTO_ERROR(H5E_HEAP, H5E_OVERFLOW, FAIL, "ran off end of input buffer while decoding"); H5F_addr_decode_len(udata->sizeof_addr, &image, &(heap->dblk_addr)); done: @@ -344,8 +350,7 @@ H5HL__cache_prefix_get_initial_load_size(void H5_ATTR_UNUSED *_udata, size_t *im *------------------------------------------------------------------------- */ static herr_t -H5HL__cache_prefix_get_final_load_size(const void *_image, size_t H5_ATTR_NDEBUG_UNUSED image_len, - void *_udata, size_t *actual_len) +H5HL__cache_prefix_get_final_load_size(const void *_image, size_t image_len, void *_udata, size_t *actual_len) { const uint8_t *image = (const uint8_t *)_image; /* Pointer into raw data buffer */ H5HL_cache_prfx_ud_t *udata = (H5HL_cache_prfx_ud_t *)_udata; /* User data for callback */ @@ -363,7 +368,7 @@ H5HL__cache_prefix_get_final_load_size(const void *_image, size_t H5_ATTR_NDEBUG HDmemset(&heap, 0, sizeof(H5HL_t)); /* Deserialize the heap's header */ - if (H5HL__hdr_deserialize(&heap, (const uint8_t *)image, udata) < 0) + if (H5HL__hdr_deserialize(&heap, (const uint8_t *)image, image_len, udata) < 0) HGOTO_ERROR(H5E_HEAP, H5E_CANTDECODE, FAIL, "can't decode local heap header") /* Set the final size for the cache image */ @@ -383,25 +388,22 @@ done: /*------------------------------------------------------------------------- * Function: H5HL__cache_prefix_deserialize * - * Purpose: Given a buffer containing the on disk image of the local - * heap prefix, deserialize it, load its contents into a newly allocated - * instance of H5HL_prfx_t, and return a pointer to the new instance. - * - * Return: Success: Pointer to in core representation - * Failure: NULL - * - * Programmer: John Mainzer - * 6/21/14 + * Purpose: Given a buffer containing the on disk image of the local + * heap prefix, deserialize it, load its contents into a newly + * allocated instance of H5HL_prfx_t, and return a pointer to + * the new instance. * + * Return: Success: Pointer to in core representation + * Failure: NULL *------------------------------------------------------------------------- */ static void * -H5HL__cache_prefix_deserialize(const void *_image, size_t H5_ATTR_NDEBUG_UNUSED len, void *_udata, - hbool_t H5_ATTR_UNUSED *dirty) +H5HL__cache_prefix_deserialize(const void *_image, size_t len, void *_udata, hbool_t H5_ATTR_UNUSED *dirty) { H5HL_t *heap = NULL; /* Local heap */ H5HL_prfx_t *prfx = NULL; /* Heap prefix deserialized */ const uint8_t *image = (const uint8_t *)_image; /* Pointer into decoding buffer */ + const uint8_t *p_end = image + len - 1; /* End of image buffer */ H5HL_cache_prfx_ud_t *udata = (H5HL_cache_prfx_ud_t *)_udata; /* User data for callback */ void *ret_value = NULL; /* Return value */ @@ -422,7 +424,7 @@ H5HL__cache_prefix_deserialize(const void *_image, size_t H5_ATTR_NDEBUG_UNUSED HGOTO_ERROR(H5E_HEAP, H5E_CANTALLOC, NULL, "can't allocate local heap structure"); /* Deserialize the heap's header */ - if (H5HL__hdr_deserialize(heap, (const uint8_t *)image, udata) < 0) + if (H5HL__hdr_deserialize(heap, (const uint8_t *)image, len, udata) < 0) HGOTO_ERROR(H5E_HEAP, H5E_CANTDECODE, NULL, "can't decode local heap header") /* Allocate the heap prefix */ @@ -446,6 +448,8 @@ H5HL__cache_prefix_deserialize(const void *_image, size_t H5_ATTR_NDEBUG_UNUSED image = ((const uint8_t *)_image) + heap->prfx_size; /* Copy the heap data from the speculative read buffer */ + if (H5_IS_BUFFER_OVERFLOW(image, heap->dblk_size, p_end)) + HGOTO_ERROR(H5E_HEAP, H5E_OVERFLOW, NULL, "ran off end of input buffer while decoding"); H5MM_memcpy(heap->dblk_image, image, heap->dblk_size); /* Build free list */ -- cgit v0.12