GameWithPixels / DiceFirmware

Pixels dice firmware code.
MIT License
15 stars 1 forks source link

instant_anim_controller doesn't validate that the BulkData size matches it's buffer size #88

Closed ChadNedzlek closed 7 months ago

ChadNedzlek commented 7 months ago

Around here: https://github.com/GameWithPixels/DiceFirmware/blob/main/src/modules/instant_anim_controller.cpp#L120

I had miscalculated the size of my animation messages (I forgot that it was "paletteSize" and not "paletteCount"). The instant animation controller doesn't actually validate the bulk data it's receiving is the correct size, cause it to just sort of spill memory (and silently accepts the bad data). I'm not sure what else is floating around in flash memory near the animation bits, but it seems dangerous to just arbitrarily write data there).

It should probably check that and do something (either send some sort of error message back, or at least refuse the transfer ack).

I haven't figured out how to compile/test the firmware in this repo, or I'd attempt to fix it myself.

ChadNedzlek commented 7 months ago

I'm very concerned what would happen if I sent "zero" animations, and then just dumped a megabyte of "bulkdata" into the data transfer, since it's just going to spill out of whatever the "animationData" buffer allocated into adjacent memory. Unless there is some really magic bounds checking happening.

I think this is why when I was trying to set animations, the die was doing some sort of error state (did a weird blue flash on 20, rainbow flash on 20, then rainbow cycle the whole die). I was sending more data than it expected (by only 4 bytes, so presumably the damage I was doing the firmware memory was negligable)

ChadNedzlek commented 7 months ago

I think it should be relatively easy to use the "size" values in the callback and return null if they don't match/overflow? the BulkDataRecieve seems to handle that correctly. It's been a solid decade since I did any native coding though.