Re: [PATCH 02/11] This adds code to read OSTC3 firmware files.

On 21-11-14 21:28, Anton Lundin wrote:

+// FIXME: This key is specific to the ostc3 +// The ostc sport has another key but the same protocoll +// How should we refactor the code for that? +static unsigned char ostc3_key[16] = {

• 0xF1, 0xE9, 0xB0, 0x30,
• 0x45, 0x6F, 0xBE, 0x55,
• 0xFF, 0xE7, 0xF8, 0x31,
• 0x13, 0x6C, 0xF2, 0xFE

+};

This should be marked as const.

+static unsigned int hw_ostc3_firmware_checksum(hw_ostc3_firmware_t *firmware) +{

• unsigned short low = 0;
• unsigned short high = 0;
• for(unsigned int i = 0;i < SZ_FIRMWARE;i++)
• {

• low  += firmware->data[i];
  

• high += low;
  

• }
• return (((unsigned int)high) << 16) + low;

+}

This looks like a variant of the fletcher16 checksum (with a 16bit sum instead of 8bit sum, and modulo 2^n instead of 2^n-1). So I suggest moving it to checksum.c where it can be reused elsewhere.

+static dc_status_t +hw_ostc3_firmware_readline(FILE *fp, unsigned int addr, char data[], unsigned int size) +{

• char ascii[40];
• // 1 byte :, 6 bytes addr, X*2 bytes hex -> X bytes data.
• unsigned line_size = size * 2 + 1 + 6 + 1;
• unsigned char faddr_byte[3];
• unsigned int faddr = 0;
• if (line_size > sizeof(ascii))

• return DC_STATUS_INVALIDARGS;
  

• if (fread (ascii, sizeof(char), line_size, fp) != line_size)

• return DC_STATUS_IO;
  

• if (ascii[0] != ':')

• return DC_STATUS_IO;
  

• if (array_convert_hex2bin(ascii + 1, 6, faddr_byte, sizeof(faddr_byte)) != 0) {

• return DC_STATUS_IO;
  

• }
• faddr = array_uint24_be (faddr_byte);
• if (faddr != addr)

• return DC_STATUS_IO;
  

• if (array_convert_hex2bin (ascii + 1 + 6, size*2, data, size) != 0)

• return DC_STATUS_IO;
  

• return DC_STATUS_SUCCESS;

+}

This will needs some improvements to be able to deal with different line endings (e.g CRLF vs LF). See ihex.c for an example.

DC_STATUS_DATAFORMAT is also a more appropriate error code than DC_STATUS_IO here. Except for the fread one.

+static dc_status_t +hw_ostc3_firmware_readfile (hw_ostc3_firmware_t *firmware, dc_context_t *context, const char *filename) +{

• dc_status_t rc = DC_STATUS_SUCCESS;
• FILE *fp;
• unsigned char iv[16] = {0};
• unsigned char tmpbuf[16] = {0};
• unsigned char encrypted[16] = {0};
• unsigned int bytes = 0, addr = 0, faddr = 0;
• char checksum[4];
• if (firmware == NULL) {

• ERROR (context, "Invalid arguments.");
  

• return DC_STATUS_INVALIDARGS;
  

• }
• // Initialize the buffers.
• memset (firmware->data, 0xFF, sizeof (firmware->data));
• firmware->checksum = 0;
• fp = fopen (filename, "rb");
• if (fp == NULL) {

• ERROR (context, "Failed to open the file.");
  

• return DC_STATUS_IO;
  

• }
• rc = hw_ostc3_firmware_readline(fp, 0, iv, sizeof(iv));
• if (rc != DC_STATUS_SUCCESS) {

• fclose (fp);
  

• ERROR (context, "Failed to parse header.");
  

• return rc;
  

• }
• bytes += 16;
• // Load the iv for AES-FCB-mode
• AES128_ECB_encrypt(iv, ostc3_key, tmpbuf);
• for(addr = 0; addr < SZ_FIRMWARE; addr += 16, bytes += 16) {

• rc = hw_ostc3_firmware_readline(fp, bytes, encrypted, sizeof(encrypted));
  

• if (rc != DC_STATUS_SUCCESS) {
  

• 	fclose (fp);
  

• 	ERROR (context, "Failed to parse file data.");
  

• 	return rc;
  

• }
  

• // Decrypt AES-FCB data
  

• for(int i=0;i < 16;i++)
  

• 	firmware->data[addr + i] = encrypted[i] ^ tmpbuf[i];
  

• // Run the next round of encryption
  

• AES128_ECB_encrypt(encrypted, ostc3_key, tmpbuf);
  

• }
• // This file format contains a tail with the checksum in
• rc = hw_ostc3_firmware_readline(fp, bytes, checksum, sizeof(checksum));
• if (rc != DC_STATUS_SUCCESS) {

• fclose (fp);
  

• ERROR (context, "Failed to parse file tail.");
  

• return rc;
  

• }
• fclose(fp);
• firmware->checksum = array_uint32_le(checksum);
• if (firmware->checksum != hw_ostc3_firmware_checksum(firmware)) {

• ERROR (context, "Failed to verify file checksum.");
  

• return DC_STATUS_IO;
  

• }
• return DC_STATUS_SUCCESS;

+}

Two questions here:

If I understand correctly, the address in the firmware file is not really an address in the normal sense, but more like a sequence number, right? Can they be in a random order, or are they always nicely incrementing as the code assumes?

Is the payload always guaranteed to be 16 bytes (except for the last line with the checksum)? I assume it is because there is no length stored anywhere.

Jef