From f8998bdd485e587d5f4d4ddcaaf9785809f3da5b Mon Sep 17 00:00:00 2001 From: Andreas Nordal Date: Wed, 1 May 2024 14:57:44 +0200 Subject: [PATCH] [1] [2] [2] FreeRTOS_DHCP_utest: Fix systematic memory errors in test input The trivial problem: Screenfuls of misaligned store warnings: Store to misaligned address 0x7ffe* for type 'uint32_t', which requires 4 byte alignment I assume there is no reason not to convert these to memcpy. I have done that when that was the only problem I saw. The more involved problem: The length fields, buffer sizes and stop byte (0xFF) offsets have fallen out of maintenance. This would lead the DHCP option parser under test to read out of bounds when it overshot the stop byte. Symptom: AddressSanitizer: dynamic-stack-buffer-overflow on address 0x7ffc9dfa5c07 READ of size 1 at 0x7ffc9dfa5c07 thread T0 #0 0x459a49 in prvProcessDHCPReplies test/unit-test/build/Annexed_TCP_Sources/FreeRTOS_DHCP.c:1310 #1 0x4526d2 in vHandleWaitingAcknowledge test/unit-test/build/Annexed_TCP_Sources/FreeRTOS_DHCP.c:495 #2 0x4544ef in vDHCPProcessEndPoint test/unit-test/build/Annexed_TCP_Sources/FreeRTOS_DHCP.c:739 #3 0x43dbd9 in test_vDHCPProcess_eWaitingAcknowledge_DNSIncorrectLength2 (test/unit-test/build/bin/tests/FreeRTOS_DHCP_utest+0x43dbd9) Address 0x7ffc9dfa5c07 is located in stack of thread T0 SUMMARY: AddressSanitizer: dynamic-stack-buffer-overflow test/unit-test/build/Annexed_TCP_Sources/FreeRTOS_DHCP.c:1310 in prvProcessDHCPReplies Shadow bytes around the buggy address: 0x7ffc9dfa5980: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7ffc9dfa5a00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7ffc9dfa5a80: 00 00 00 00 00 00 00 00 ca ca ca ca 00 00 00 00 0x7ffc9dfa5b00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7ffc9dfa5b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x7ffc9dfa5c00:[05]cb cb cb cb cb cb cb 00 00 00 00 00 00 00 00 0x7ffc9dfa5c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7ffc9dfa5d00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7ffc9dfa5d80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7ffc9dfa5e00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7ffc9dfa5e80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Left alloca redzone: ca Right alloca redzone: cb I think this calls for some helper functions, which I've added. These eliminate the possibility for such inconsistencies, drastically cuts down on low-entropy code, and by wrapping memcpy, also avoids the first problem. I have done that instead wherever I also saw this kind of inconsistencies. --- .../FreeRTOS_DHCP/FreeRTOS_DHCP_utest.c | 168 ++++-------------- 1 file changed, 30 insertions(+), 138 deletions(-) diff --git a/test/unit-test/FreeRTOS_DHCP/FreeRTOS_DHCP_utest.c b/test/unit-test/FreeRTOS_DHCP/FreeRTOS_DHCP_utest.c index 08cfddf2e..610d4974b 100644 --- a/test/unit-test/FreeRTOS_DHCP/FreeRTOS_DHCP_utest.c +++ b/test/unit-test/FreeRTOS_DHCP/FreeRTOS_DHCP_utest.c @@ -4879,22 +4879,15 @@ void test_vDHCPProcess_eWaitingAcknowledge_SubnetMaskIncorrectLength( void ) const BaseType_t xTotalLength = sizeof( struct xDHCPMessage_IPv4 ) + 1U /* Padding */ + 3U /* DHCP offer */ + 6U /* Server IP address */ - + 6U /* Subnet Mask */ - + 6U /* Gateway */ - + 6U /* Lease time */ + + 5U /* Subnet Mask, truncated */ + 1U /* End */; uint8_t DHCPMsg[ xTotalLength ]; uint32_t DHCPServerAddress = 0xC0A80001; /* 192.168.0.1 */ uint32_t ulClientIPAddress = 0xC0A8000A; /* 192.168.0.10 */ uint32_t ulSubnetMask = 0xFFFFF100; /* 255.255.241.0 */ - uint32_t ulGateway = 0xC0A80001; /* 192.168.0.1 */ - uint32_t ulLeaseTime = 0x00000096; /* 150 seconds */ DHCPMessage_IPv4_t * pxDHCPMessage = ( DHCPMessage_IPv4_t * ) DHCPMsg; NetworkEndPoint_t xEndPoint = { 0 }, * pxEndPoint = &xEndPoint; - DHCPMsg[ xTotalLength - 1U ] = 0xFF; - - /* Set the header - or at least the start of DHCP message. */ memset( DHCPMsg, 0, sizeof( DHCPMsg ) ); /* Copy the header here. */ @@ -4910,28 +4903,13 @@ void test_vDHCPProcess_eWaitingAcknowledge_SubnetMaskIncorrectLength( void ) /* Leave one byte for the padding. */ uint8_t * DHCPOption = &DHCPMsg[ sizeof( struct xDHCPMessage_IPv4 ) + 1 ]; - /* Add Message type code. */ - DHCPOption[ 0 ] = dhcpIPv4_MESSAGE_TYPE_OPTION_CODE; - /* Add length. */ - DHCPOption[ 1 ] = 1; - /* Add the offer byte. */ - DHCPOption[ 2 ] = dhcpMESSAGE_TYPE_ACK; - DHCPOption += 4; - /* Add Message type code. */ - DHCPOption[ 0 ] = dhcpIPv4_SERVER_IP_ADDRESS_OPTION_CODE; - /* Add length. */ - DHCPOption[ 1 ] = 4; - /* Add the offer byte. */ - *( ( uint32_t * ) &DHCPOption[ 2 ] ) = DHCPServerAddress; + prvWriteDHCPOptionU8( &DHCPOption, dhcpIPv4_MESSAGE_TYPE_OPTION_CODE, dhcpMESSAGE_TYPE_ACK ); + prvWriteDHCPOptionU32( &DHCPOption, dhcpIPv4_SERVER_IP_ADDRESS_OPTION_CODE, DHCPServerAddress ); + prvWriteDHCPOption( &DHCPOption, dhcpIPv4_SUBNET_MASK_OPTION_CODE, &ulSubnetMask, 3 ); - DHCPOption += 6; - /* Add Message type code. */ - DHCPOption[ 0 ] = dhcpIPv4_SUBNET_MASK_OPTION_CODE; - /* Add incorrect length. */ - DHCPOption[ 1 ] = 3; - /* Add the offer byte. */ - *( ( uint32_t * ) &DHCPOption[ 2 ] ) = ulSubnetMask; + *DHCPOption++ = 0xFF; + TEST_ASSERT_EQUAL( DHCPOption - DHCPMsg, xTotalLength ); /* Put the information in global variables to be returned by * the FreeRTOS_recvrom. */ @@ -4997,20 +4975,15 @@ void test_vDHCPProcess_eWaitingAcknowledge_GatewayIncorrectLength( void ) + 6U /* Server IP address */ + 6U /* Subnet Mask */ + 6U /* Gateway */ - + 6U /* Lease time */ + 1U /* End */; uint8_t DHCPMsg[ xTotalLength ]; uint32_t DHCPServerAddress = 0xC0A80001; /* 192.168.0.1 */ uint32_t ulClientIPAddress = 0xC0A8000A; /* 192.168.0.10 */ uint32_t ulSubnetMask = 0xFFFFF100; /* 255.255.241.0 */ uint32_t ulGateway = 0xC0A80001; /* 192.168.0.1 */ - uint32_t ulLeaseTime = 0x00000096; /* 150 seconds */ DHCPMessage_IPv4_t * pxDHCPMessage = ( DHCPMessage_IPv4_t * ) DHCPMsg; NetworkEndPoint_t xEndPoint = { 0 }, * pxEndPoint = &xEndPoint; - DHCPMsg[ xTotalLength - 1U ] = 0xFF; - - /* Set the header - or at least the start of DHCP message. */ memset( DHCPMsg, 0, sizeof( DHCPMsg ) ); /* Copy the header here. */ @@ -5026,36 +4999,21 @@ void test_vDHCPProcess_eWaitingAcknowledge_GatewayIncorrectLength( void ) /* Leave one byte for the padding. */ uint8_t * DHCPOption = &DHCPMsg[ sizeof( struct xDHCPMessage_IPv4 ) + 1 ]; - /* Add Message type code. */ - DHCPOption[ 0 ] = dhcpIPv4_MESSAGE_TYPE_OPTION_CODE; - /* Add length. */ - DHCPOption[ 1 ] = 1; - /* Add the offer byte. */ - DHCPOption[ 2 ] = dhcpMESSAGE_TYPE_ACK; - DHCPOption += 4; - /* Add Message type code. */ - DHCPOption[ 0 ] = dhcpIPv4_SERVER_IP_ADDRESS_OPTION_CODE; - /* Add length. */ - DHCPOption[ 1 ] = 4; - /* Add the offer byte. */ - *( ( uint32_t * ) &DHCPOption[ 2 ] ) = DHCPServerAddress; - - DHCPOption += 6; - /* Add Message type code. */ - DHCPOption[ 0 ] = dhcpIPv4_SUBNET_MASK_OPTION_CODE; - /* Add length. */ - DHCPOption[ 1 ] = 4; - /* Add the offer byte. */ - *( ( uint32_t * ) &DHCPOption[ 2 ] ) = ulSubnetMask; + prvWriteDHCPOptionU8( &DHCPOption, dhcpIPv4_MESSAGE_TYPE_OPTION_CODE, dhcpMESSAGE_TYPE_ACK ); + prvWriteDHCPOptionU32( &DHCPOption, dhcpIPv4_SERVER_IP_ADDRESS_OPTION_CODE, DHCPServerAddress ); + prvWriteDHCPOptionU32( &DHCPOption, dhcpIPv4_SUBNET_MASK_OPTION_CODE, ulSubnetMask ); - DHCPOption += 6; /* Add Message type code. */ DHCPOption[ 0 ] = dhcpIPv4_GATEWAY_OPTION_CODE; /* Add incorrect length. */ DHCPOption[ 1 ] = 2; /* Add the offer byte. */ - *( ( uint32_t * ) &DHCPOption[ 2 ] ) = ulGateway; + memcpy( &DHCPOption[ 2 ], &ulGateway, sizeof(ulGateway) ); + DHCPOption += 6; + + *DHCPOption++ = 0xFF; + TEST_ASSERT_EQUAL( DHCPOption - DHCPMsg, xTotalLength ); /* Put the information in global variables to be returned by * the FreeRTOS_recvrom. */ @@ -5122,7 +5080,7 @@ void test_vDHCPProcess_eWaitingAcknowledge_LeaseTimeIncorrectLength( void ) + 6U /* Server IP address */ + 6U /* Subnet Mask */ + 6U /* Gateway */ - + 6U /* Lease time */ + + 5U /* Lease time, truncated */ + 1U /* End */; uint8_t DHCPMsg[ xTotalLength ]; uint32_t DHCPServerAddress = 0xC0A80001; /* 192.168.0.1 */ @@ -5133,9 +5091,6 @@ void test_vDHCPProcess_eWaitingAcknowledge_LeaseTimeIncorrectLength( void ) DHCPMessage_IPv4_t * pxDHCPMessage = ( DHCPMessage_IPv4_t * ) DHCPMsg; NetworkEndPoint_t xEndPoint = { 0 }, * pxEndPoint = &xEndPoint; - DHCPMsg[ xTotalLength - 1U ] = 0xFF; - - /* Set the header - or at least the start of DHCP message. */ memset( DHCPMsg, 0, sizeof( DHCPMsg ) ); /* Copy the header here. */ @@ -5151,45 +5106,15 @@ void test_vDHCPProcess_eWaitingAcknowledge_LeaseTimeIncorrectLength( void ) /* Leave one byte for the padding. */ uint8_t * DHCPOption = &DHCPMsg[ sizeof( struct xDHCPMessage_IPv4 ) + 1 ]; - /* Add Message type code. */ - DHCPOption[ 0 ] = dhcpIPv4_MESSAGE_TYPE_OPTION_CODE; - /* Add length. */ - DHCPOption[ 1 ] = 1; - /* Add the offer byte. */ - DHCPOption[ 2 ] = dhcpMESSAGE_TYPE_ACK; - DHCPOption += 4; - /* Add Message type code. */ - DHCPOption[ 0 ] = dhcpIPv4_SERVER_IP_ADDRESS_OPTION_CODE; - /* Add length. */ - DHCPOption[ 1 ] = 4; - /* Add the offer byte. */ - *( ( uint32_t * ) &DHCPOption[ 2 ] ) = DHCPServerAddress; - - DHCPOption += 6; - /* Add Message type code. */ - DHCPOption[ 0 ] = dhcpIPv4_SUBNET_MASK_OPTION_CODE; - /* Add length. */ - DHCPOption[ 1 ] = 4; - /* Add the offer byte. */ - *( ( uint32_t * ) &DHCPOption[ 2 ] ) = ulSubnetMask; - - DHCPOption += 6; - /* Add Message type code. */ - DHCPOption[ 0 ] = dhcpIPv4_GATEWAY_OPTION_CODE; - /* Add length. */ - DHCPOption[ 1 ] = 4; - /* Add the offer byte. */ - *( ( uint32_t * ) &DHCPOption[ 2 ] ) = ulGateway; - - DHCPOption += 6; - /* Add Message type code. */ - DHCPOption[ 0 ] = dhcpIPv4_LEASE_TIME_OPTION_CODE; - /* Add incorrect length. */ - DHCPOption[ 1 ] = 3; - /* Add the offer byte. */ - *( ( uint32_t * ) &DHCPOption[ 2 ] ) = ulLeaseTime; + prvWriteDHCPOptionU8( &DHCPOption, dhcpIPv4_MESSAGE_TYPE_OPTION_CODE, dhcpMESSAGE_TYPE_ACK ); + prvWriteDHCPOptionU32( &DHCPOption, dhcpIPv4_SERVER_IP_ADDRESS_OPTION_CODE, DHCPServerAddress ); + prvWriteDHCPOptionU32( &DHCPOption, dhcpIPv4_SUBNET_MASK_OPTION_CODE, ulSubnetMask ); + prvWriteDHCPOptionU32( &DHCPOption, dhcpIPv4_GATEWAY_OPTION_CODE, ulGateway ); + prvWriteDHCPOption( &DHCPOption, dhcpIPv4_LEASE_TIME_OPTION_CODE, &ulLeaseTime, 3 ); + *DHCPOption++ = 0xFF; + TEST_ASSERT_EQUAL( DHCPOption - DHCPMsg, xTotalLength ); /* Put the information in global variables to be returned by * the FreeRTOS_recvrom. */ @@ -5255,7 +5180,7 @@ void test_vDHCPProcess_eWaitingAcknowledge_LeaseTimeIncorrectLength2( void ) + 6U /* Server IP address */ + 6U /* Subnet Mask */ + 6U /* Gateway */ - + 6U /* Lease time */ + + 5U /* Lease time */ + 1U /* End */; uint8_t DHCPMsg[ xTotalLength ]; uint32_t DHCPServerAddress = 0xC0A80001; /* 192.168.0.1 */ @@ -5266,9 +5191,6 @@ void test_vDHCPProcess_eWaitingAcknowledge_LeaseTimeIncorrectLength2( void ) DHCPMessage_IPv4_t * pxDHCPMessage = ( DHCPMessage_IPv4_t * ) DHCPMsg; NetworkEndPoint_t xEndPoint = { 0 }, * pxEndPoint = &xEndPoint; - DHCPMsg[ xTotalLength - 1U ] = 0xFF; - - /* Set the header - or at least the start of DHCP message. */ memset( DHCPMsg, 0, sizeof( DHCPMsg ) ); /* Copy the header here. */ @@ -5284,45 +5206,15 @@ void test_vDHCPProcess_eWaitingAcknowledge_LeaseTimeIncorrectLength2( void ) /* Leave one byte for the padding. */ uint8_t * DHCPOption = &DHCPMsg[ sizeof( struct xDHCPMessage_IPv4 ) + 1 ]; - /* Add Message type code. */ - DHCPOption[ 0 ] = dhcpIPv4_MESSAGE_TYPE_OPTION_CODE; - /* Add length. */ - DHCPOption[ 1 ] = 1; - /* Add the offer byte. */ - DHCPOption[ 2 ] = dhcpMESSAGE_TYPE_ACK; - - DHCPOption += 4; - /* Add Message type code. */ - DHCPOption[ 0 ] = dhcpIPv4_SERVER_IP_ADDRESS_OPTION_CODE; - /* Add length. */ - DHCPOption[ 1 ] = 4; - /* Add the offer byte. */ - *( ( uint32_t * ) &DHCPOption[ 2 ] ) = DHCPServerAddress + 0x1234; - - DHCPOption += 6; - /* Add Message type code. */ - DHCPOption[ 0 ] = dhcpIPv4_SUBNET_MASK_OPTION_CODE; - /* Add length. */ - DHCPOption[ 1 ] = 4; - /* Add the offer byte. */ - *( ( uint32_t * ) &DHCPOption[ 2 ] ) = ulSubnetMask; - DHCPOption += 6; - /* Add Message type code. */ - DHCPOption[ 0 ] = dhcpIPv4_GATEWAY_OPTION_CODE; - /* Add length. */ - DHCPOption[ 1 ] = 4; - /* Add the offer byte. */ - *( ( uint32_t * ) &DHCPOption[ 2 ] ) = ulGateway; - - DHCPOption += 6; - /* Add Message type code. */ - DHCPOption[ 0 ] = dhcpIPv4_LEASE_TIME_OPTION_CODE; - /* Add incorrect length. */ - DHCPOption[ 1 ] = 3; - /* Add the offer byte. */ - *( ( uint32_t * ) &DHCPOption[ 2 ] ) = ulLeaseTime; + prvWriteDHCPOptionU8( &DHCPOption, dhcpIPv4_MESSAGE_TYPE_OPTION_CODE, dhcpMESSAGE_TYPE_ACK ); + prvWriteDHCPOptionU32( &DHCPOption, dhcpIPv4_SERVER_IP_ADDRESS_OPTION_CODE, DHCPServerAddress + 0x1234 ); + prvWriteDHCPOptionU32( &DHCPOption, dhcpIPv4_SUBNET_MASK_OPTION_CODE, ulSubnetMask ); + prvWriteDHCPOptionU32( &DHCPOption, dhcpIPv4_GATEWAY_OPTION_CODE, ulGateway ); + prvWriteDHCPOption( &DHCPOption, dhcpIPv4_LEASE_TIME_OPTION_CODE, &ulLeaseTime, 3 ); + *DHCPOption++ = 0xFF; + TEST_ASSERT_EQUAL( DHCPOption - DHCPMsg, xTotalLength ); /* Put the information in global variables to be returned by * the FreeRTOS_recvrom. */