[PATCH v2 04/15] Add code to read and decrypt OSTC3 firmware files

[Anton Lundin]

On 19 December, 2014 - Jef Driesen wrote:

> On 2014-12-17 23:11, Anton Lundin wrote:
> >+// This is a variant of fletcher16 with 16 bit sum instead of 8bit
> >+// and modulo 256 instead of 255
> >+static unsigned int hw_ostc3_firmware_checksum(hw_ostc3_firmware_t
> >*firmware)
> 
> The comment is wrong. It's modulo 65536 (2^16) vs 65535 (2^16-1) due to the
> 16bit sum.
> 

Yea, you're correct. I just wanted to document that its not fletcher16
or fletcher32, but some sort of in-between.


> >+static dc_status_t
> >+hw_ostc3_firmware_readline(FILE *fp, unsigned int addr, unsigned char
> >data[], unsigned int size)
> >+{
> >+	unsigned char ascii[40];
> >+	// 1 byte :, 6 bytes addr, X*2 bytes hex -> X bytes data.
> >+	const 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_DATAFORMAT;
> >+	// Is it a CRLF file?
> >+	// Read away that trailing LF
> >+	if (ascii[line_size] == '\r')
> >+		if (fread (ascii + line_size, sizeof(char), 1, fp) != 1)
> >+			return DC_STATUS_DATAFORMAT;
> >+	if (array_convert_hex2bin(ascii + 1, 6, faddr_byte,
> >sizeof(faddr_byte)) != 0) {
> >+		return DC_STATUS_DATAFORMAT;
> >+	}
> >+	faddr = array_uint24_be (faddr_byte);
> >+	if (faddr != addr)
> >+		return DC_STATUS_DATAFORMAT;
> >+	if (array_convert_hex2bin (ascii + 1 + 6, size*2, data, size) != 0)
> >+		return DC_STATUS_DATAFORMAT;
> >+
> >+	return DC_STATUS_SUCCESS;
> >+}
> 
> There are two bugs in this code. You read line_size bytes, and then try to
> access ascii[line_size], which is left uninitialized. The ascii array is
> also not large enough to store CRLF, resulting in a buffer overflow.
> 

There are two -1 that was missing there to make the code do what i
ment for it to do.

I never ment to store CRLF, only LF, so if there is a CRLF ending, throw
away the CR.

> I have attached a different implementation, which is based on the existing
> ihex code. It can handle arbitrary line endings, and not just CRLF or LF. So
> if you don't mind, I prefer to use this version.
> 

I actually think its uglier, and thats why i didn't copy that pattern.
But if thats your preferred way to read data, you're the maintainer.


And... a bit more thinking. The firmware files are distributed inside
zip-files. If you do anything to them, i would say its a feature to
reject that file. The odds of flipping some bits that decrypt into
something that matches the checksum are pretty slim, but i wouldn't
trust a firmware file that had a blank line in it...


//Anton


-- 
Anton Lundin	+46702-161604