search

SonixQMK / qmk_firmware

Open-source keyboard firmware for Atmel AVR and Arm USB families
https://qmk.fm
GNU General Public License v2.0
515 stars 408 forks source link

Fixed out of bounds array access in SN32F24XB RGB matrix #308

Closed JoJoBond closed 1 year ago

JoJoBond commented 1 year ago

Fixed out of bounds array access in SN32F24XB RGB matrix

Description

We get led_index from g_led_config.matrix_co, this is the first part in the g_led_config. When a LED is not present in g_led_config.matrix_co it is marked with NO_LED which has the value 255. Now we use led_index as index on array led_state, but this array is defined with DRIVER_LED_TOTAL elements, which is typically smaller than 255. This is a typical array out of bounds access, that would most likely segfault on x86. But ARM doesn't care and just reads whatever is in memory.

Types of Changes

dexter93 commented 1 year ago

This is causing mayhem with COL2ROW indexing. output is shifted by 1 row. Reverting to keep this branch stable

Noted down however, will be included in the upcoming rgb driver updates

JoJoBond commented 1 year ago

This is causing mayhem with COL2ROW indexing. output is shifted by 1 row. Reverting to keep this branch stable

Noted down however, will be included in the upcoming rgb driver updates

I don't see how that would happen, other than it being a timing issue. The same issue should also happen if some LEDs have to show the value 0x000000 (Black).

dexter93 commented 1 year ago

This is causing mayhem with COL2ROW indexing. output is shifted by 1 row. Reverting to keep this branch stable Noted down however, will be included in the upcoming rgb driver updates

it is a timing issue. the CT registers can only be updated after the period cycle is complete. This should be a bug, but apparently it's the way to go unless the pwm logic gets changed for COL2ROW significantly. You can see some updates in #312 with this in place ( which will replace this driver when merged). Accepting suggestions there

JoJoBond commented 1 year ago

Looks good in #312. Was just confused for a second. No hard feelings as long as the out of bounds read is fixed :)

dexter93 commented 1 year ago

Looks good in #312. Was just confused for a second. No hard feelings as long as the out of bounds read is fixed :)

Always nice to see someone check over a piece of code, thanks for looking into it.