Skip to content

Commit 423c1d4

Browse files
committed
Revert misguided secret_size guard in sata_unlock_disk
F-4647 added an exact-size guard in sata_unlock_disk(): if (secret_size != ATA_UNLOCK_DISK_KEY_SZ) { r = -1; goto cleanup; } on the assumption that the disk unlock secret is a fixed ATA_UNLOCK_DISK_KEY_SZ (32) byte key. That assumption is wrong: the secret is a variable-length base64 string produced from ATA_SECRET_RANDOM_BYTES (21) random bytes, i.e. 28 encoded chars plus a NUL = 29 bytes. The guard therefore rejected every valid unseal result (29 != 32) and panicked on every boot, failing the fsp_qemu_test CI job ("Secret 29 bytes" -> "wolfBoot: PANIC!"). The Fenrir F-4647 finding was only partly off: the real issue it flagged -- an out-of-bounds read from an unbounded copy of a non-NUL-terminated passphrase buffer -- is genuine and remains fixed in security_command_passphrase() (the passphrase copy is length-bounded to ATA_SECURITY_PASSWORD_LEN). Only the additional sata_unlock_disk size guard was based on a wrong premise. Remove the guard and document why none belongs there: the secret is a variable-length string (so an equality check is wrong), and a size guard is unnecessary because sata_get_unlock_secret() passes sizeof(secret) as the wolfBoot_unseal() capacity, so secret_size can never exceed the buffer.
1 parent 590164e commit 423c1d4

1 file changed

Lines changed: 9 additions & 4 deletions

File tree

src/x86/ahci.c

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -436,10 +436,15 @@ int sata_unlock_disk(int drv, int freeze)
436436
r = sata_get_unlock_secret(secret, &secret_size);
437437
if (r != 0)
438438
goto cleanup;
439-
if (secret_size != ATA_UNLOCK_DISK_KEY_SZ) {
440-
r = -1;
441-
goto cleanup;
442-
}
439+
/* No secret_size check here on purpose: the unlock secret is a
440+
* variable-length base64 string (ATA_SECRET_RANDOM_BYTES random bytes
441+
* encoded, e.g. 29 bytes), not a fixed ATA_UNLOCK_DISK_KEY_SZ key, so an
442+
* equality check is wrong. A size guard is also unnecessary:
443+
* sata_get_unlock_secret() passes sizeof(secret) (ATA_UNLOCK_DISK_KEY_SZ)
444+
* as the wolfBoot_unseal() capacity, so secret_size can never exceed the
445+
* buffer. The non-NUL-terminated read in the ATA passphrase path is bounded
446+
* separately by strnlen(passphrase, ATA_SECURITY_PASSWORD_LEN) in
447+
* security_command_passphrase(). */
443448
ata_st = ata_security_get_state(drv);
444449
wolfBoot_printf("ATA: Security state SEC%d\r\n", ata_st);
445450
#if defined(TARGET_x86_fsp_qemu)

0 commit comments

Comments
 (0)