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 ); +}