Fix several issues uncovered by CVE-2025-7067 #5938

jhendersonHDF · 2025-10-23T16:57:05Z

Fixes several crashes/assertion failures related to CVE-2025-7067

Important

Fixes CVE-2025-7067 issues by updating H5FS_sect_add to handle section merging/shrinking and improving error handling.

Functionality:
- H5FS_sect_add in H5FSsection.c now accepts an additional bool *merged_or_shrunk parameter to track if sections are merged or shrunk.
- Updates to H5MF__add_sect in H5MF.c to handle merged_or_shrunk parameter.
Error Handling:
- Improved error handling in H5FS_vfd_alloc_hdr_and_section_info_if_needed in H5MF.c to manage allocation failures.
Tests:
- Updated test cases in freespace.c and mf.c to include the new parameter in H5FS_sect_add calls.

^{This description was created by}^{for ef61da4. You can customize this summary. It will automatically update as commits are pushed.}

jhendersonHDF · 2025-10-23T16:59:58Z

src/H5FSsection.c

 herr_t
-H5FS_sect_add(H5F_t *f, H5FS_t *fspace, H5FS_section_info_t *sect, unsigned flags, void *op_data)
+H5FS_sect_add(H5F_t *f, H5FS_t *fspace, H5FS_section_info_t *sect, unsigned flags, void *op_data,
+              bool *merged_or_shrunk)


Callers of this function need to know whether the free space section passed in was merged or shrunk away, even on failure. Otherwise, calling code can incorrectly assume that it needs to free the passed in section if it was allocated by the calling code and H5FS_sect_add() failed. In some cases, the passed in section will have already been freed, eventually resulting in a double free situation when the calling code frees the section.

An alternative approach would be to pass in a H5FS_section_info_t **, but that seemed uglier and prone to the caller accidentally losing a pointer.

jhendersonHDF · 2025-10-23T17:00:58Z

src/H5FSsection.c

    } /* end if */

 done:
