Portability and general behavior issues

[Gh76-wq]

Issue Description

I hope this message finds you well. At TrustInSoft we have tested Keychron QMK using our exhaustive static analysis tools, TrustInSoft Analyzer to verify if the code has any undefined behaviors.
We are pleased to inform you that we found zero undefined behaviors when testing Trampoline-OSEK.
However, we did find a few issues related to portability as well as general behavior, which I will list below. We’re happy to share the diffs proposed for corrective action.
List of identified issues:
Location: lib/chibios/os/hal/ports/STM32/LLD/GPIOv3/hal_pal_lld.c::_pal_lld_setgroupmode()
Issue: invalid signed integer shift in m1and m2 variables.
Proposed correction: switching constants to unsigned

Location: quantum/rgb_matrix/animations/pixel_rain_anim.h::PIXEL_RAIN()
Issue: use of GCC-specific inline sub-functions. Even clang does not support this construct.
Proposed correction: inline the code already. This reduces the code size, and may increase speed.

Location: platforms/chibios/eeprom_stm32_l4.c::eeprom_read_block(),eeprom_write_block()
Issue: invalid pointer calculation. The overarching issue is that eeprom offset is passed as a pointer, but contains only an offset value. Keeping calculations always on pointers cause some undefined behavior in the two functions.
Proposed correction: at the first line of the function, cast the addr back to a uint16_t, then do regular unsigned integer calculations. This removes several casts in the code and increases readability.

Location: quantum/action_util.c::is_oneshot_layer_active()
Issue: invalid uint8_t to bool downcast. The function get_oneshot_layer_state may return other values than 0 or 1, leading to values undefined in the _Bool type.
Proposed correction: prepend the function call in the function by a !! to reduce the integer to a boolean.
Location: quantum/action.c::process_action()
Issue: invalid shift in switch case ACT_LAYER, based on calculation of the local variable shift. The multiplication by 4 is unwarranted as the part already identifies a valid value.
Proposed correction: Remove * 4in both cases where action.layer_bitop_on is tested.

Location: quantum/rgb_matrix/rgb_matrix.c::rgb_matrix_map_row_column_to_led()
Issue: led_count may return NO_LED, which is an unexpected value in higher call stack levels.
Proposed solution: add an else clause to set led_count to 0 if led_count was set to NO_LED

Location: quantum/keymap_common.c::action_for_keycode()
Issue: action_t is a union, but initializer does not contain any explicit initializer for any item of the union. The expression action_t action = {} misleadingly does NOT initialize the variable, but just refers to an uninitialized contents of the union.
Proposed solution: Change the line to action_t action = {0}; to explicitly set to zero.

To provide full transparency and demonstrate our findings, TrustInSoft will include our analysis and findings about Keychron QMK and Chibios on our online series Code Unboxed by TrustInSoft on December 12, 2024.
We understand the importance of security in software development and wanted to provide you with this information ahead of our livestream. If you have any questions or need more assistance, please contact us at TrustInSoft.