Skip to content

mips: zero-extend microMIPS LWM16/SWM16 reglist offset#2991

Open
Samin061 wants to merge 1 commit into
capstone-engine:nextfrom
Samin061:mips-mm-reglist-neg-shift
Open

mips: zero-extend microMIPS LWM16/SWM16 reglist offset#2991
Samin061 wants to merge 1 commit into
capstone-engine:nextfrom
Samin061:mips-mm-reglist-neg-shift

Conversation

@Samin061

@Samin061 Samin061 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Your checklist for this pull request

  • I've documented or updated the documentation of every API function and struct this PR changes.
  • I've added tests that prove my fix is effective or that my feature works (if possible)

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.

DecodeMemMMReglistImm4Lsl2 decodes the offset of the microMIPS LWM16/SWM16 instructions. The ISA defines the address as:

vAddr = zero_extend(offset||0^2) + GPR[sp]

so the 4-bit offset field is concatenated with two zero bits and zero-extended. The MMR6 path already reads it unsigned via fieldFromInstruction_4, but the non-MMR6 path used SignExtend32((Insn & 0xf), 4). Whenever the field's top bit is set (field >= 8) 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.

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=undefined and confirmed both directions:

  • before: cstool micromips 4508 and test_poc abort at arch/Mips/MipsDisassembler.c:2432 with runtime error: left shift of negative value -8.
  • after: test_poc exits cleanly and cstool micromips 4508 prints lwm16 $s0, $ra, 0x20($sp) (8 << 2 == 0x20, the zero-extended value).

Added test_ub_shift_mips_mm_reglist to tests/integration/test_poc.c, next to the existing test_ub_shift_sh_dsp_p signed-shift regression.

Closing issues

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>
@Samin061

Samin061 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

@Rot127 this is the reworked version of #2989. Two things from your review:

  • Switched the non-MMR6 path to fieldFromInstruction_4(Insn, 0, 4) instead of a raw mask, so it reads the field unsigned the same way the MMR6 branch and the ISA (zero_extend(offset||0^2)) do.
  • Validated both directions under -fsanitize=undefined: on the old code cstool micromips 4508 and test_poc abort at MipsDisassembler.c:2432 with 'left shift of negative value -8'; with the fix test_poc exits clean and 4508 decodes to lwm16 $s0, $ra, 0x20($sp) (8 << 2 == 0x20).

I do lean on a tool for research here and there, but I ran the before/after myself to confirm this one holds up.

@github-actions github-actions Bot added the Mips Arch label Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Mips Arch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant