mips: zero-extend microMIPS LWM16/SWM16 reglist offset#2991
Open
Samin061 wants to merge 1 commit into
Open
Conversation
The ISA defines the offset as zero_extend(offset||0^2), but the non-MMR6 path in DecodeMemMMReglistImm4Lsl2 sign-extended the 4-bit field before the << 2. When the field's top bit is set that makes Offset negative, so Offset << 2 is a left shift of a negative value (UB), and the decoded offset came out negative instead of the zero-extended value the ISA requires. Use fieldFromInstruction_4(Insn, 0, 4) to extract the field unsigned, matching the MMR6 branch and the ISA. The shift is now always applied to a non-negative value. Verified under -fsanitize=undefined: before, cstool micromips 4508 and test_poc abort at MipsDisassembler.c:2432 with 'left shift of negative value -8'; after, test_poc exits clean and cstool prints 'lwm16 $s0, $ra, 0x20($sp)' (8 << 2 == 0x20). Signed-off-by: bibi samina <sam@bugqore.com>
2 tasks
Contributor
Author
|
@Rot127 this is the reworked version of #2989. Two things from your review:
I do lean on a tool for research here and there, but I ran the before/after myself to confirm this one holds up. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Your checklist for this pull request
Detailed description
Reopening #2989 with the fix reworked per review. That branch was force-pushed so GitHub won't let it be reopened, hence a fresh PR.
DecodeMemMMReglistImm4Lsl2decodes the offset of the microMIPSLWM16/SWM16instructions. The ISA defines the address as:so the 4-bit
offsetfield is concatenated with two zero bits and zero-extended. The MMR6 path already reads it unsigned viafieldFromInstruction_4, but the non-MMR6 path usedSignExtend32((Insn & 0xf), 4). Whenever the field's top bit is set (field >= 8) that makesOffsetnegative, soOffset << 2is a left shift of a negative value (UB), and the decoded offset came out negative instead of the zero-extended value the ISA requires.The fix reads the field with
fieldFromInstruction_4(Insn, 0, 4), matching the MMR6 branch and the ISA. The shift is then always applied to a non-negative value.Test plan
Built with
-fsanitize=undefined -fno-sanitize-recover=undefinedand confirmed both directions:cstool micromips 4508andtest_pocabort atarch/Mips/MipsDisassembler.c:2432withruntime error: left shift of negative value -8.test_pocexits cleanly andcstool micromips 4508printslwm16 $s0, $ra, 0x20($sp)(8 << 2 == 0x20, the zero-extended value).Added
test_ub_shift_mips_mm_reglisttotests/integration/test_poc.c, next to the existingtest_ub_shift_sh_dsp_psigned-shift regression.Closing issues