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

@@ -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
 
 
 

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

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)

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 <check.h>
+#include <stdint.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <x86/ata.h>
+
+#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;
+}