Skip to content
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

Fix signed integer overflow in RV32IM #324

Merged
merged 2 commits into from
Jan 8, 2024

Conversation

visitorckw
Copy link
Collaborator

@visitorckw visitorckw commented Jan 6, 2024

UBSAN has identified several signed integer overflow issues in the RV32IM implementation, including incorrect type conversions to int32_t for add/sub operations and a lack of prevention for overflow in multiplication.

Address these issues by ensuring proper type handling and preventing integer overflow in the RV32IM implementation.

The implementation of add, sub, and addi instructions incorrectly used
int32_t for arithmetic operations, leading to signed integer overflow.
Address the issue by maintaining the uint32_t type for arithmetic
operations, ensuring compliance with the laws of arithmetic modulo 2^n.
This approach prevents undefined behavior resulting from signed integer
overflow.
The current implementation of the mul instruction does not guard
against integer overflow, potentially leading to undefined behavior.
Cast the operands to int64_t before performing the multiplication to
ensure that the result can be accommodated without overflow. The lower
32 bits of the product are then extracted, preserving the correct
uint32_t type.
@jserv jserv requested a review from qwe661234 January 7, 2024 01:52
@jserv
Copy link
Contributor

jserv commented Jan 7, 2024

I defer to @qwe661234 for conformation. Meanwhile, I am thinking of enabling more checks about undefined behavior in CI pipeline.

@qwe661234
Copy link
Collaborator

This modification seems reasonable, but we need a test case to verify it.

@visitorckw
Copy link
Collaborator Author

visitorckw commented Jan 7, 2024

I observe this undefined behavior in several prebuilt ELF binaries. Taking scimark2.elf as an example, by adding -g -fsanitize=undefined -fno-sanitize=alignment to CFLAGS and LDFLAGS in the Makefile, recompiling, and executing, we can observe the following errors:

src/rv32_template.c:1052:1: runtime error: signed integer overflow: 41505 * 63875 cannot be represented in type 'int'
src/rv32_template.c:733:1: runtime error: signed integer overflow: -1833556764 + -1620771432 cannot be represented in type 'int'
src/rv32_template.c:746:1: runtime error: signed integer overflow: -2095260944 - 984558896 cannot be represented in type 'int'

@visitorckw
Copy link
Collaborator Author

visitorckw commented Jan 7, 2024

Additionally, if the -fno-sanitize=alignment is removed, UBSAN will complain about a load of misaligned address caused by io.c.

src/io.c:84:12: runtime error: load of misaligned address 0x7f0b7abff92e for type 'const uint32_t', which requires 4 byte alignment

@visitorckw
Copy link
Collaborator Author

I defer to @qwe661234 for conformation. Meanwhile, I am thinking of enabling more checks about undefined behavior in CI pipeline.

Perhaps we can consider incorporating UBSAN along with prebuilt ELF binaries for testing in CI. Alternatively, we may reconsider using a fuzzer for detection.

See: #267

@jserv
Copy link
Contributor

jserv commented Jan 7, 2024

Perhaps we can consider incorporating UBSAN along with prebuilt ELF binaries for testing in CI. Alternatively, we may reconsider using a fuzzer for detection.

Cc. @henrybear327

@jserv jserv merged commit 7dec1c1 into sysprog21:master Jan 8, 2024
6 checks passed
@visitorckw visitorckw deleted the fix-signed-integer-overflow branch January 8, 2024 12:47
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.

3 participants