From cd4e4be2f654af69c9a215608c77b3a6f6cd30fd Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Thu, 2 Jul 2026 14:27:04 +0200 Subject: [PATCH] F-5968: zeroize disk-unlock passphrase from static ATA command buffer security_command_passphrase() copies the plaintext disk-unlock secret into the file-static DMA buffer `buffer` at ATA_SECURITY_PASSWORD_OFFSET but never wipes it. On the slot<0 early return no ATA command is even dispatched, and on failures of ata_security_set_password()/ ata_security_unlock_device() (which sata_unlock_disk() turns into a panic()) no later IDENTIFY DMA ever overwrites it, so the secret is left resident in BSS for as long as the device stays powered. Zeroize the password field on the slot<0 path and after synchronous command completion. The async path (used only by the currently unreachable ata_security_erase_unit()) is left untouched because the HBA may still be DMAing out of the buffer when exec_cmd_slot_ex() returns ATA_ERR_BUSY; wiping it there would race the transfer. --- .gitignore | 1 + src/x86/ata.c | 22 +++ tools/unit-tests/Makefile | 5 + .../unit-ata-security-passphrase-zeroize.c | 142 ++++++++++++++++++ 4 files changed, 170 insertions(+) create mode 100644 tools/unit-tests/unit-ata-security-passphrase-zeroize.c diff --git a/.gitignore b/.gitignore index 4e58cb92..bca5feab 100644 --- a/.gitignore +++ b/.gitignore @@ -211,6 +211,7 @@ tools/unit-tests/unit-flash-erase-wb tools/unit-tests/unit-fwtpm-nv-oob tools/unit-tests/unit-x86-paging-oob tools/unit-tests/unit-ahci-unlock-panic +tools/unit-tests/unit-ata-security-passphrase-zeroize diff --git a/src/x86/ata.c b/src/x86/ata.c index bb6dd1fa..5858c4d3 100644 --- a/src/x86/ata.c +++ b/src/x86/ata.c @@ -433,6 +433,19 @@ static int security_command(int drv, uint8_t ata_cmd) return ret; } +/** + * @brief Wipe the plaintext passphrase bytes out of the static command + * buffer once the HBA no longer needs them. + */ +static void ata_security_buffer_zeroize(void) +{ + volatile uint8_t *p = (volatile uint8_t *)buffer + ATA_SECURITY_PASSWORD_OFFSET; + size_t len = ATA_SECURITY_PASSWORD_LEN; + while (len-- > 0U) { + *p++ = 0U; + } +} + /** * @brief Helper function to execute an ATA command from the security set that * require transmitting a passphrase. @@ -471,6 +484,7 @@ static int security_command_passphrase(int drv, uint8_t ata_cmd, memcpy(buffer + ATA_SECURITY_PASSWORD_OFFSET, passphrase, passphrase_len); if (slot < 0) { + ata_security_buffer_zeroize(); return slot; } cmd = (struct hba_cmd_header *)(uintptr_t)ata->clb_port; @@ -482,6 +496,14 @@ static int security_command_passphrase(int drv, uint8_t ata_cmd, cmdfis->command = ata_cmd; cmdfis->count = 1; ret = exec_cmd_slot_ex(drv, slot, async); + /* exec_cmd_slot_ex() only returns once the HBA has finished the DMA + * transfer of this buffer when running synchronously (async = 0), so + * it is safe to wipe the passphrase here. In async mode the transfer + * may still be in flight when we return (the caller polls completion + * via ata_cmd_complete_async()), so clearing the buffer now would race + * the HBA and could corrupt the command still in progress. */ + if (!async) + ata_security_buffer_zeroize(); return ret; } diff --git a/tools/unit-tests/Makefile b/tools/unit-tests/Makefile index 7bec24b9..352e1796 100644 --- a/tools/unit-tests/Makefile +++ b/tools/unit-tests/Makefile @@ -76,6 +76,7 @@ TESTS+=unit-flash-erase-u3 TESTS+=unit-otp-keystore TESTS+=unit-x86-paging-oob TESTS+=unit-ahci-unlock-panic +TESTS+=unit-ata-security-passphrase-zeroize TESTS+=unit-fwtpm-nv-oob TESTS+=unit-elf-bss-guard TESTS+=unit-arm-tee-psa-ipc @@ -569,6 +570,10 @@ unit-ahci-unlock-panic: ../../include/target.h unit-ahci-unlock-panic.c gcc -o $@ unit-ahci-unlock-panic.c $(CFLAGS) \ -ffunction-sections -fdata-sections $(LDFLAGS) -Wl,--gc-sections +unit-ata-security-passphrase-zeroize: ../../include/target.h unit-ata-security-passphrase-zeroize.c + gcc -o $@ unit-ata-security-passphrase-zeroize.c $(CFLAGS) \ + -ffunction-sections -fdata-sections $(LDFLAGS) -Wl,--gc-sections + unit-elf-bss-guard: unit-elf-bss-guard.c gcc -o $@ $< -I../../include -DWOLFBOOT_ELF -DARCH_FLASH_OFFSET=0 \ -DWOLFBOOT_NO_PRINTF -g $(LDFLAGS) diff --git a/tools/unit-tests/unit-ata-security-passphrase-zeroize.c b/tools/unit-tests/unit-ata-security-passphrase-zeroize.c new file mode 100644 index 00000000..f0df0d6c --- /dev/null +++ b/tools/unit-tests/unit-ata-security-passphrase-zeroize.c @@ -0,0 +1,142 @@ +/* unit-ata-security-passphrase-zeroize.c + * + * Regression test for security_command_passphrase() leaving the plaintext + * disk-unlock passphrase resident in the file-static ATA command DMA buffer + * ("buffer" in src/x86/ata.c) after it returns, instead of wiping it like + * ahci.c does for its own copies of the same secret (ahci_secret_zeroize()). + * Exercised through the public ata_security_unlock_device() wrapper, which + * is exactly how sata_unlock_disk() reaches it. + */ + +#include +#include +#include +#include +#include + +#define WOLFBOOT_ATA_DISK_LOCK + +/* Mocked AHCI port registers. security_command_passphrase() reaches these + * only through find_cmd_slot() (SACT/CI, to allocate a slot) and + * exec_cmd_slot_ex() (TFD/IS/CI, to wait for command completion). An + * always-idle model is enough to drive a real command to synchronous + * completion without simulating actual AHCI hardware; mock_slots_full + * additionally lets a test force prepare_cmd_h2d_slot() down its "no free + * slot" error path. */ +static int mock_slots_full; + +uint32_t mmio_read32(uintptr_t address) +{ + (void)address; + return mock_slots_full ? 0xFFFFFFFF : 0; +} + +void mmio_write32(uintptr_t address, uint32_t value) +{ + (void)address; + (void)value; +} + +void panic(void) +{ + ck_abort_msg("panic!"); +} + +#include "../../src/x86/ata.c" + +/* struct ata_drive stores clb_port/ctable_port as uint32_t "physical" + * addresses, matching the real x86 target's 32-bit DMA pointers. MAP_32BIT + * keeps these allocations inside that range on a 64-bit test host so the + * truncating uint32_t assignment below doesn't lose address bits. */ +static uint8_t *clb_mem; +static uint8_t *ctable_mem; + +static void setup(void) +{ + mock_slots_full = 0; + clb_mem = mmap(NULL, sizeof(struct hba_cmd_header) * 32, + PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS | MAP_32BIT, -1, 0); + ck_assert_ptr_ne(clb_mem, MAP_FAILED); + ctable_mem = mmap(NULL, sizeof(struct hba_cmd_table), + PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS | MAP_32BIT, -1, 0); + ck_assert_ptr_ne(ctable_mem, MAP_FAILED); + + memset(&ATA_Drv[0], 0, sizeof(ATA_Drv[0])); + ATA_Drv[0].ahci_base = 0x10000; /* arbitrary: mmio_* mocks ignore it */ + ATA_Drv[0].ahci_port = 0; + ATA_Drv[0].clb_port = (uint32_t)(uintptr_t)clb_mem; + ATA_Drv[0].ctable_port = (uint32_t)(uintptr_t)ctable_mem; +} + +static void teardown(void) +{ + munmap(clb_mem, sizeof(struct hba_cmd_header) * 32); + munmap(ctable_mem, sizeof(struct hba_cmd_table)); +} + +static void assert_password_field_zero(const char *ctx) +{ + int i; + for (i = 0; i < ATA_SECURITY_PASSWORD_LEN; i++) { + ck_assert_msg(buffer[ATA_SECURITY_PASSWORD_OFFSET + i] == 0, + "%s: plaintext passphrase still resident in static ATA command " + "buffer: byte %d = 0x%02x", ctx, i, + buffer[ATA_SECURITY_PASSWORD_OFFSET + i]); + } +} + +/* Reachable path taken every time sata_unlock_disk() unlocks a drive + * (ata_st == ATA_SEC4): the command dispatches and completes synchronously. + * Per the report, nothing overwrites the password bytes on return other + * than the next unrelated command that happens to reuse the buffer. */ +START_TEST(test_unlock_zeroizes_passphrase_after_command_completes) +{ + static const char passphrase[] = "unit-test-disk-secret"; + int r; + + r = ata_security_unlock_device(0, passphrase, 0); + ck_assert_int_eq(r, 0); + + assert_password_field_zero("after successful SECURITY UNLOCK"); +} +END_TEST + +/* Reachable when the HBA has no free command slot: security_command_passphrase() + * still memcpy()s the passphrase into the static buffer before checking + * `slot < 0`, so the secret is written even though no ATA command is ever + * dispatched. */ +START_TEST(test_unlock_zeroizes_passphrase_on_no_free_slot) +{ + static const char passphrase[] = "unit-test-disk-secret"; + int r; + + mock_slots_full = 1; + r = ata_security_unlock_device(0, passphrase, 0); + ck_assert_int_eq(r, -1); + + assert_password_field_zero("after no-free-slot error return"); +} +END_TEST + +static Suite *ata_security_passphrase_zeroize_suite(void) +{ + Suite *s = suite_create("ata_security_passphrase_zeroize"); + TCase *tc = tcase_create("zeroize"); + tcase_add_checked_fixture(tc, setup, teardown); + tcase_add_test(tc, test_unlock_zeroizes_passphrase_after_command_completes); + tcase_add_test(tc, test_unlock_zeroizes_passphrase_on_no_free_slot); + suite_add_tcase(s, tc); + return s; +} + +int main(void) +{ + Suite *s = ata_security_passphrase_zeroize_suite(); + SRunner *sr = srunner_create(s); + srunner_run_all(sr, CK_NORMAL); + int failed = srunner_ntests_failed(sr); + srunner_free(sr); + return failed == 0 ? 0 : 1; +}