From c4015e7157ad07ffdd37f794ee77c5695a001059 Mon Sep 17 00:00:00 2001 From: Grond Date: Tue, 23 Jun 2026 16:44:24 -0700 Subject: Fix the handling of reserved bits in uncommon loads/stores There's a pattern of load/store instructions that look something like * `ldr , [, +/- ]` * `str , [, +/- ]` These instructions share a common encoding format, which includes the presence of 4 reserved bits starting at bit 8 in the instruction word. These bits should always be zero, according to `DDI0406C_d_armv7ar_arm.pdf`. However, while this crate did attempt to validate these bits sometimes, the validation was inverted. Instead of checking that the bits were zero, the validation would raise an error if all of the bits were zero! Additionally, some instructions were missing the check and some instructions that did not use the aforementioned encoding scheme incorrectly had the check applied to them. This commit fixes all of the issues I've found along these lines so far. --- CHANGELOG | 1 + src/armv7.rs | 53 +++++++++++++++++++++++++++++++++++------------------ tests/armv7/mod.rs | 24 ++++++++++++++++-------- 3 files changed, 52 insertions(+), 26 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 02c60e5..8229e4d 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -16,6 +16,7 @@ several fixes from @Grond66: yaxpeax-arm incorrectly reported a shift of 0, but should have reported 32. * ARMv7: reject unconditional instructions with op1=1111xxxx. these were decoded as CDP, MCR, or MRC when the bit pattern is actually undefined. +* ARMv7: fix inverted reserved bit test for some ldr*/str* (immediate) forms. thank you for the patches! diff --git a/src/armv7.rs b/src/armv7.rs index 2901508..8b55e65 100644 --- a/src/armv7.rs +++ b/src/armv7.rs @@ -1822,6 +1822,14 @@ impl Decoder for InstDecoder { } else { inst.opcode = Opcode::STRH; } + // According to the encoding A1 of STRH described at + // A8-703, and the encoding A2 of STRHT described at + // A8-705, these reserved bits must be zero. + if self.should_is_must { + if (word >> 8) as u8 & 0b1111 != 0 { + return Err(DecodeError::Nonconforming); + } + } inst.operands = [ Operand::Reg(Reg::from_u8(Rd)), if P { @@ -1842,6 +1850,14 @@ impl Decoder for InstDecoder { } else { inst.opcode = Opcode::LDRH; } + // According to the encoding A1 of LDRH described at + // A8-447, and the encoding A2 of LDRHT described at + // A8-449, these reserved bits must be zero. + if self.should_is_must { + if (word >> 8) as u8 & 0b1111 != 0 { + return Err(DecodeError::Nonconforming); + } + } inst.operands = [ Operand::Reg(Reg::from_u8(Rd)), if P { @@ -1856,7 +1872,7 @@ impl Decoder for InstDecoder { ]; } 0b00100 => { - // STRHT or STRH + // STRHT or STRH (immediate) if !P && W { // flags == 0b0x110 inst.opcode = Opcode::STRHT; } else { @@ -1877,7 +1893,7 @@ impl Decoder for InstDecoder { ]; } 0b00101 => { - // LDRHT or LDRH + // LDRHT or LDRH (immediate) if !P && W { // flags == 0b0x111 inst.opcode = Opcode::LDRHT; } else { @@ -1919,7 +1935,9 @@ impl Decoder for InstDecoder { inst.opcode = Opcode::LDRD; } if self.should_is_must { - if (word >> 8) & 0b1111 == 0 { + // According to encoding A1 of LDRD described in + // A8-431, these bits should be zero. + if (word >> 8) & 0b1111 != 0 { return Err(DecodeError::Nonconforming); } } @@ -1952,7 +1970,10 @@ impl Decoder for InstDecoder { inst.opcode = Opcode::LDRSB; } if self.should_is_must { - if (word >> 8) & 0b1111 == 0 { + // According to the A1 encoding of LDRSB described in + // A8-455, and the A2 encoding of LDRSBT described in + // A8-457, these reserved bits should be zero. + if (word >> 8) & 0b1111 != 0 { return Err(DecodeError::Nonconforming); } } @@ -2000,17 +2021,12 @@ impl Decoder for InstDecoder { ]; }, 0b00101 => { - // LDRSB or LDRSBT + // LDRSB or LDRSBT (immediate) if !P && W { // flags == 0b0x010 inst.opcode = Opcode::LDRSBT; } else { inst.opcode = Opcode::LDRSB; } - if self.should_is_must { - if (word >> 8) & 0b1111 == 0 { - return Err(DecodeError::Nonconforming); - } - } let Rt = (word >> 12) as u8 & 0b1111; let Rn = (word >> 16) as u8 & 0b1111; let imm = (HiOffset << 4) as u16 | LoOffset as u16; @@ -2044,8 +2060,11 @@ impl Decoder for InstDecoder { } else { inst.opcode = Opcode::STRD; } + if self.should_is_must { - if (word >> 8) & 0b1111 == 0 { + // According to encoding A1 of STRD described at + // A8-689, these reserved bits should be zero. + if (word >> 8) & 0b1111 != 0 { return Err(DecodeError::Nonconforming); } } @@ -2075,7 +2094,10 @@ impl Decoder for InstDecoder { inst.opcode = Opcode::LDRSH; } if self.should_is_must { - if (word >> 8) & 0b1111 == 0 { + // According to encoding A1 of LDRSH described at + // A8-463, and encoding A2 of LDRSHT described at + // A8-465, these reserved bits should be zero. + if (word >> 8) & 0b1111 != 0 { return Err(DecodeError::Nonconforming); } } @@ -2123,17 +2145,12 @@ impl Decoder for InstDecoder { ]; }, 0b00101 => { - // LDRSH or LDRSHT + // LDRSH or LDRSHT (immediate) if !P && W { // flags == 0b0x010 inst.opcode = Opcode::LDRSHT; } else { inst.opcode = Opcode::LDRSH; } - if self.should_is_must { - if (word >> 8) & 0b1111 == 0 { - return Err(DecodeError::Nonconforming); - } - } let Rt = (word >> 12) as u8 & 0b1111; let Rn = (word >> 16) as u8 & 0b1111; let imm = (HiOffset << 4) as u16 | LoOffset as u16; diff --git a/tests/armv7/mod.rs b/tests/armv7/mod.rs index 7fff4a8..fc909af 100644 --- a/tests/armv7/mod.rs +++ b/tests/armv7/mod.rs @@ -220,24 +220,32 @@ fn test_decode_str_ldr() { test_all([0x10, 0x00, 0x3f, 0xe4], "ldrt r0, [pc], -0x10"); test_all([0x10, 0x00, 0x4f, 0xe4], "strb r0, [pc], -0x10"); // Extra load/store instructions A5.2.8, page A5-201 - test_all([0xbb, 0x38, 0xa5, 0xe1], "strh r3, [r5, fp]!"); - test_all([0xbb, 0x38, 0xb5, 0xe1], "ldrh r3, [r5, fp]!"); + test_all([0xbb, 0x30, 0xa5, 0xe1], "strh r3, [r5, fp]!"); + test_all([0xbb, 0x30, 0xb5, 0xe1], "ldrh r3, [r5, fp]!"); test_all([0xbb, 0x38, 0xe5, 0xe1], "strh r3, [r5, 0x8b]!"); test_all([0xbb, 0x38, 0xf5, 0xe1], "ldrh r3, [r5, 0x8b]!"); - test_armv5([0xdb, 0x48, 0xa6, 0xe1], "ldrd r4, r5, [r6, fp]!"); - test_invalid([0xdb, 0x38, 0xa5, 0xe1]); - test_all([0xdb, 0x38, 0xb5, 0xe1], "ldrsb r3, [r5, fp]!"); + test_armv5([0xdb, 0x40, 0xa6, 0xe1], "ldrd r4, r5, [r6, fp]!"); + test_invalid([0xdb, 0x30, 0xa5, 0xe1]); + test_all([0xdb, 0x30, 0xb5, 0xe1], "ldrsb r3, [r5, fp]!"); test_armv5([0xdb, 0x48, 0xe6, 0xe1], "ldrd r4, r5, [r6, 0x8b]!"); test_invalid([0xdb, 0x38, 0xe5, 0xe1]); test_all([0xdb, 0x38, 0xf5, 0xe1], "ldrsb r3, [r5, 0x8b]!"); test_invalid([0xfb, 0x38, 0xa5, 0xe1]); - test_all([0xfb, 0x48, 0xa6, 0xe1], "strd r4, r5, [r6, fp]!"); - test_all([0xfb, 0x38, 0xb5, 0xe1], "ldrsh r3, [r5, fp]!"); - test_invalid([0xfb, 0x38, 0xe5, 0xe1]); + test_all([0xfb, 0x40, 0xa6, 0xe1], "strd r4, r5, [r6, fp]!"); + test_all([0xfb, 0x30, 0xb5, 0xe1], "ldrsh r3, [r5, fp]!"); + test_invalid([0xfb, 0x30, 0xe5, 0xe1]); test_all([0xfb, 0x48, 0xe6, 0xe1], "strd r4, r5, [r6, 0x8b]!"); test_all([0xfb, 0x38, 0xf5, 0xe1], "ldrsh r3, [r5, 0x8b]!"); test_all([0xfb, 0x38, 0xff, 0xe1], "ldrsh r3, [pc, 0x8b]!"); + // load/store have had issues about which bits are reserved and which aren't. the + // immediate-offset forms (encoding A1 of LDRSH and LDRSB) had incorrect checks for bits 8..11 + // being 0 when they are simply part of the immediate (as seen above as well!). test both with + // bits in this range set and with all bits clear. + test_all([0xdb, 0x38, 0xf5, 0xe1], "ldrsb r3, [r5, 0x8b]!"); + test_all([0xfb, 0x38, 0xf5, 0xe1], "ldrsh r3, [r5, 0x8b]!"); + test_all([0xfb, 0x30, 0xf5, 0xe1], "ldrsh r3, [r5, 0xb]!"); + test_all([0xdb, 0x40, 0xf5, 0xe1], "ldrsb r4, [r5, 0xb]!"); } #[test] -- cgit v1.1