make firmware upload protocol less secure but lean; boot loader works

NOTES

@@ -93,3 +93,5 @@ authentication:
 - shared secret (128 bit, SHA1-hashed text with 128 bit salt)
 - salt (128 bit)
 - SHA1 from avrcryptolib
+- due to chip limitations, the secret key for firmware updates is sent
+  in the clear

PROTOCOL

@@ -7,14 +7,8 @@ Protocol
 
    1	0	0	Pong (maybe return version string in the future)
 
-   2	0	3	Unlock salt A (64 bytes payload)
+   4	0	N	Unlock secret (64 bytes payload)
-   2	1	3	Unlock salt B
+   4	1..N-1	N	Firmware binary (64 bytes payload)
-   2	2	3	Unlock hash A
-   2	3	3	Unlock hash B
-
-   3	n	0	Unlock ACK
-
-   4	0..N-1	N	Firmware binary (64 bytes payload)
    4	N	N	First half of hash
 
    5	n	N	Firmware ACK

fw/Makefile

@@ -15,15 +15,13 @@ SHELL = /bin/bash
 NAME = antorcha
 
 CFLAGS = -g -mmcu=$(CHIP) \
-	 -DBOOT_ADDR=$(BOOT_ADDR) -DAPP_ADDR=$(APP_ADDR) -DAPP_END=$(APP_END) \
+	 -DBOOT_ADDR=$(BOOT_ADDR) \
 	 -Wall -Wextra -Wshadow -Werror -Wno-unused-parameter \
 	 -Wmissing-prototypes -Wmissing-declarations -Wstrict-prototypes
 
 CHIP = atmega168
 HOST = jlime
-BOOT_ADDR = 0
+BOOT_ADDR = 0x3800
-APP_ADDR = 0x1000
-APP_END = 0x4000
 
 AVR_PREFIX = $(BIN_PATH) avr-
 CC = $(AVR_PREFIX)gcc
@@ -31,9 +29,9 @@ OBJCOPY = $(AVR_PREFIX)objcopy
 #OBJDUMP = $(AVR_PREFIX)objdump
 SIZE = $(AVR_PREFIX)size
 
-OBJS = $(NAME).o $(COMMON_OBJS)
+OBJS = $(NAME).o dispatch.o hash.o $(COMMON_OBJS)
 BOOT_OBJS = boot.o flash.o fw.o $(COMMON_OBJS)
-COMMON_OBJS = dispatch.o hash.o rf.o spi.o
+COMMON_OBJS = rf.o spi.o
 
 # ----- Verbosity control -----------------------------------------------------
 
@@ -64,8 +62,7 @@ all:		$(NAME).bin boot.bin
 
 $(NAME).elf:	$(OBJS)
 		$(MAKE) version.o
-		$(CC) $(CFLAGS) -o $@ $(OBJS) version.o \
+		$(CC) $(CFLAGS) -o $@ $(OBJS) version.o
-		  -Wl,--section-start=.text=$(APP_ADDR)
 
 boot.elf:	$(BOOT_OBJS)
 		$(CC) $(CFLAGS) -o $@ $(BOOT_OBJS) \
@@ -140,7 +137,9 @@ prog-app:
 
 prog:
 		ssh $(HOST) avrdude -F -p $(CHIP) -c nanonote_antorcha -e \
-		  -U flash:w:boot.hex:i
+		  -U flash:w:boot.hex:i \
+		  -U efuse:w:0x00:m \
+		  -U lfuse:w:0xe2:m
 #		  -U lfuse:w:0x60:m \
 #		  -U hfuse:w:0xd8:m \
 #		  -U lock:w:0x2f:m

fw/antorcha.c

@@ -1,5 +1,5 @@
 /*
- * fw/main.c - Initialization of Antorcha firmware
+ * fw/antorcha.c - Initialization of Antorcha application firmware
  *
  * Written 2012 by Werner Almesberger
  * Copyright 2012 Werner Almesberger
@@ -11,50 +11,43 @@
  */
 
 
+#include <stddef.h>
+#include <stdint.h>
+
 #include <avr/io.h>
+#define F_CPU   8000000UL
+#include <util/delay.h>
 
 #include "io.h"
 #include "rf.h"
