fix(hid): Correct off-by-one buffer overflow with NKRO

[khoek]

We correct an off-by-one buffer overflow I found while diagnosing #2257. The problem is that we should be rounding up the computed key array length; consider the case where we want to show 10 keys, i.e. ZMK_HID_KEYBOARD_NKRO_MAX_USAGE is 9 and so we need 10 bits (because this range is inclusive); then we only use an array of length 1, which doesn't have enough space.

(An actual buffer overflow by one byte occurs e.g. here:

zmk/app/src/hid.c

Line 151 in 20df200

TOGGLE_KEYBOARD(usage, 1);

but in any case the 1 will not be observed because the number of bytes transmitted to the USB endpoint will be too small.)

[petejohanson]

We correct an off-by-one buffer overflow I found while diagnosing #2257. The problem is that we should be rounding up the computed key array length; consider the case where we want to show 10 keys, i.e. ZMK_HID_KEYBOARD_NKRO_MAX_USAGE is 9 and so we need 10 bits (because this range is inclusive); then we only use an array of length 1, which doesn't have enough space.

(An actual buffer overflow by one byte occurs e.g. here:

zmk/app/src/hid.c

Line 151 in 20df200

TOGGLE_KEYBOARD(usage, 1);

but in any case the 1 will not be observed because the number of bytes transmitted to the USB endpoint will be too small.)

I generally like the fix proposed, but your analysis of the problem has a me a bit confused, so want to be sure we're on the same page here:

ZMK_HID_KEYBOARD_NKRO_MAX_USAGE in this case is the HID usage value, e.g. HID_USAGE_KEY_KEYPAD_EQUAL/0x67 used for a bitmask NKRO HID report to transmit the state of a whole range of HID values by using a "bit-per-usage" report structure. Far more than 6 or 10 usages can be sent all at once.

We also support a HKRO (defaulted to 6) which sends a collection of individual usages in an H-sized list/array to the host, where each entry in that array is the usage value itself, but that is not the code in question here to which you've made changes.

So... back to your fix. Your fix seems to address the case where we have a usage that is not exactly divisible by 8, and this will truncate instead of rounding up as need to actually store the higher order bits.

But our currently selected possible max usages lead to a division by 8 with no remainder:

#if IS_ENABLED(CONFIG_ZMK_HID_KEYBOARD_NKRO_EXTENDED_REPORT)
#define ZMK_HID_KEYBOARD_NKRO_MAX_USAGE HID_USAGE_KEY_KEYBOARD_LANG8
#else
#define ZMK_HID_KEYBOARD_NKRO_MAX_USAGE HID_USAGE_KEY_KEYPAD_EQUAL
#endif

This leads to possible values of 0x67 and 0x97 (which have one added then divided by 8, leading to either 13 or 19 bytes of storage used, respectively.

So... I don't see how with the currently possible ZMK_HID_KEYBOARD_NKRO_MAX_USAGE values this bug could get hit? Is there some aspect to this I've missed?

Thanks again.

[khoek]

@petejohanson Thank you for your exhaustive analysis and I apologize that my sparseness lead you to need to write it for me! I completely agree with you and am sorry I used an illustrative example with a small number of bits which isn't going to come up in practice at all, and is easily confused with HKRO.

I also would have acknowledged the important fact that that this cannot currently trigger in practice because of the divisibility reason you have described, but alas I was confused because of recently dealing with an older version of ZMK when ZMK_HID_KEYBOARD_NKRO_MAX_USAGE still was a naked #define (prior to 9bacaff), and could more easily have been changed to anything larger in the future. Thanks for clarifying that too.

[petejohanson]

Glad we're all clear on this. Fix is reasonable and would prevent future issues should we allow changing the max usage later.