undefined behavior in shamir unbitslice()

[dsprenkels]

undefined behavior in shamir unbitslice()

[dsprenkels]

@invd Thank you for the bug report! Sorry for acting on it after such a long time, I have been on vacation.

I feel that the real bug here is that (1 << arr_idx) is an int, instead of a uint32_t, but that does not matter for the fix.

I would personally prefer first fix:

r[arr_idx] |= ((cur >> arr_idx) & 1) << bit_idx;

[dsprenkels]

Note: We should also fix the bitslice part of this bug located at:

sss/hazmat.c

Line 43 in b3ac4e7

r[bit_idx] |= ((cur & (1 << bit_idx)) >> bit_idx) << arr_idx;

[andrewkozlik]

Note: We should also fix the bitslice part of this bug located at:

sss/hazmat.c

Line 43 in b3ac4e7

r[bit_idx] |= ((cur & (1 << bit_idx)) >> bit_idx) << arr_idx;

Technically this is OK, because bit_idx is at most 7 and the minimum size of int is 16 bits. But I agree it would be more readable and consistent to write it in the other form.

[invd]

@dsprenkels: no problem for the delay!
I agree with @andrewkozlik for the patch consideration.
We have the proposed patch in the linked Trezor issue.

[prusnak]

For reference - here's the patch we merged to Trezor codebase: trezor/trezor-firmware@5429acd