+    if (ret_value < 0) {


When this function fails, remove everything that was previously inserted into the metadata cache. Otherwise, crashes or assertion failures can occur later on when trying to flush out half-initialized entries that were inserted.

src/H5MF.c

jhendersonHDF · 2025-10-23T17:02:31Z

src/H5MF.c

-        if (H5MF__add_sect(f, alloc_type, f->shared->fs_man[fs_type], node) < 0)
+        if (H5MF__add_sect(f, alloc_type, f->shared->fs_man[fs_type], node, &section_merged_or_shunk) < 0) {
+            if (section_merged_or_shunk)
+                node = NULL;


Avoid double-freeing this section that was allocated above in case H5MF__add_sect() already freed it.

jhendersonHDF · 2025-10-23T17:03:42Z

src/H5MF.c

-            if (H5MF__add_sect(f, alloc_type, fspace, node) < 0)
+            if (H5MF__add_sect(f, alloc_type, fspace, node, NULL) < 0) {
+                node->sect_info.addr -= size;
+                node->sect_info.size += size;


Reverse the modifications made above on failure.

jhendersonHDF · 2025-10-23T17:16:35Z

src/H5MF.c


-        assert(!H5_addr_defined(f->shared->fs_addr[sm_fshdr_fs_type]));
-        assert(!H5_addr_defined(f->shared->fs_addr[sm_fssinfo_fs_type]));
+        if (H5_addr_defined(f->shared->fs_addr[sm_fshdr_fs_type]))


These assertions can be triggered by a file space info message in the file's superblock extension that was specifically crafted. Throwing an error here causes the library to fail to be able to settle the MDFSM, which eventually leads to issues closing the library and leaked memory due to the metadata cache not being able to be flushed and destroyed. In Debug builds this still results in an assertion failure when specifically-crafted files are opened, but addressing that will require significant changes to the metadata cache shutdown process.

An alternative is to simply wipe out these addresses to HADDR_UNDEF and let the following logic take over, but this could end up dropping free space on the floor and also causes far more problems when given a specifically crafted file.

Could this be detected when the file space info message is decoded?

In theory yes, however there was no obvious documentation or information I could find on which of the free space managers are and aren't allowed to have a defined address right after decoding and after asking around the opinion seemed to be that any of the managers could be defined. In this specific case however it seems the assumption is that these particular ones won't have a defined address.

jhendersonHDF · 2025-10-23T17:20:53Z

src/H5MFsection.c

    assert(sect1);
    assert(sect2);
    assert(sect1->sect_info.type == sect2->sect_info.type); /* Checks "MERGE_SYM" flag */
-    assert(H5_addr_lt(sect1->sect_info.addr, sect2->sect_info.addr));


With the file given in #5577, an attempt is made to place an overlapping section of file free space into the free space manager. That eventually causes this assertion to trigger. While a true fix would likely involve detecting this situation earlier on, this is probably a non-trivial effort. Since these functions are only checking whether two free space sections can be merged and not actually doing the merging, removing these assertions and just letting them return false seemed appropriate.

We should leave open an issue to add the correct fix, and note that these assertions should be re enabled at that point

Will do. Should I instead comment these out for now then?

That sounds good

fortnern · 2025-10-28T22:09:38Z

src/H5FSsection.c

+         * Avoid expunging the entry, as the information needs to be kept around
+         * until we finish trying to settle the metadata free space manager(s).
+         */
+        if (inserted_section || allocated_section) {


Why do we need this line?

I believe these checks are actually redundant and can be removed at this point. Previously I was removing the cache entries with expunge_entry and the "free file space" flag, in which case I had two branches with different logic where I didn't want to double free the file space with the call to H5MF_xfree followed by expunge_entry. That caused some problems and needed a switch to remove_entry instead, so I think this line and the other line are no longer relevant.

fortnern · 2025-10-28T22:11:34Z

src/H5FSsection.c

+         * Avoid expunging the entry, as the information needs to be kept around
+         * until we finish trying to settle the metadata free space manager(s).
+         */
+        if (inserted_header || allocated_header) {


Same here, is this one just for the assertion?

fortnern · 2025-10-28T22:24:56Z

Finished reviewing, 2 questions as comments, 2 as replies

jhendersonHDF assigned fortnern, bmribler and mattjala Oct 23, 2025

github-project-automation bot added this to HDF5 - TRIAGE & TRACK Oct 23, 2025

github-project-automation bot moved this to To be triaged in HDF5 - TRIAGE & TRACK Oct 23, 2025

jhendersonHDF commented Oct 23, 2025

View reviewed changes

ellipsis-dev bot reviewed Oct 23, 2025

View reviewed changes

src/H5MF.c Outdated Show resolved Hide resolved

src/H5MF.c Outdated Show resolved Hide resolved

jhendersonHDF commented Oct 23, 2025

View reviewed changes

jhendersonHDF force-pushed the 5577_fixes branch from 13e10f6 to c67c523 Compare October 23, 2025 17:10

jhendersonHDF commented Oct 23, 2025

View reviewed changes

jhendersonHDF force-pushed the 5577_fixes branch from 107376c to ef61da4 Compare October 23, 2025 17:17

Fix several issues uncovered by CVE-2025-7067

ef61da4

jhendersonHDF commented Oct 23, 2025

View reviewed changes

jhendersonHDF moved this from To be triaged to In progress in HDF5 - TRIAGE & TRACK Oct 23, 2025

jhendersonHDF marked this pull request as ready for review October 23, 2025 18:09

jhendersonHDF requested review from bmribler, brtnfld, byrnHDF, derobins, fortnern, glennsong09, lrknox, mattjala, qkoziol and vchoi-hdfgroup as code owners October 23, 2025 18:09

mattjala approved these changes Oct 24, 2025

View reviewed changes

fortnern reviewed Oct 28, 2025

View reviewed changes

nbagha1 added this to the Release 2.0.0 milestone Oct 29, 2025

jhendersonHDF removed this from the Release 2.0.0 milestone Oct 29, 2025

Uh oh!

Fix several issues uncovered by CVE-2025-7067 #5938

Are you sure you want to change the base?

Fix several issues uncovered by CVE-2025-7067 #5938

Uh oh!

Conversation

jhendersonHDF commented Oct 23, 2025 • edited by ellipsis-dev bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fortnern commented Oct 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jhendersonHDF commented Oct 23, 2025 •

edited by ellipsis-dev bot

Loading