Skip to content

All: Fix left-shifts of signed vs unsigned constants #2801

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

AndreasFuchsTPM
Copy link
Member

@AndreasFuchsTPM AndreasFuchsTPM commented Apr 9, 2024

In order to silence -fsanitize=undefined all left shifts of constants are now cast before shifting, in order to avoid undefined behavior, if the target variable is unsigned.

Fixes: #2799

Copy link
Member

@JuergenReppSIT JuergenReppSIT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment.

selection[1] |= (1 << (pcr_index[i] - 8)) % 256;
selection[2] |= (1 << (pcr_index[i] - 16)) % 256;
selection[3] |= (1 << (pcr_index[i] - 24)) % 256;
selection[0] |= (((UINT32)1) << pcr_index[i]) % 256;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not be used here UINT8?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because we want to shift for up to 24 bits to the left. So we need 32bit.
Then we cast it down to BYTE using modulo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndreasFuchsTPM yes that's true. There are still some cases in the test files and one in mpsse.c.
grep -R "1 << [^0-9]" .
Should they also be changed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mpsse.c file should be ok for the time being. In ReadBits() and WriteBits(), the code restricts the max left shift to 8 bits. Regarding PinState(), the max left shift is up to 12+4 bits (currently only 12 GPIOs are defined by libmpsse: https://github.com/devttys0/libmpsse/blob/master/docs/README.GPIO).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the remaining shifts in the test files (displayed by grep). They also should be ok.

@wxleong
Copy link
Member

wxleong commented Apr 11, 2024

Reference: https://github.com/AGHABEY/Books/blob/master/c-in-a-nutshell-o-reilly-peter-prinz-tony-crawford.pdf

I believe the error flagged by "-fsanitize=undefined" is due to the integer promotion operation.

Considering the integer promotion rules, operations like ((uint8_t)1 << x) are ineffective/not necessary. This is due to the integer promotion:

  • The operands of the shift operators must be integers. Before the actual bit-shift, the integer promotions are performed on both operands.
  • Any operand whose type ranks lower than int is automatically converted to the type int.

Type ranks:

  • The standard integer types are ranked in the order:
    _Bool < char < short < int < long < long long
  • Each signed integer type has the same rank as the corresponding unsigned

The correct implementation should be: (__output_datatype__)((unsigned int)1 << x). Using unsigned int explicitly prevents the integer promotion operation from occurring, as unsigned int is of the same type rank as int.

You can experiment with:

#include <stdint.h>
int main() {
    uint8_t var1 = (uint8_t)(((uint8_t)1) << 31); // Error flagged by sanitizer
    int8_t var2 = ((int8_t)1) << 7;
    int8_t var3 = (int8_t)(((int8_t)1) << 30);
    int var4 = ((int8_t)1) << 30;
    int var5 = 1 << 31; // Error flagged by sanitizer
    int var6 = ((unsigned int)1) << 31;
    return 0;
}
$ gcc -fsanitize=undefined test.c`
$ ./a.out
test.c:3:43: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'
test.c:7:18: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'

In order to silence -fsanitize=undefined all left shifts of constants
are now cast before shifting, in order to avoid undefined behavior,
if the target variable is unsigned.

Signed-off-by: Andreas Fuchs <andreas.fuchs@infineon.com>
@AndreasFuchsTPM AndreasFuchsTPM merged commit fdb3594 into tpm2-software:master Apr 12, 2024
25 checks passed
@AndreasFuchsTPM AndreasFuchsTPM deleted the fix_shiftcasts branch April 12, 2024 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use of TPM2_HR_PERSISTENT triggers undefined behaviour
3 participants