Addressed reviewer's comments

philljj · Sep 5, 2023 · 87f97c1 · 87f97c1
1 parent 75444cf
commit 87f97c1
Show file tree

Hide file tree

Showing 4 changed files with 20 additions and 41 deletions.
diff --git a/include/x86/ata.h b/include/x86/ata.h
@@ -87,11 +87,19 @@ enum ata_security_state ata_security_get_state(int);
 #define ATA_CMD_WRITE_DMA 0xCA
 #define ATA_CMD_IDENTIFY_DEVICE 0xEC
 
+#define ATA_IDENTIFY_DEVICE_COMMAND_LEN          (256 * 2)
+
+
+/* Security feature set */
 #define ATA_CMD_SECURITY_SET_PASSWORD       0xF1
 #define ATA_CMD_SECURITY_UNLOCK             0xF2
 #define ATA_CMD_SECURITY_ERASE_PREPARE      0xF3
 #define ATA_CMD_SECURITY_ERASE_UNIT         0xF4
 #define ATA_CMD_SECURITY_FREEZE_LOCK        0xF5
 #define ATA_CMD_SECURITY_DISABLE_PASSWORD   0xF6
 
+/* Constants for security set commands */
+#define ATA_SECURITY_COMMAND_LEN                (256 * 2)
+#define ATA_SECURITY_PASSWORD_OFFSET            (1 * 2)
+
 #endif
diff --git a/options.mk b/options.mk
@@ -536,7 +536,6 @@ ifeq ($(LINUX_PAYLOAD),1)
   endif
 endif
 
-
 ifeq ($(64BIT),1)
   CFLAGS+=-DWOLFBOOT_64BIT
 endif

diff --git a/src/x86/ahci.c b/src/x86/ahci.c
@@ -65,8 +65,6 @@
 #define PCI_REG_MAP_AHCI_MODE (0x1 << 6)
 #define PCI_REG_MAP_ALL_PORTS (0x1 << 5)
 
-#define DEBUG_AHCI
-
 #ifdef DEBUG_AHCI
 #define AHCI_DEBUG_PRINTF(...) wolfBoot_printf(__VA_ARGS__)
 #else
@@ -395,12 +393,15 @@ void sata_enable(uint32_t base) {
 
 #ifdef WOLFBOOT_ATA_DISK_LOCK
                         ata_st = ata_security_get_state(drv);
+                        wolfBoot_printf("ATA: Security state SEC%d\r\n", ata_st);
                         if (ata_st == ATA_SEC1) {
                             AHCI_DEBUG_PRINTF("ATA identify: calling freeze lock\r\n", r);
                             r = ata_security_freeze_lock(drv);
                             AHCI_DEBUG_PRINTF("ATA security freeze lock: returned %d\r\n", r);
                             r = ata_identify_device(drv);
                             AHCI_DEBUG_PRINTF("ATA identify: returned %d\r\n", r);
+                            ata_st = ata_security_get_state(drv);
+                            wolfBoot_printf("ATA: Security disabled. State SEC%d\r\n", ata_st);
                         }
                         else if (ata_st == ATA_SEC4) {
                             AHCI_DEBUG_PRINTF("ATA identify: calling device unlock\r\n", r);
@@ -420,6 +421,8 @@ void sata_enable(uint32_t base) {
                             if (ata_st != ATA_SEC6) {
                                 panic();
                             }
+                            ata_st = ata_security_get_state(drv);
+                            wolfBoot_printf("ATA: Security enabled. State SEC%d\r\n", ata_st);
                         }
 #endif
                     }

diff --git a/src/x86/ata.c b/src/x86/ata.c
@@ -41,8 +41,6 @@
 
 #define CACHE_INVALID 0xBADF00DBADC0FFEEULL
 
-#define DEBUG_ATA
-
 #ifdef DEBUG_ATA
 /**
  * @brief This macro is used to conditionally print debug messages for the ATA
@@ -162,36 +160,6 @@ struct __attribute__((packed)) fis_reg_d2h {
     uint32_t _res4;
 };
 
-/**
- * @brief This packed structure defines the format of a Data FIS used for
- * PIO transfer setup.
- */
-struct __attribute__((packed)) fis_pio_setup {
-    uint8_t  fis_type;
-    uint8_t pmport:4, _res0:1, dir:1,  i:1, _res1:1;
-
-    uint8_t status;
-    uint8_t error;
-
-    uint8_t  lba0;
-	uint8_t  lba1;
-	uint8_t  lba2;
-    uint8_t  device;
-
-
-	uint8_t  lba3;
-	uint8_t  lba4;
-	uint8_t  lba5;
-	uint8_t  _res2;
-
-	uint8_t  count_l;
-	uint8_t  count_h;
-	uint8_t  _res3;
-	uint8_t  e_status;
-
-	uint16_t transfer_count;
-	uint8_t  _res4[2];
-};
 
 /**
  * @brief This packed structure defines the format of a Data FIS used for
@@ -382,7 +350,7 @@ static int security_command(int drv, uint8_t ata_cmd)
     struct fis_reg_h2d *cmdfis;
     struct ata_drive *ata = &ATA_Drv[drv];
     int ret;
-    int slot = prepare_cmd_h2d_slot(drv, buffer, 512, 0);
+    int slot = prepare_cmd_h2d_slot(drv, buffer, ATA_SECURITY_COMMAND_LEN, 0);
     if (slot < 0) {
         return slot;
     }
@@ -420,9 +388,10 @@ static int security_command_passphrase(int drv, uint8_t ata_cmd,
     struct fis_reg_h2d *cmdfis;
     struct ata_drive *ata = &ATA_Drv[drv];
     int ret;
-    int slot = prepare_cmd_h2d_slot(drv, buffer, 512, 1);
-    memset(buffer, 0, 512);
-    memcpy(buffer + 2, passphrase, strlen(passphrase));
+    int slot = prepare_cmd_h2d_slot(drv, buffer,
+            ATA_SECURITY_COMMAND_LEN, 1);
+    memset(buffer, 0, ATA_SECURITY_COMMAND_LEN);
+    memcpy(buffer + ATA_SECURITY_PASSWORD_OFFSET, passphrase, strlen(passphrase));
     if (slot < 0) {
         return slot;
     }
@@ -543,13 +512,13 @@ int ata_identify_device(int drv)
     uint8_t serial_no[ATA_ID_SERIAL_NO_LEN];
     uint8_t model_no[ATA_ID_MODEL_NO_LEN];
     int ret = 0;
-    int slot = prepare_cmd_h2d_slot(drv, buffer, 512, 0);
+    int slot = prepare_cmd_h2d_slot(drv, buffer,
+            ATA_IDENTIFY_DEVICE_COMMAND_LEN, 0);
     int s_locked, s_frozen, s_enabled, s_supported;
 
     if (slot < 0)
         return slot;
 
-
     cmd = (struct hba_cmd_header *)(uintptr_t)ata->clb_port;
     cmd += slot;