Bugs in various applications

[mchesser]

I've been working on tools for finding bugs in MSP430 code and had a look at this repo and found a few bugs.

Possible OOB write when writing to dmesg_buffer (putchar)

When writing to the dmesg_buffer it appears to be possible for the code to write one byte out-of -bounds. i.e., in the code below if dmesg_index = DMESGLEN - 1 then the code will write to dmesg_buffer[DMSGLEN] (0x2c00).

goodwatch/firmware/dmesg.c

Lines 25 to 30 in 60673a7

	int putchar(int c){
	dmesg_index++;
	while(dmesg_index>DMESGLEN)
	dmesg_index=dmesg_index-DMESGLEN;
	return dmesg_buffer[dmesg_index]=(char) c;
	}

I'm entirely sure what the MCU will actually do in this case, but since I think there isn't any memory mapped after that point it will generate an S-NMI.

It looks like that the code should be reordered such that the assignment on line 29 should be performed before dmesg_index is incremented.

Default config does not define enough messages for ook button array.

The code at:

goodwatch/firmware/apps/ook.c

Lines 128 to 130 in 60673a7

	if(lastch<='9' && lastch>='0'){
	transmit(lastch-'0');
	}

Transmits a pre-configured OOK packet from button_array when one of the numeric keys (0-9) is held. However, there are 10 buttons but only 9 messages a defined in the default configuration

goodwatch/firmware/configdefault.h

Lines 39 to 52 in 60673a7

	#define OOKBUTTONS \
	/* Magicfly doorbell first, 200us.*/ \
	"\xf7\x93" "\x00\xe8\xe8\x88\x88\xee\x88\x8e\x88\xee\x8e\x88\xee\x80\x00\x00", \
	/* Settings for the Eco-Worthy Digital Motor Controller, ~430us */ \
	"\x86\xd9" "\x00\xee\x8e\x8e\x8e\x8e\x8e\x8e\x8e\x88\x88\x88\xee\x80\x00\x00", \
	"\x86\xd9" "\x00\xee\x8e\x8e\x8e\x8e\x8e\x8e\x8e\x88\x88\xee\x88\x80\x00\x00", \
	"\x86\xd9" "\x00\xee\x8e\x8e\x8e\x8e\x8e\x8e\x8e\x88\xee\x88\x88\x80\x00\x00", \
	"\x86\xd9" "\x00\xee\x8e\x8e\x8e\x8e\x8e\x8e\x8e\xee\x88\x88\x88\x80\x00\x00", \
	/* No-name wireless relay controller. ~340us */ \
	"\x86\xd9" "\x00\x00\xe8\xe8\xee\x88\xe8\x8e\xe8\x88\xee\xe8\x88\x8e\x80\x00", \
	"\x86\xd9" "\x00\x00\xe8\xe8\xee\x88\xe8\x8e\xe8\x88\xee\xe8\x88\xe8\x80\x00", \
	"\x86\xd9" "\x00\x00\xe8\xe8\xee\x88\xe8\x8e\xe8\x88\xee\xe8\x8e\x88\x80\x00", \
	"\x86\xd9" "\x00\x00\xe8\xe8\xee\x88\xe8\x8e\xe8\x88\xee\xe8\xe8\x88\x80\x00"
	#endif

this causes the code to dereference an invalid pointer when button '9' is pressed.

BCD lookup for hours in stopwatch app is invalid after 60 hours

This one is very minor, but since the int2bcd command only supports conversion for the range 0-59, the stopwatch app will start displaying garbage digits for hours if it manages to stay running for over 60 hours, because the hour value is computed using int2bcd and does not rollover.

goodwatch/firmware/apps/stopwatch.c

Line 129 in 60673a7

hourhex=int2bcd(hour);

Bugs in UART debugging interface

I assume that the UART interface is intended for debugging only, but I also found a couple of issues in there as well which might be of interest.

Incorrect handling of zero length messages in handle_rxbyte

Messages sent to the UART interface with the length field set to zero are handled incorrectly. In the MSG state, after storing the current byte at uart_buffer[index] index is incrrementd and compared to length, however since the store and increment of index occurs before comparing against length for zero length messages the index == length condition will never be true:

goodwatch/firmware/uart.c

Lines 156 to 159 in 60673a7

	uart_buffer[index++]=byte;
	if(index==length)
	state=CRCL;
	break;

RANDINT monitor command can request too many values

This one might already be known, but if you request a large number of random numbers to be generated via RANDINT, then the stack allocated buffer rints overflows the area of memory reserved for the stack and starts overwriting random memory:

goodwatch/firmware/monitor.c

Line 58 in 60673a7

uint16_t rints[n]; // are var len arrays ok here?