Qwiic Fan Controller Firmware Bugs

[dlkeng]

In reviewing and testing the "Qwiic_Fan_Controller.ino" firmware, I have observed and run into the following bugs:

1. In function updateSettings(): line 266: What if registerMap[WIRE_ADDR] is an invalid I2C address? The receiveEvent() allows any value to be written to any register!
2. In function requestEvent(): line 388: requestEvent() always writes 17 bytes. What if registerIndex is greater than zero? It will then return bytes beyond the end of registerMap[]! (not fatal, but not good!)
3. In function receiveEvent(): line 435:
   • What if receivedCommands[0] is an invalid register address? It will then allow bytes to be written beyond the end of registerMap[]!
   • What if bytesReceived is greater than MAX_SENT_BYTES? It will then allow whatever is in the memory beyond the end of receivedCommands[] to be written to registerMap[]!
   • What prevents writing beyond the end of registerMap[]? For example, assume receivedCommands[0] is TRIMMER_DISABLE and multiple bytes were sent. Any received bytes after the first one would be written to memory beyond the end of registerMap[]!

In summary, the receiveEvent() callback function has some serious memory integrity issues!

[NPoole]

Thanks for the code review. Yeah this all looks about right.

1. I removed address validation during debugging and forgot to put it back
2. This is designed to wrap around so that you can read sequentially from every register but I forgot to implement the actual looping part. The fact that it always writes the entire register map is a consequence of the way the wire library ISRs work. It should be OK once it loops instead of wandering off into unknown memory.
3. Same problem as 2 in reverse. I'll implement cyclic writes and register validation.

I'll correct these issues and make the new firmware available as soon as I can.

[NPoole]

v11 addresses all of these issues to my satisfaction. Please feel free to check it out,

• Any address may be written to registerMap[WIRE_ADDR] but the address is validated before applying the change and committing it to EEPROM. If the address is outside of the valid range (0x08 to 0x77 inclusive) the address change is ignored and the register contents are restored to the current working address.
• requestEvent() now writes all register values starting from the requested register to the end of the map and stops
• receiveEvent() can no longer write past the end of the register map, it will wrap around when it reaches the end

Please also keep in mind that the current firmware now compiles to 8094 bytes and the target uC only has 8192 bytes of progmem. While some features/fixes may not be robust, trade-offs have to be made. I believe (with the addition of i2c address validation) that any wayward register writes could now, in the worst case, be corrected by power cycling the device, which may not be a high bar to clear, but thanks to your testing we now clear it.