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).
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.
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.
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:
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:
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_bufferit appears to be possible for the code to write one byte out-of -bounds. i.e., in the code below ifdmesg_index = DMESGLEN - 1then the code will write todmesg_buffer[DMSGLEN](0x2c00).https://github.com/travisgoodspeed/goodwatch/blob/60673a72eaed8878cf79b56a9efcb623a1e7d2f4/firmware/dmesg.c#L25-L30
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_indexis incremented.Default config does not define enough messages for
ookbutton array.The code at:
https://github.com/travisgoodspeed/goodwatch/blob/60673a72eaed8878cf79b56a9efcb623a1e7d2f4/firmware/apps/ook.c#L128-L130
Transmits a pre-configured OOK packet from
button_arraywhen one of the numeric keys (0-9) is held. However, there are 10 buttons but only 9 messages a defined in the default configuration https://github.com/travisgoodspeed/goodwatch/blob/60673a72eaed8878cf79b56a9efcb623a1e7d2f4/firmware/configdefault.h#L39-L52 this causes the code to dereference an invalid pointer when button '9' is pressed.BCD lookup for hours in
stopwatchapp is invalid after 60 hoursThis one is very minor, but since the
int2bcdcommand only supports conversion for the range 0-59, thestopwatchapp will start displaying garbage digits for hours if it manages to stay running for over 60 hours, because the hour value is computed usingint2bcdand does not rollover.https://github.com/travisgoodspeed/goodwatch/blob/60673a72eaed8878cf79b56a9efcb623a1e7d2f4/firmware/apps/stopwatch.c#L129
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_rxbyteMessages sent to the UART interface with the length field set to zero are handled incorrectly. In the
MSGstate, after storing the current byte atuart_buffer[index]indexis incrrementd and compared tolength, however since the store and increment of index occurs before comparing against length for zero length messages theindex == lengthcondition will never be true:https://github.com/travisgoodspeed/goodwatch/blob/60673a72eaed8878cf79b56a9efcb623a1e7d2f4/firmware/uart.c#L156-L159
RANDINTmonitor command can request too many valuesThis one might already be known, but if you request a large number of random numbers to be generated via
RANDINT, then the stack allocated bufferrintsoverflows the area of memory reserved for the stack and starts overwriting random memory:https://github.com/travisgoodspeed/goodwatch/blob/60673a72eaed8878cf79b56a9efcb623a1e7d2f4/firmware/monitor.c#L58