From 3edc0001e6fe8989d2987769c14866de372ae068 Mon Sep 17 00:00:00 2001 From: Michael Cardell Widerkrantz Date: Thu, 10 Apr 2025 16:20:44 +0200 Subject: [PATCH] fw: Fix splint complaints - Make splint work on current code. - Check error return values. - Cast to void when not caring about result. - Declare internal functions to be static. --- hw/application_fpga/Makefile | 8 ++++- hw/application_fpga/fw/tk1/flash.c | 36 +++++++++++--------- hw/application_fpga/fw/tk1/flash.h | 3 -- hw/application_fpga/fw/tk1/main.c | 4 +-- hw/application_fpga/fw/tk1/partition_table.c | 18 ++++++---- hw/application_fpga/fw/tk1/preload_app.c | 16 +++++---- hw/application_fpga/fw/tk1/spi.c | 8 ++++- hw/application_fpga/fw/tk1/spi.h | 1 - hw/application_fpga/fw/tk1/storage.c | 18 ++++++---- hw/application_fpga/fw/tk1/syscall_handler.c | 3 +- 10 files changed, 71 insertions(+), 44 deletions(-) diff --git a/hw/application_fpga/Makefile b/hw/application_fpga/Makefile index fa715e0..e7ab9f4 100644 --- a/hw/application_fpga/Makefile +++ b/hw/application_fpga/Makefile @@ -222,7 +222,7 @@ check: .PHONY: splint splint: splint \ - -nolib \ + +unixlib \ -predboolint \ +boolint \ -nullpass \ @@ -233,6 +233,12 @@ splint: -unreachable \ -unqualifiedtrans \ -fullinitblock \ + +gnuextensions \ + -fixedformalarray \ + -mustfreeonly \ + -I $(LIBDIR)/include \ + -I $(LIBDIR) \ + -I $(LIBDIR)/blake2s \ $(CHECK_SOURCES) testfw.elf: tkey-libs $(TESTFW_OBJS) $(P)/fw/tk1/firmware.lds diff --git a/hw/application_fpga/fw/tk1/flash.c b/hw/application_fpga/fw/tk1/flash.c index d2dffe5..cdbcf94 100644 --- a/hw/application_fpga/fw/tk1/flash.c +++ b/hw/application_fpga/fw/tk1/flash.c @@ -21,6 +21,10 @@ static volatile uint32_t *timer_ctrl = (volatile uint32_t *)TK1_MMIO_TIMER_CTRL #define CPUFREQ 21000000 #define PAGE_SIZE 256 +static bool flash_is_busy(void); +static void flash_wait_busy(void); +static void flash_write_enable(void); + static void delay(int timeout_ms) { // Tick once every centisecond @@ -36,12 +40,12 @@ static void delay(int timeout_ms) *timer_ctrl |= (1 << TK1_MMIO_TIMER_CTRL_STOP_BIT); } -bool flash_is_busy(void) +static bool flash_is_busy(void) { uint8_t tx_buf = READ_STATUS_REG_1; uint8_t rx_buf = {0x00}; - spi_transfer(&tx_buf, sizeof(tx_buf), NULL, 0, &rx_buf, sizeof(rx_buf)); + assert(spi_transfer(&tx_buf, sizeof(tx_buf), NULL, 0, &rx_buf, sizeof(rx_buf)) == 0); if (rx_buf & (1 << STATUS_REG_BUSY_BIT)) { return true; @@ -51,25 +55,25 @@ bool flash_is_busy(void) } // Blocking until !busy -void flash_wait_busy(void) +static void flash_wait_busy(void) { while (flash_is_busy()) { delay(10); } } -void flash_write_enable(void) +static void flash_write_enable(void) { uint8_t tx_buf = WRITE_ENABLE; - spi_transfer(&tx_buf, sizeof(tx_buf), NULL, 0, NULL, 0); + assert(spi_transfer(&tx_buf, sizeof(tx_buf), NULL, 0, NULL, 0) == 0); } void flash_write_disable(void) { uint8_t tx_buf = WRITE_DISABLE; - spi_transfer(&tx_buf, sizeof(tx_buf), NULL, 0, NULL, 0); + assert(spi_transfer(&tx_buf, sizeof(tx_buf), NULL, 0, NULL, 0) == 0); } void flash_sector_erase(uint32_t address) @@ -81,7 +85,7 @@ void flash_sector_erase(uint32_t address) /* tx_buf[3] is within a sector, and hence does not make a difference */ flash_write_enable(); - spi_transfer(tx_buf, sizeof(tx_buf), NULL, 0, NULL, 0); + assert(spi_transfer(tx_buf, sizeof(tx_buf), NULL, 0, NULL, 0) == 0); flash_wait_busy(); } @@ -94,7 +98,7 @@ void flash_block_32_erase(uint32_t address) tx_buf[3] = (address >> ADDR_BYTE_1_BIT) & 0xFF; flash_write_enable(); - spi_transfer(tx_buf, sizeof(tx_buf), NULL, 0, NULL, 0); + assert(spi_transfer(tx_buf, sizeof(tx_buf), NULL, 0, NULL, 0) == 0); flash_wait_busy(); } @@ -108,7 +112,7 @@ void flash_block_64_erase(uint32_t address) * difference */ flash_write_enable(); - spi_transfer(tx_buf, sizeof(tx_buf), NULL, 0, NULL, 0); + assert(spi_transfer(tx_buf, sizeof(tx_buf), NULL, 0, NULL, 0) == 0); flash_wait_busy(); } @@ -117,14 +121,14 @@ void flash_release_powerdown(void) uint8_t tx_buf[4] = {0x00}; tx_buf[0] = RELEASE_POWER_DOWN; - spi_transfer(tx_buf, sizeof(tx_buf), NULL, 0, NULL, 0); + assert(spi_transfer(tx_buf, sizeof(tx_buf), NULL, 0, NULL, 0) == 0); } void flash_powerdown(void) { uint8_t tx_buf = POWER_DOWN; - spi_transfer(&tx_buf, sizeof(tx_buf), NULL, 0, NULL, 0); + assert(spi_transfer(&tx_buf, sizeof(tx_buf), NULL, 0, NULL, 0) == 0); } void flash_read_manufacturer_device_id(uint8_t *device_id) @@ -134,7 +138,7 @@ void flash_read_manufacturer_device_id(uint8_t *device_id) uint8_t tx_buf[4] = {0x00}; tx_buf[0] = READ_MANUFACTURER_ID; - spi_transfer(tx_buf, sizeof(tx_buf), NULL, 0, device_id, 2); + assert(spi_transfer(tx_buf, sizeof(tx_buf), NULL, 0, device_id, 2) == 0); } void flash_read_jedec_id(uint8_t *jedec_id) @@ -143,7 +147,7 @@ void flash_read_jedec_id(uint8_t *jedec_id) uint8_t tx_buf = READ_JEDEC_ID; - spi_transfer(&tx_buf, sizeof(tx_buf), NULL, 0, jedec_id, 3); + assert(spi_transfer(&tx_buf, sizeof(tx_buf), NULL, 0, jedec_id, 3) == 0); } void flash_read_unique_id(uint8_t *unique_id) @@ -153,7 +157,7 @@ void flash_read_unique_id(uint8_t *unique_id) uint8_t tx_buf[5] = {0x00}; tx_buf[0] = READ_UNIQUE_ID; - spi_transfer(tx_buf, sizeof(tx_buf), NULL, 0, unique_id, 8); + assert(spi_transfer(tx_buf, sizeof(tx_buf), NULL, 0, unique_id, 8) == 0); } void flash_read_status(uint8_t *status_reg) @@ -162,10 +166,10 @@ void flash_read_status(uint8_t *status_reg) uint8_t tx_buf = READ_STATUS_REG_1; - spi_transfer(&tx_buf, sizeof(tx_buf), NULL, 0, status_reg, 1); + assert(spi_transfer(&tx_buf, sizeof(tx_buf), NULL, 0, status_reg, 1) == 0); tx_buf = READ_STATUS_REG_2; - spi_transfer(&tx_buf, sizeof(tx_buf), NULL, 0, status_reg + 1, 1); + assert(spi_transfer(&tx_buf, sizeof(tx_buf), NULL, 0, status_reg + 1, 1) == 0); } int flash_read_data(uint32_t address, uint8_t *dest_buf, size_t size) diff --git a/hw/application_fpga/fw/tk1/flash.h b/hw/application_fpga/fw/tk1/flash.h index cad6f07..1151d4e 100644 --- a/hw/application_fpga/fw/tk1/flash.h +++ b/hw/application_fpga/fw/tk1/flash.h @@ -39,9 +39,6 @@ #define STATUS_REG_BUSY_BIT 0 #define STATUS_REG_WEL_BIT 1 -bool flash_is_busy(void); -void flash_wait_busy(void); -void flash_write_enable(void); void flash_write_disable(void); void flash_sector_erase(uint32_t address); void flash_block_32_erase(uint32_t address); diff --git a/hw/application_fpga/fw/tk1/main.c b/hw/application_fpga/fw/tk1/main.c index ac96a14..e70c569 100644 --- a/hw/application_fpga/fw/tk1/main.c +++ b/hw/application_fpga/fw/tk1/main.c @@ -51,7 +51,7 @@ struct context { bool use_uss; // Use USS? uint8_t uss[32]; // User Supplied Secret, if any uint8_t flash_slot; // App is loaded from flash slot number - volatile uint8_t *ver_digest; // Verify loaded app against this digest + /*@null@*/ volatile uint8_t *ver_digest; // Verify loaded app against this digest }; static void print_hw_version(void); @@ -575,7 +575,7 @@ int main(void) } } - memset((void*)resetinfo->app_digest, 0, sizeof(resetinfo->app_digest)); + (void)memset((void*)resetinfo->app_digest, 0, sizeof(resetinfo->app_digest)); jump_to_app(); break; // Not reached diff --git a/hw/application_fpga/fw/tk1/partition_table.c b/hw/application_fpga/fw/tk1/partition_table.c index 48a593c..0eed036 100644 --- a/hw/application_fpga/fw/tk1/partition_table.c +++ b/hw/application_fpga/fw/tk1/partition_table.c @@ -16,7 +16,9 @@ enum part_status part_get_status(void) { return part_status; } -void part_digest(struct partition_table *part_table, uint8_t *out_digest, size_t out_len) { +static void part_digest(struct partition_table *part_table, uint8_t *out_digest, size_t out_len); + +static void part_digest(struct partition_table *part_table, uint8_t *out_digest, size_t out_len) { int blake2err = 0; uint8_t key[16] = { @@ -52,11 +54,13 @@ int part_table_read(struct partition_table_storage *storage) } flash_release_powerdown(); - memset(storage, 0x00, sizeof(*storage)); + (void)memset(storage, 0x00, sizeof(*storage)); for (int i = 0; i < 2; i ++) { - flash_read_data(offset[i], (uint8_t *)storage, - sizeof(*storage)); + if (flash_read_data(offset[i], (uint8_t *)storage, + sizeof(*storage)) != 0) { + return -1; + } part_digest(&storage->table, check_digest, sizeof(check_digest)); if (memeq(check_digest, storage->check_digest, sizeof(check_digest))) { @@ -86,8 +90,10 @@ int part_table_write(struct partition_table_storage *storage) for (int i = 0; i < 2; i ++) { flash_sector_erase(offset[i]); - flash_write_data(offset[i], (uint8_t *)storage, - sizeof(*storage)); + if (flash_write_data(offset[i], (uint8_t *)storage, + sizeof(*storage)) != 0) { + return -1; + } } return 0; diff --git a/hw/application_fpga/fw/tk1/preload_app.c b/hw/application_fpga/fw/tk1/preload_app.c index 563f25f..3936dda 100644 --- a/hw/application_fpga/fw/tk1/preload_app.c +++ b/hw/application_fpga/fw/tk1/preload_app.c @@ -119,7 +119,9 @@ int preload_store_finalize(struct partition_table_storage *part_table_storage, s debug_putinthex(app_size); debug_lf(); - part_table_write(part_table_storage); + if (part_table_write(part_table_storage) != 0) { + return -6; + } return 0; } @@ -149,13 +151,15 @@ int preload_delete(struct partition_table_storage *part_table_storage, uint8_t s part_table->pre_app_data[slot].size = 0; - memset(part_table->pre_app_data[slot].digest, 0, - sizeof(part_table->pre_app_data[slot].digest)); + (void)memset(part_table->pre_app_data[slot].digest, 0, + sizeof(part_table->pre_app_data[slot].digest)); - memset(part_table->pre_app_data[slot].signature, 0, - sizeof(part_table->pre_app_data[slot].signature)); + (void)memset(part_table->pre_app_data[slot].signature, 0, + sizeof(part_table->pre_app_data[slot].signature)); - part_table_write(part_table_storage); + if (part_table_write(part_table_storage) != 0) { + return -6; + } /* Assumes the area is 64 KiB block aligned */ flash_block_64_erase(slot_to_start_address(slot)); // Erase first 64 KB block diff --git a/hw/application_fpga/fw/tk1/spi.c b/hw/application_fpga/fw/tk1/spi.c index 8204bdb..b2cd3bd 100644 --- a/hw/application_fpga/fw/tk1/spi.c +++ b/hw/application_fpga/fw/tk1/spi.c @@ -14,10 +14,16 @@ static volatile uint32_t *spi_xfer = (volatile uint32_t *)(TK1_MMIO_TK1_BA static volatile uint32_t *spi_data = (volatile uint32_t *)(TK1_MMIO_TK1_BASE | 0x208); // clang-format on +static int spi_ready(void); +static void spi_enable(void); +static void spi_disable(void); +static void spi_write(uint8_t *cmd, size_t size); +static void spi_read(uint8_t *buf, size_t size); + // returns non-zero when the SPI-master is ready, and zero if not // ready. This can be used to check if the SPI-master is available // in the hardware. -int spi_ready(void) +static int spi_ready(void) { return *spi_xfer; } diff --git a/hw/application_fpga/fw/tk1/spi.h b/hw/application_fpga/fw/tk1/spi.h index 2c59f4c..d2571e0 100644 --- a/hw/application_fpga/fw/tk1/spi.h +++ b/hw/application_fpga/fw/tk1/spi.h @@ -7,7 +7,6 @@ #include #include -int spi_ready(void); int spi_transfer(uint8_t *cmd, size_t cmd_size, uint8_t *tx_buf, size_t tx_size, uint8_t *rx_buf, size_t rx_size); diff --git a/hw/application_fpga/fw/tk1/storage.c b/hw/application_fpga/fw/tk1/storage.c index c917f92..dc14e71 100644 --- a/hw/application_fpga/fw/tk1/storage.c +++ b/hw/application_fpga/fw/tk1/storage.c @@ -99,7 +99,9 @@ int storage_allocate_area(struct partition_table_storage *part_table_storage) part_table->app_storage[index].status = 0x01; auth_app_create(&part_table->app_storage[index].auth); - part_table_write(part_table_storage); + if (part_table_write(part_table_storage) != 0) { + return -5; + } return 0; } @@ -135,14 +137,16 @@ int storage_deallocate_area(struct partition_table_storage *part_table_storage) /* Clear partition table lastly */ part_table->app_storage[index].status = 0; - memset(part_table->app_storage[index].auth.nonce, 0x00, - sizeof(part_table->app_storage[index].auth.nonce)); + (void)memset(part_table->app_storage[index].auth.nonce, 0x00, + sizeof(part_table->app_storage[index].auth.nonce)); - memset( - part_table->app_storage[index].auth.authentication_digest, 0x00, - sizeof(part_table->app_storage[index].auth.authentication_digest)); + (void)memset( + part_table->app_storage[index].auth.authentication_digest, 0x00, + sizeof(part_table->app_storage[index].auth.authentication_digest)); - part_table_write(part_table_storage); + if (part_table_write(part_table_storage) != 0) { + return -5; + } return 0; } diff --git a/hw/application_fpga/fw/tk1/syscall_handler.c b/hw/application_fpga/fw/tk1/syscall_handler.c index 2a464e0..8f1ddd3 100644 --- a/hw/application_fpga/fw/tk1/syscall_handler.c +++ b/hw/application_fpga/fw/tk1/syscall_handler.c @@ -36,7 +36,7 @@ int32_t syscall_handler(uint32_t number, uint32_t arg1, uint32_t arg2, return -1; } - memset((void *)resetinfo, 0, sizeof(*resetinfo)); + (void)memset((void *)resetinfo, 0, sizeof(*resetinfo)); resetinfo->type = userreset->type; memcpy((void *)resetinfo->app_digest, userreset->app_digest, 32); memcpy((void *)resetinfo->next_app_data, userreset->next_app_data, arg2); @@ -44,6 +44,7 @@ int32_t syscall_handler(uint32_t number, uint32_t arg1, uint32_t arg2, // Should not be reached. assert(1 == 2); + break; case TK1_SYSCALL_ALLOC_AREA: if (storage_allocate_area(&part_table_storage) < 0) {