+#include "dispatch.h"
+
+
+
+static const struct handler *protos[] = {
+	NULL
+};
 
 
 int main(void)
 {
-	/* Port B has two LEDs and the rest is SPI */
+	uint8_t buf[PAYLOAD+5]; /* 3 bytes header, 2 bytes CRC */
-	PORTB = MASK(B, RF_nSS) | MASK(B, RF_nRST);
+	uint8_t got;
-	DDRB = MASK(B, RF_SCLK) | MASK(B, RF_MOSI) | MASK(B, RF_nSS) |
-	    MASK(B, RF_nRST) | 0xc0;
 
-	/* All port C pins drive LEDs */
+	/*
-	PORTC = 0;
+	 * The boot loader has already initialized PORTx,  DDRx, and MCUCR.PUD.
-	DDRC = 0x3f;
+	 * It has also brought up RF and the underlying SPI.
-
+	 */
-	/* All port D pins drive LEDs */
-	PORTD = 0;
-	DDRD = 0xff;
-
-	/* disable pull-ups */
-	MCUCR |= 1 << PUD;
-
-	rf_init();
-
-	while (0) {
-		PORTC = 2;
-		PORTC = 0;
-		rf_send("HOLA ?", 6);
-	}
 
 while (1) {
-		uint8_t buf[1], got;
+	SET(LED_B8);
-
+	_delay_ms(100);
-		do {
+	CLR(LED_B8);
-PORTC = 1;
+	_delay_ms(100);
-PORTC = 0;
-got = rf_recv(buf, 1);
 }
-		while (!got);
+	while (1) {
-
+		got = rf_recv(buf, sizeof(buf));
-PORTC = 2;
+		if (got > 2)
-PORTC = 0;
+			dispatch(buf, got-2, protos);
-		PORTD = buf[0];
 	}
 }

fw/boot.c

@@ -21,19 +21,19 @@
 #include "fw.h"
 
 
-#define	MS_TO_LOOP(ms)	((uint32_t) (ms)*335)
+#define	MS_TO_LOOP(ms)	((uint32_t) (ms)*106)
 
 
 static void wait_upload(void)
 {
-	uint8_t buf[PAYLOAD+5];
+	uint8_t buf[PAYLOAD+5]; /* 3 bytes header, 2 bytes CRC */
 	uint32_t i;
 	uint8_t got;
 
 restart:
-	for (i = 0; i != MS_TO_LOOP(2000); i++) {
+	for (i = 0; i != MS_TO_LOOP(5000); i++) {
 		got = rf_recv(buf, sizeof(buf));
-		if (got && dispatch(buf, got, fw_protos))
+		if (got > 2 && fw_packet(buf, got-2))
 			goto restart;
 	}
 
@@ -70,9 +70,7 @@ int main(void)
 		SET(LED_B8);
 		wait_upload();
 		CLR(LED_B8);
-while (1);
+	} while (pgm_read_byte(zero) == 0xff);
-	} while (pgm_read_byte(zero) != 0xff);
-
 
 	((void (*)(void)) 0)();
 

fw/dispatch.c

@@ -15,13 +15,9 @@
 #include <stdbool.h>
 #include <stdint.h>
 
-#define F_CPU   8000000UL
-#include <util/delay.h>
-
 #include "rf.h"
 #include "proto.h"
 #include "dispatch.h"
-#include "io.h"
 
 
 static uint8_t seq;	/* last sequence number seen */
@@ -34,10 +30,7 @@ static void send_ack(const uint8_t *buf)
 {
 	uint8_t ack[3] = { buf[0]+1, buf[1], 0 };
 
-SET(LED_B6);
-	_delay_ms(1);
 	rf_send(ack, sizeof(ack));
-CLR(LED_B6);
 }
 
 
@@ -54,8 +47,6 @@ static bool answer_ping(const uint8_t *buf)
 
 bool dispatch(const uint8_t *buf, uint8_t len, const struct handler **protos)
 {
-SET(LED_B7);
-CLR(LED_B7);
 	if (len == 3 && buf[0] == PING)
 		return answer_ping(buf);
 
@@ -78,23 +69,24 @@ CLR(LED_B7);
 		limit = buf[2];
 		send_ack(buf);
 		return 1;
-	}
+	} else {
-
 		if (!curr_proto)
 			return 0;
 		if (buf[0] != type)
 			return 0;
-	if (buf[1] > limit)
-		return 0;
 		if (buf[2] != limit)
 			return 0;
+		if (buf[1] > limit)
+			return 0;
 
-	if (buf[1] == seq) {
+		if (buf[1]+1 == seq) {
 			send_ack(buf);
 			return 0;
 		}
-	if (buf[1] != seq+1)
+		if (buf[1] != seq)
 			return 0;
+	}
+
 	if (!curr_proto->more(buf[1], limit, buf+3))
 		return 0;
 	seq++;

fw/dispatch.h

@@ -16,6 +16,8 @@
 #include <stdbool.h>
 #include <stdint.h>
 
+#include "proto.h"
+
 
 struct handler {
 	enum pck_type type;

fw/flash.c

@@ -26,15 +26,15 @@
 static uint32_t payload;
 
 
-void flash_start(uint32_t addr)
+void flash_start(void)
 {
-	payload = addr;
+	payload = 0;
 }
 
 
 int flash_can_write(uint16_t size)
 {
-	return payload <= APP_END-size;
+	return payload <= BOOT_ADDR-size;
 }
 
 

fw/flash.h

@@ -16,7 +16,7 @@
 #include <stdint.h>
 
 
-void flash_start(uint32_t addr);
+void flash_start(void);
 int flash_can_write(uint16_t size);
 void flash_write(const uint8_t *buf, uint16_t size);
 void flash_end_write(void);

fw/fw.c

@@ -1,5 +1,5 @@
 /*
- * fw/fw.h - Firmware upload protocols
+ * fw/fw.h - Firmware upload protocol
  *
  * Written 2012 by Werner Almesberger
  * Copyright 2012 Werner Almesberger
@@ -11,123 +11,85 @@
  */
 
 
-#include <stddef.h>
 #include <stdbool.h>
 #include <stdint.h>
+#include <string.h>
 
-#include "hash.h"
+#include "io.h"
 #include "flash.h"
 #include "proto.h"
-#include "dispatch.h"
+#include "rf.h"
 #include "fw.h"
 
 
-static const uint8_t unlock_secret[] = {
+static const uint8_t unlock_secret[PAYLOAD] = {
 	#include "unlock-secret.inc"
 };
 
 
-static void panic(void)
+static bool locked = 1;
+
+
+static bool fw_payload(uint8_t seq, uint8_t limit, const uint8_t *payload)
 {
-	/* ??? */
+	if (!seq) {
-}
+		locked = memcmp(unlock_secret, payload, PAYLOAD) != 0;
-
+		flash_start();
-
-/* ----- Unlocking --------------------------------------------------------- */
-
-
-static bool unlocked = 0;
-static bool unlock_failed;
-
-
-static bool unlock_first(const uint8_t *payload)
-{
-	hash_init();
-	hash_merge(unlock_secret, sizeof(unlock_secret));
-	hash_merge(payload, PAYLOAD);
-	unlocked = 0;
-	unlock_failed = 0;
 		return 1;
 	}
-
+	if (locked)
-
-static bool unlock_more(uint8_t seq, uint8_t limit, const uint8_t *payload)
-{
-	switch (seq) {
-	case 1:
-		hash_merge(payload, PAYLOAD);
-		hash_end();
-		break;
-	case 2:
-		if (!hash_eq(payload, PAYLOAD, 0))
-			unlock_failed = 1;
-		break;
-	case 3:
-		if (unlock_failed)
-			return 1;
-		if (hash_eq(payload, PAYLOAD, PAYLOAD))
-			unlocked = 1;
-		else
-			unlock_failed = 1;
-		break;
-	default:
 		return 0;
-	}
-	return 1;
-}
-
-
-static const struct handler unlock_proto = {
-	.type	= UNLOCK,
-	.first	= unlock_first,
-	.more	= unlock_more,
-};
-
-
-/* ----- Firmware upload --------------------------------------------------- */
-
-
-static bool fw_first(const uint8_t *payload)
-{
-//	if (!unlocked)
-//		return 0;
-	hash_init();
-	hash_merge(payload, PAYLOAD);
-	flash_start(APP_ADDR);
-	flash_write(payload, PAYLOAD);
-	return 1;
-}
-
-
-static bool fw_more(uint8_t seq, uint8_t limit, const uint8_t *payload)
-{
 	if (!flash_can_write(PAYLOAD))
 		return 0;
-	if (seq != limit) {
-		hash_merge(payload, PAYLOAD);
 	flash_write(payload, PAYLOAD);
-		return 1;
+	if (seq == limit)
-	}
 		flash_end_write();
-	hash_end();
-	if (!hash_eq(payload, PAYLOAD, 0))
-		panic();
 	return 1;
 }
 
 
-static const struct handler fw_proto = {
+bool fw_packet(const uint8_t *buf, uint8_t len)
-	.type	= FIRMWARE,
+{
-	.first	= fw_first,
+	static uint8_t seq = 0;
-	.more	= fw_more,
+	static uint8_t limit;
-};
+	uint8_t ack[] = { FIRMWARE+1, buf[1], buf[2] };
 
+	/* short (barely visible) flash to indicate reception */
+	SET(LED_B7);
+	CLR(LED_B7);
 
-/* ----- Protocol table ---------------------------------------------------- */
+	/* Check packet for formal validity */
 
+	if (len != 64+3)
+		return 0;
+	if (buf[0] != FIRMWARE)
+		return 0;
+	if (buf[1] > buf[2])
+		return 0;
 
-const struct handler *fw_protos[] = {
+	/* Synchronize sequence numbers */
-	&unlock_proto,
+
-	&fw_proto,
+	if (!buf[1]) {
-	NULL
+		seq = buf[1];
-};
+		limit = buf[2];
+	} else {
+		if (buf[2] != limit)
+			return 0;
+		if (buf[1]+1 == seq)
+			goto ack;
+		if (buf[1] != seq)
+			return 0;
+	}
+
+	/* Process the payload */
+
+	if (!fw_payload(buf[1], limit, buf+3))
+		return 0;
+	seq++;
+ack:
+	/* clearly visible short blink to indicate progress */
+	SET(LED_B6);
+	rf_send(ack, sizeof(ack));
+	CLR(LED_B6);
+	return 1;
+}

fw/fw.h

@@ -1,5 +1,5 @@
 /*
- * fw/fw.h - Firmware upload protocols
+ * fw/fw.h - Firmware upload protocol
  *
  * Written 2012 by Werner Almesberger
  * Copyright 2012 Werner Almesberger
@@ -13,9 +13,9 @@
 #ifndef FW_H
 #define	FW_H
 
-#include "proto.h"
+#include <stdint.h>
 
 
-extern const struct handler *fw_protos[];
+bool fw_packet(const uint8_t *buf, uint8_t len);
 
 #endif /* !FW_H */

fw/proto.h

@@ -18,8 +18,6 @@
 enum pck_type {
 	PING		= 0,	/* version query */
 	PONG		= 1,	/* version response */
-	UNLOCK		= 2,	/* unlock firmware upload */
-	UNLOCK_ACK	= 3,	/* unlock acknowledgement */
 	FIRMWARE	= 4,	/* firmware upload */
 	FIRMWARE_ACK	= 5,	/* firmware upload acknowledgement */
 	IMAGE		= 6,	/* image upload */

fw/rf.c

@@ -89,6 +89,9 @@ void rf_send(const void *buf, uint8_t size)
 	reg_write(REG_TRX_STATE, TRX_CMD_PLL_ON);
 	_delay_us(1);	/* tTR9 = 1 us */
 
+	/* be nice to senders with long turn-around time, e.g., atusb */
+	_delay_ms(2);
+
 	spi_begin();
 	spi_send(AT86RF230_BUF_WRITE);
 	spi_send(size+2); /* CRC */
@@ -115,6 +118,9 @@ uint8_t rf_recv(void *buf, uint8_t size)
 {
 	uint8_t irq, len, i;
 
+	if (!PIN(RF_IRQ))
+		return 0;
+
 	irq = reg_read(REG_IRQ_STATUS);
 	if (!(irq & IRQ_TRX_END))
 		return 0;

tools/antorcha.c

@@ -45,13 +45,17 @@ static void rf_init(struct atrf_dsc *dsc, int trim, int channel)
 
 static void rf_send(struct atrf_dsc *dsc, void *buf, int len)
 {
+	uint8_t tmp[MAX_PSDU];
+
+	/* Copy the message to append the CRC placeholders */
+	memcpy(tmp, buf, len);
 	atrf_reg_write(dsc, REG_TRX_STATE, TRX_CMD_PLL_ON);
-	atrf_buf_write(dsc, buf, len);
+	atrf_buf_write(dsc, tmp, len+2);
 	atrf_reg_write(dsc, REG_TRX_STATE, TRX_CMD_TX_START);
 	wait_for_interrupt(dsc, IRQ_TRX_END,
 	    IRQ_TRX_END | IRQ_PLL_LOCK, 10);
 	atrf_reg_write(dsc, REG_TRX_STATE, TRX_CMD_RX_ON);
-#if 1
+#if 0
 int i;
 fprintf(stderr, "\r%d:", len);
 for (i = 0; i != len; i++)
@@ -96,7 +100,7 @@ static void ping(struct atrf_dsc *dsc)
 
 
 static void packet(struct atrf_dsc *dsc,
-    uint8_t type, uint8_t seq, uint8_t last, void *payload, int len)
+    uint8_t type, uint8_t seq, uint8_t last, const void *payload, int len)
 {
 	uint8_t tx_buf[PAYLOAD+3] = { type, seq, last };
 	uint8_t rx_buf[10];
@@ -109,8 +113,11 @@ static void packet(struct atrf_dsc *dsc,
 		if (verbose)
 			write(2, ">", 1);
 		got = rf_recv(dsc, rx_buf, sizeof(rx_buf));
-		if (got <= 0)
+		if (got <= 0) {
+			if (!seq && verbose)
+				write(2, "\b", 1);
 			continue;
+		}
 		if (verbose)
 			write(2, "\b?", 2);
 		if (got < 3)
@@ -126,34 +133,11 @@ static void packet(struct atrf_dsc *dsc,
 }
 
 
-static const uint8_t unlock_secret[] = {
+static const uint8_t unlock_secret[PAYLOAD] = {
 	#include "unlock-secret.inc"
 };
 
 
-static void unlock(struct atrf_dsc *dsc)
-{
-	uint8_t payload[PAYLOAD];
-
-	if (verbose)
-		write(2, "unlock   ", 9);
-	memset(payload, 0, PAYLOAD);
-	hash_init();
-	hash_merge(unlock_secret, sizeof(unlock_secret));
-	hash_merge(payload, PAYLOAD);
-	hash_merge(payload, PAYLOAD);
-	packet(dsc, UNLOCK, 0, 3, payload, PAYLOAD);
-	packet(dsc, UNLOCK, 1, 3, payload, PAYLOAD);
-	hash_end();
-	hash_cp(payload, PAYLOAD, 0);
-	packet(dsc, UNLOCK, 2, 3, payload, PAYLOAD);
-	hash_cp(payload, PAYLOAD, PAYLOAD);
-	packet(dsc, UNLOCK, 3, 3, payload, PAYLOAD);
-	if (verbose)
-		write(2, "\n", 1);
-}
-
-
 static void send_firmware(struct atrf_dsc *dsc, void *buf, int len)
 {
 	uint8_t payload[PAYLOAD];
@@ -163,9 +147,9 @@ static void send_firmware(struct atrf_dsc *dsc, void *buf, int len)
 		write(2, "firmware ", 9);
 	last = (len+63)/64;
 	seq = 0;
+	packet(dsc, FIRMWARE, seq++, last, unlock_secret, PAYLOAD);
 	while (len >= PAYLOAD) {
 		packet(dsc, FIRMWARE, seq++, last, buf, PAYLOAD);
-		hash_merge(buf, PAYLOAD);
 		buf += PAYLOAD;
 		len -= PAYLOAD;
 	}
@@ -173,11 +157,7 @@ static void send_firmware(struct atrf_dsc *dsc, void *buf, int len)
 		memcpy(payload, buf, len);
 		memset(payload+len, 0, PAYLOAD-len);
 		packet(dsc, FIRMWARE, seq++, last, payload, PAYLOAD);
-		hash_merge(payload, PAYLOAD);
 	}
-	hash_end();
-	hash_cp(payload, PAYLOAD, 0);
-	packet(dsc, FIRMWARE, seq, last, payload, PAYLOAD);
 	if (verbose)
 		write(2, "\n", 1);
 }
@@ -201,7 +181,6 @@ static void firmware(struct atrf_dsc *dsc, const char *name)
 	}
 	fclose(file);
 
-	unlock(dsc);
 	send_firmware(dsc, fw, len);
 }