check firmware prior to writing, disclaimer updates in readme, functi…

…onal-test script (#20)

Changes here add:
* A safeguard to make it less possible to write invalid (bricking)
firmware to the device
* Disclaimer in readme warning about the potentially unwanted
side-effects of reading from the device
* A functional test script to run prior to making any pushes or releases
(sadly specific to my development environment)

README.md

@@ -10,8 +10,14 @@ This is an experimental tool, so use it at your own risk.
 
 ## Usage
 
+Read [here](https://github.com/carlossless/sinowealth-kb-tool/issues/19) for ISP quirks.
+
 ### Reading
 
+⚠️ Reading is not entirely an idempotent operation. A read operation can change values in the `0xeffb - 0xeffd` region.
+
+⚠️ The ISP bootloader will blank out bytes in the `0xeffb - 0xeffd` region, therefore the produced dump might not reflect the actual state in ROM.
+
 ```sh
 # reads firmware excluding isp bootloader 
 sinowealth-kb-tool read -p nuphy-air60 foobar.hex

flake.nix

@@ -27,6 +27,7 @@
         buildInputs = with pkgs; [
           pkg-config
           libusb1
+          binutils
         ] ++
         (lib.optionals (stdenv.hostPlatform.isLinux) [
           udev

src/isp.rs

@@ -39,6 +39,17 @@ pub struct ISPDevice {
     part: Part,
 }
 
+#[derive(Debug, Error)]
+pub enum FirmwareError {
+    #[error("No LJMP found at {location_addr:#06x}")]
+    NoLJMP { location_addr: u16 },
+    #[error("LJMP at {location_addr:#06x} points to invalid address {target_addr:#06x}")]
+    InvalidLJMPAddr {
+        location_addr: u16,
+        target_addr: u16,
+    },
+}
+
 #[derive(Debug, Error)]
 pub enum ISPError {
     #[error("Duplicate devices found")]
@@ -49,6 +60,8 @@ pub enum ISPError {
     HidError(#[from] HidError),
     #[error(transparent)]
     VerificationError(#[from] VerificationError),
+    #[error(transparent)]
+    FirmwareError(#[from] FirmwareError),
 }
 
 #[derive(Debug, Clone)]
@@ -256,9 +269,10 @@ impl ISPDevice {
     pub fn write_cycle(&self, firmware: &mut Vec<u8>) -> Result<(), ISPError> {
         let length = firmware.len();
 
+        self.check_firmware(firmware)?;
+
         self.erase()?;
         self.write(firmware)?;
-        let written = self.read(0, self.part.flash_size)?;
 
         // ARCANE: the ISP will copy the LJMP instruction (if existing) from the end to the very start of memory.
         // We need to make modifications to the expected payload to account for this.
@@ -269,6 +283,7 @@ impl ISPDevice {
         }
 
         info!("Verifying...");
+        let written = self.read(0, self.part.flash_size)?;
         util::verify(firmware, &written).map_err(ISPError::from)?;
         self.finalize()?;
         Ok(())
@@ -281,6 +296,37 @@ impl ISPDevice {
         &self.request_device
     }
 
+    fn check_firmware(&self, firmware: &mut Vec<u8>) -> Result<(), ISPError> {
+        let length = firmware.len();
+        if firmware[length - 5] != LJMP_OPCODE {
+            info!("No LJMP detected at {:#06x}", length - 5);
+            if firmware[0] != LJMP_OPCODE {
+                return Err(ISPError::FirmwareError(FirmwareError::NoLJMP {
+                    location_addr: 0x0000,
+                }));
+            }
+            let ljmp_addr = (firmware[1] as u16) << 8 | firmware[2] as u16;
+            if ljmp_addr == 0x0000 {
+                return Err(ISPError::FirmwareError(FirmwareError::InvalidLJMPAddr {
+                    location_addr: 0x0000,
+                    target_addr: ljmp_addr,
+                }));
+            }
+            info!("Copying LJMP from {:#06x} to {:#06x}", 0x0000, length - 5);
+            firmware[length - 5] = LJMP_OPCODE;
+            firmware.copy_within(1..3, length - 4); // Copy LJMP address
+        } else {
+            let ljmp_addr = (firmware[length - 4] as u16) << 8 | firmware[length - 3] as u16;
+            if ljmp_addr == 0x0000 {
+                return Err(ISPError::FirmwareError(FirmwareError::InvalidLJMPAddr {
+                    location_addr: (length - 5) as u16,
+                    target_addr: ljmp_addr,
+                }));
+            }
+        }
+        return Ok(());
+    }
+
     /// Allows firmware to be read prior to erasing it
     fn magic_sauce(&self) -> Result<(), ISPError> {
         let cmd: [u8; COMMAND_LENGTH] = [

tools/functional-test.sh

@@ -0,0 +1,128 @@
+#!/usr/bin/env bash
+
+set -euo pipefail
+
+# TOOL=sinowealth-kb-tool
+TOOL="cargo run --"
+FILE_PREFIX="private/test-$(date +'%Y%m%dT%H%M%S')"
+PART="nuphy-air60"
+
+EXPECTED_BOOTLOADER_MD5="3e0ebd0c440af5236d7ff8872343f85d"
+
+FILE_DEFAULT="$FILE_PREFIX-read.hex"
+FILE_BOOTLOADER="$FILE_PREFIX-read-bootloader.hex"
+FILE_FULL="$FILE_PREFIX-read-full.hex"
+FILE_CUSTOM="$FILE_PREFIX-read-custom.hex"
+FILE_OVERRIDE="$FILE_PREFIX-read-override.hex"
+FILE_POST_WRITE="$FILE_PREFIX-post-write.hex"
+
+function reboot_device () {
+    echo "Turning off port..."
+    uhubctl -a off -p 1 -l 65-1
+    sleep 1
+    echo "Turning on port..."
+    uhubctl -a on -p 1 -l 65-1
+    echo "Waiting..."
+    sleep 5
+}
+
+function get_md5 () {
+    MD5SUM=($(md5sum "$1"))
+    echo $MD5SUM
+}
+
+function get_md5_from_hex () {
+    objcopy --input-target=ihex --output-target=binary "$1" "${1%.hex}.bin"
+    MD5SUM=$(get_md5 "${1%.hex}.bin")
+    echo $MD5SUM
+}
+
+echo "Initial reboot..."
+reboot_device
+
+echo "Standard read..."
+$TOOL read --part "$PART" "$FILE_DEFAULT"
+
+reboot_device
+
+echo "Bootloader read..."
+$TOOL read --part "$PART" -b "$FILE_BOOTLOADER"
+
+reboot_device
+
+echo "Full read..."
+$TOOL read --part "$PART" --full "$FILE_FULL"
+
+reboot_device
+
+echo "Custom read..."
+$TOOL read \
+    --flash_size 61440 \
+    --bootloader_size 4096 \
+    --page_size 2048 \
+    --vendor_id 0x05ac \
+    --product_id 0x024f \
+    "$FILE_CUSTOM"
+
+reboot_device
+
+echo "Override read..."
+$TOOL read \
+    --part "$PART" \
+    --vendor_id 0x05ac \
+    --product_id 0x024f \
+    "$FILE_OVERRIDE"
+
+reboot_device
+
+READ_MD5=$(get_md5_from_hex "$FILE_DEFAULT")
+READ_BOOTLOADER_MD5=$(get_md5_from_hex "$FILE_BOOTLOADER")
+READ_FULL_MD5=$(get_md5_from_hex "$FILE_FULL")
+READ_CUSTOM_MD5=$(get_md5_from_hex "$FILE_CUSTOM")
+READ_OVERRIDE_MD5=$(get_md5_from_hex "$FILE_OVERRIDE")
+
+echo "Checking bootloader checksum"
+if [[ "$READ_BOOTLOADER_MD5" != "$EXPECTED_BOOTLOADER_MD5" ]]; then
+    echo "MD5 mismatch $READ_BOOTLOADER_MD5 != $EXPECTED_BOOTLOADER_MD5"
+    exit 1
+fi
+
+echo "Checking custom checksum"
+if [[ "$READ_CUSTOM_MD5" != "$READ_MD5" ]]; then
+    echo "MD5 mismatch $READ_CUSTOM_MD5 != $READ_MD5"
+    exit 1
+fi
+
+echo "Checking override checksum"
+if [[ "$READ_OVERRIDE_MD5" != "$READ_MD5" ]]; then
+    echo "MD5 mismatch $READ_OVERRIDE_MD5 != $READ_MD5"
+    exit 1
+fi
+
+echo "Checking standard+bootloader == full"
+cat "${FILE_DEFAULT%.hex}.bin" "${FILE_BOOTLOADER%.hex}.bin" > "$FILE_PREFIX-concat-full.bin"
+EXPECTED_FULL_MD5=$(get_md5 "$FILE_PREFIX-concat-full.bin")
+if [[ "$READ_FULL_MD5" != "$EXPECTED_FULL_MD5" ]]; then
+    echo "MD5 mismatch $READ_FULL_MD5 != $EXPECTED_FULL_MD5"
+    exit 1
+fi
+
+echo "Standard write..."
+$TOOL write --part "$PART" "$FILE_DEFAULT"
+
+reboot_device
+
+echo "Post-write read..."
+$TOOL read --part "$PART" "$FILE_POST_WRITE"
+
+READ_POST_WRITE_MD5=$(get_md5_from_hex "$FILE_POST_WRITE")
+
+echo "Checking post-write checksum"
+if [[ "$READ_POST_WRITE_MD5" != "$READ_MD5" ]]; then
+    echo "MD5 mismatch $READ_POST_WRITE_MD5 != $READ_MD5"
+    exit 1
+fi
+
+reboot_device
+
+echo "Passed all tests!"