From 9c04d2cd2d5f467e0dc3931f7e5fec1a2b7d54d0 Mon Sep 17 00:00:00 2001 From: Kody Stribrny Date: Tue, 23 Jun 2026 14:17:24 -0700 Subject: [PATCH] fix: Limit GPT headerSize to the sector size GPT sector size is unlimited however the use of 16-bit integers limit a practical change to supporting 65KB sectors. Luckily the most common sector size is 4096. The library should be good for some time. Without this change the code would blindly trust the given header size value even if the sector couldn't possibly be that size. --- .github/.cSpellWords.txt | 1 + ff_ioman.c | 30 +++- test/unit-test/ff_ioman_utest.c | 245 ++++++++++++++++++++++++++++++++ 3 files changed, 274 insertions(+), 2 deletions(-) diff --git a/.github/.cSpellWords.txt b/.github/.cSpellWords.txt index 0845b2e..7967088 100644 --- a/.github/.cSpellWords.txt +++ b/.github/.cSpellWords.txt @@ -167,6 +167,7 @@ TXFIFOF TXFIFOHE TXUNDERR Tibosch +UEFI UNACKED UNRE UNSTUFF diff --git a/ff_ioman.c b/ff_ioman.c index c300fb7..338ab91 100644 --- a/ff_ioman.c +++ b/ff_ioman.c @@ -96,9 +96,12 @@ FF_IOManager_t * FF_CreateIOManager( FF_CreationParameters_t * pxParameters, /* Normally: * ulSectorSize = 512 * ulCacheSize = N x ulSectorSize. */ - if( ( ( usSectorSize % 512 ) != 0 ) || ( usSectorSize == 0 ) ) + if( ( ( usSectorSize % 512 ) != 0 ) || ( usSectorSize == 0 ) || + ( usSectorSize > 0xFFFFU ) ) { - /* ulSectorSize Size not a multiple of 512 or it is zero*/ + /* ulSectorSize is not a multiple of 512, is zero, or is too large to be + * stored in the uint16_t 'usSectorSize' field (which would silently + * truncate, e.g. 65536 -> 0). Fail cleanly instead. */ xError = FF_createERR( FF_ERR_IOMAN_BAD_BLKSIZE, FF_CREATEIOMAN ); } else if( ( ( ulCacheSize % ( uint32_t ) usSectorSize ) != 0 ) || ( ulCacheSize == 0 ) || @@ -1278,6 +1281,12 @@ FF_Error_t FF_PartitionSearch( FF_IOManager_t * pxIOManager, #define FF_GPT_HEAD_CRC 0x10 #define FF_GPT_HEAD_LENGTH 0x0C +/* The UEFI specification fixes the minimum GPT header size at 92 bytes. The + * 'HeaderSize' field is read from the disk and used as the byte count for the + * header CRC, so it must be validated against this minimum and against the + * sector buffer size before use. */ +#define FF_GPT_HEAD_MIN_LENGTH 92U + static FF_Error_t FF_GetEfiPartitionEntry( FF_IOManager_t * pxIOManager, uint32_t ulPartitionNumber ) { @@ -1328,6 +1337,23 @@ static FF_Error_t FF_GetEfiPartitionEntry( FF_IOManager_t * pxIOManager, ulGPTHeadCRC = FF_getLong( pxBuffer->pucBuffer, FF_GPT_HEAD_CRC ); ulGPTHeadLength = FF_getLong( pxBuffer->pucBuffer, FF_GPT_HEAD_LENGTH ); + /* 'ulGPTHeadLength' is read from the (untrusted) GPT header and is used + * below as the byte count for FF_GetCRC32() over the sector buffer. The + * buffer is exactly one sector (pxIOManager->usSectorSize bytes). + */ + if( ( ulGPTHeadLength < FF_GPT_HEAD_MIN_LENGTH ) || + ( ulGPTHeadLength > pxIOManager->usSectorSize ) ) + { + xError = FF_ReleaseBuffer( pxIOManager, pxBuffer ); + + if( FF_isERR( xError ) == pdFALSE ) + { + xError = FF_createERR( FF_ERR_IOMAN_INVALID_FORMAT, FF_GETEFIPARTITIONENTRY ); + } + + break; + } + /* Calculate Head CRC */ /* Blank CRC field */ FF_putLong( pxBuffer->pucBuffer, FF_GPT_HEAD_CRC, 0x00000000 ); diff --git a/test/unit-test/ff_ioman_utest.c b/test/unit-test/ff_ioman_utest.c index 330c998..178fc5b 100644 --- a/test/unit-test/ff_ioman_utest.c +++ b/test/unit-test/ff_ioman_utest.c @@ -348,3 +348,248 @@ void test_PartitionSearch_self_referential_chain_terminates( void ) ( void ) FF_DeleteIOManager( pxIOManager ); } + +/*-----------------------------------------------------------*/ +/* GPT header validation in FF_GetEfiPartitionEntry(). */ +/* */ +/* FF_GetEfiPartitionEntry() is static; it is reached through */ +/* the public FF_Mount() API when the MBR advertises a GPT */ +/* protective partition (type 0xEE). The 'HeaderSize' field */ +/* (offset 0x0C) is disk-controlled and is used as the byte */ +/* count for FF_GetCRC32() over the one-sector header buffer, */ +/* so it must be bounded to [92, usSectorSize] before use. */ +/* */ +/* FF_GetCRC32() is a mock: in the rejection tests it is left */ +/* without any expectation, so CMock fails the test if the */ +/* out-of-bounds CRC call is ever reached. */ +/*-----------------------------------------------------------*/ + +/* GPT byte offsets within the header sector (mirror of the private + * definitions in ff_ioman.c). */ +#define GPT_OFF_HEAD_LENGTH ( 0x0CU ) +#define GPT_OFF_HEAD_CRC ( 0x10U ) +#define GPT_OFF_HEAD_PART_ENTRY_LBA ( 0x48U ) +#define GPT_OFF_HEAD_ENTRY_SIZE ( 0x54U ) + +#define GPT_PROTECTIVE_PART_ID ( 0xEEU ) +#define GPT_HEADER_SECTOR ( 1U ) + +/* UEFI-mandated minimum GPT header size, mirrors FF_GPT_HEAD_MIN_LENGTH in + * ff_ioman.c. */ +#define GPT_HEAD_MIN_LENGTH ( 92U ) + +static FF_Disk_t xTestDisk; + +/* + * Lay out an MBR (sector 0) that advertises a single GPT protective partition + * pointing at a GPT header sector, and write a GPT header there with a caller + * supplied 'HeaderSize' and header CRC. + */ +static void prvBuildGptDisk( uint32_t ulHeaderLength, + uint32_t ulHeaderCrc ) +{ + uint8_t * pucMbr; + uint8_t * pucHeader; + + memset( ucVirtualDisk, 0, sizeof( ucVirtualDisk ) ); + + /* MBR: one non-extended protective partition (type 0xEE). */ + pucMbr = prvSector( 0 ); + prvSetPartEntry( pucMbr, 0, 0x00, GPT_PROTECTIVE_PART_ID, + GPT_HEADER_SECTOR, TEST_DISK_SECTORS - GPT_HEADER_SECTOR ); + prvSetSignature( pucMbr ); + + /* GPT header sector. */ + pucHeader = prvSector( GPT_HEADER_SECTOR ); + memcpy( pucHeader, "EFI PART", 8 ); + prvPutLong( pucHeader, GPT_OFF_HEAD_LENGTH, ulHeaderLength ); + prvPutLong( pucHeader, GPT_OFF_HEAD_CRC, ulHeaderCrc ); + /* Only consulted if the header passes validation. */ + prvPutLong( pucHeader, GPT_OFF_HEAD_ENTRY_SIZE, 128U ); + prvPutLong( pucHeader, GPT_OFF_HEAD_PART_ENTRY_LBA, GPT_HEADER_SECTOR + 1U ); +} + +static FF_IOManager_t * prvCreateMountableDisk( void ) +{ + FF_IOManager_t * pxIOManager = prvCreateTestIOManager(); + + memset( &xTestDisk, 0, sizeof( xTestDisk ) ); + xTestDisk.pxIOManager = pxIOManager; + + return pxIOManager; +} + +/* + * A 'HeaderSize' far larger than the sector buffer (0xFFFFFFFF, the value the + * PoC sets) must be rejected as an invalid format, and the out-of-bounds + * FF_GetCRC32() over the header buffer must never be reached. + */ +void test_Mount_gpt_header_length_oversized_is_rejected( void ) +{ + FF_IOManager_t * pxIOManager; + FF_Error_t xError; + + prvBuildGptDisk( 0xFFFFFFFFU, 0x12345678U ); + + pxIOManager = prvCreateMountableDisk(); + TEST_ASSERT_NOT_NULL( pxIOManager ); + + xError = FF_Mount( &xTestDisk, 0 ); + + TEST_ASSERT_TRUE( FF_isERR( xError ) ); + TEST_ASSERT_EQUAL_HEX32( FF_ERR_IOMAN_INVALID_FORMAT, FF_GETERROR( xError ) ); + + ( void ) FF_DeleteIOManager( pxIOManager ); +} + +/* + * Boundary: 'HeaderSize' one byte past the sector size must be rejected and + * must not reach the CRC. + */ +void test_Mount_gpt_header_length_one_past_sector_is_rejected( void ) +{ + FF_IOManager_t * pxIOManager; + FF_Error_t xError; + + prvBuildGptDisk( TEST_SECTOR_SIZE + 1U, 0x12345678U ); + + pxIOManager = prvCreateMountableDisk(); + TEST_ASSERT_NOT_NULL( pxIOManager ); + + xError = FF_Mount( &xTestDisk, 0 ); + + TEST_ASSERT_TRUE( FF_isERR( xError ) ); + TEST_ASSERT_EQUAL_HEX32( FF_ERR_IOMAN_INVALID_FORMAT, FF_GETERROR( xError ) ); + + ( void ) FF_DeleteIOManager( pxIOManager ); +} + +/* + * Boundary: 'HeaderSize' below the UEFI minimum (92 bytes) must be rejected and + * must not reach the CRC. + */ +void test_Mount_gpt_header_length_below_minimum_is_rejected( void ) +{ + FF_IOManager_t * pxIOManager; + FF_Error_t xError; + + prvBuildGptDisk( 91U, 0x12345678U ); + + pxIOManager = prvCreateMountableDisk(); + TEST_ASSERT_NOT_NULL( pxIOManager ); + + xError = FF_Mount( &xTestDisk, 0 ); + + TEST_ASSERT_TRUE( FF_isERR( xError ) ); + TEST_ASSERT_EQUAL_HEX32( FF_ERR_IOMAN_INVALID_FORMAT, FF_GETERROR( xError ) ); + + ( void ) FF_DeleteIOManager( pxIOManager ); +} + +/* + * A 'HeaderSize' at the UEFI minimum (92 bytes) is in range, so the bounds + * check passes and the CRC is computed. With a mismatching (mocked) CRC the + * header is reported corrupt - proving the valid-length branch proceeds to the + * CRC instead of being rejected. + */ +void test_Mount_gpt_header_length_at_minimum_reaches_crc( void ) +{ + FF_IOManager_t * pxIOManager; + FF_Error_t xError; + + prvBuildGptDisk( GPT_HEAD_MIN_LENGTH, 0x12345678U ); + + /* In-range length: the CRC routine is reached exactly once. The mocked + * result differs from the stored header CRC, so the header is "corrupt". */ + FF_GetCRC32_ExpectAnyArgsAndReturn( 0xDEADBEEFU ); + + pxIOManager = prvCreateMountableDisk(); + TEST_ASSERT_NOT_NULL( pxIOManager ); + + xError = FF_Mount( &xTestDisk, 0 ); + + TEST_ASSERT_TRUE( FF_isERR( xError ) ); + TEST_ASSERT_EQUAL_HEX32( FF_ERR_IOMAN_GPT_HEADER_CORRUPT, FF_GETERROR( xError ) ); + + ( void ) FF_DeleteIOManager( pxIOManager ); +} + +/* + * Boundary: 'HeaderSize' exactly equal to the sector size is the largest + * in-range value, so it must also reach the CRC rather than be rejected. + */ +void test_Mount_gpt_header_length_at_sector_size_reaches_crc( void ) +{ + FF_IOManager_t * pxIOManager; + FF_Error_t xError; + + prvBuildGptDisk( TEST_SECTOR_SIZE, 0x12345678U ); + + FF_GetCRC32_ExpectAnyArgsAndReturn( 0xDEADBEEFU ); + + pxIOManager = prvCreateMountableDisk(); + TEST_ASSERT_NOT_NULL( pxIOManager ); + + xError = FF_Mount( &xTestDisk, 0 ); + + TEST_ASSERT_TRUE( FF_isERR( xError ) ); + TEST_ASSERT_EQUAL_HEX32( FF_ERR_IOMAN_GPT_HEADER_CORRUPT, FF_GETERROR( xError ) ); + + ( void ) FF_DeleteIOManager( pxIOManager ); +} + +/*-----------------------------------------------------------*/ +/* Sector-size validation in FF_CreateIOManager(). */ +/*-----------------------------------------------------------*/ + +/* + * A sector size that does not fit in the uint16_t 'usSectorSize' field (e.g. + * 65536, which is a multiple of 512 and so passes the old check) would + * truncate to 0. It must instead fail cleanly with FF_ERR_IOMAN_BAD_BLKSIZE + * and return NULL. + */ +void test_CreateIOManager_rejects_sector_size_above_uint16( void ) +{ + FF_CreationParameters_t xParameters; + FF_Error_t xError = FF_ERR_NONE; + FF_IOManager_t * pxIOManager; + + memset( &xParameters, 0, sizeof( xParameters ) ); + xParameters.ulSectorSize = 0x10000; /* 65536 -> 0 when truncated. */ + xParameters.ulMemorySize = 2U * 0x10000; /* Two sectors of cache. */ + xParameters.fnReadBlocks = prvReadBlocks; + xParameters.fnWriteBlocks = prvWriteBlocks; + xParameters.xBlockDeviceIsReentrant = pdTRUE; + + pxIOManager = FF_CreateIOManager( &xParameters, &xError ); + + TEST_ASSERT_NULL( pxIOManager ); + TEST_ASSERT_TRUE( FF_isERR( xError ) ); + TEST_ASSERT_EQUAL_HEX32( FF_ERR_IOMAN_BAD_BLKSIZE, FF_GETERROR( xError ) ); +} + +/* + * Sector sizes larger than 512 but within the uint16_t range (e.g. 4096) remain + * valid: the upper-bound check must not reject legitimate large sectors. + */ +void test_CreateIOManager_accepts_4096_byte_sector( void ) +{ + FF_CreationParameters_t xParameters; + FF_Error_t xError = FF_ERR_NONE; + FF_IOManager_t * pxIOManager; + + memset( &xParameters, 0, sizeof( xParameters ) ); + xParameters.ulSectorSize = 4096; + xParameters.ulMemorySize = 8U * 4096U; + xParameters.fnReadBlocks = prvReadBlocks; + xParameters.fnWriteBlocks = prvWriteBlocks; + xParameters.xBlockDeviceIsReentrant = pdTRUE; + + pxIOManager = FF_CreateIOManager( &xParameters, &xError ); + + TEST_ASSERT_NOT_NULL( pxIOManager ); + TEST_ASSERT_FALSE( FF_isERR( xError ) ); + TEST_ASSERT_EQUAL_UINT16( 4096U, pxIOManager->usSectorSize ); + + ( void ) FF_DeleteIOManager( pxIOManager ); +}