* [PATCH] arm64: fix relocation of movz instruction with negative immediate @ 2016-01-04 16:09 Ard Biesheuvel 2016-01-04 17:21 ` Dave Martin 0 siblings, 1 reply; 5+ messages in thread From: Ard Biesheuvel @ 2016-01-04 16:09 UTC (permalink / raw) To: linux-arm-kernel The test whether a movz instruction with a signed immediate should be turned into a movn instruction (i.e., when the immediate is negative) is flawed, since the value of imm is always positive. So check sval instead. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- arch/arm64/kernel/module.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c index f4bc779e62e8..39e4a29cab50 100644 --- a/arch/arm64/kernel/module.c +++ b/arch/arm64/kernel/module.c @@ -128,7 +128,7 @@ static int reloc_insn_movw(enum aarch64_reloc_op op, void *place, u64 val, * immediate is less than zero. */ insn &= ~(3 << 29); - if ((s64)imm >= 0) { + if (sval >= 0) { /* >=0: Set the instruction to MOVZ (opcode 10b). */ insn |= 2 << 29; } else { -- 2.5.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH] arm64: fix relocation of movz instruction with negative immediate 2016-01-04 16:09 [PATCH] arm64: fix relocation of movz instruction with negative immediate Ard Biesheuvel @ 2016-01-04 17:21 ` Dave Martin 2016-01-04 17:33 ` Ard Biesheuvel 2016-01-04 17:48 ` Will Deacon 0 siblings, 2 replies; 5+ messages in thread From: Dave Martin @ 2016-01-04 17:21 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jan 04, 2016 at 05:09:22PM +0100, Ard Biesheuvel wrote: > The test whether a movz instruction with a signed immediate should be > turned into a movn instruction (i.e., when the immediate is negative) > is flawed, since the value of imm is always positive. So check sval > instead. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/arm64/kernel/module.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c > index f4bc779e62e8..39e4a29cab50 100644 > --- a/arch/arm64/kernel/module.c > +++ b/arch/arm64/kernel/module.c > @@ -128,7 +128,7 @@ static int reloc_insn_movw(enum aarch64_reloc_op op, void *place, u64 val, #define AARCH64_INSN_IMM_MOVNZ AARCH64_INSN_IMM_MAX #define AARCH64_INSN_IMM_MOVK AARCH64_INSN_IMM_16 /* ... */ if (imm_type == AARCH64_INSN_IMM_MOVNZ) { /* ... */ > * immediate is less than zero. > */ > insn &= ~(3 << 29); > - if ((s64)imm >= 0) { > + if (sval >= 0) { > /* >=0: Set the instruction to MOVZ (opcode 10b). */ > insn |= 2 << 29; > } else { I _think_ this may be correct, but... } imm_type = AARCH64_INSN_IMM_MOVK; } /* Update the instruction with the new encoding. */ insn = aarch64_insn_encode_immediate(imm_type, insn, imm); /* ... */ leaves imm_type as either AARCH64_INSN_IMM_16 or AARCH64_INSN_IMM_MOVK. But because AARCH64_INSN_IMM_16 == AARCH64_INSN_IMM_MOVK (required for , the negative overflow fudge is never applied, no? if (imm_type != AARCH64_INSN_IMM_16) { sval++; limit++; } I'm wondering whether there is a less confusing way to do all this... Cheers ---Dave ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] arm64: fix relocation of movz instruction with negative immediate 2016-01-04 17:21 ` Dave Martin @ 2016-01-04 17:33 ` Ard Biesheuvel 2016-01-04 18:00 ` Dave Martin 2016-01-04 17:48 ` Will Deacon 1 sibling, 1 reply; 5+ messages in thread From: Ard Biesheuvel @ 2016-01-04 17:33 UTC (permalink / raw) To: linux-arm-kernel On 4 January 2016 at 18:21, Dave Martin <Dave.Martin@arm.com> wrote: > On Mon, Jan 04, 2016 at 05:09:22PM +0100, Ard Biesheuvel wrote: >> The test whether a movz instruction with a signed immediate should be >> turned into a movn instruction (i.e., when the immediate is negative) >> is flawed, since the value of imm is always positive. So check sval >> instead. >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> arch/arm64/kernel/module.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c >> index f4bc779e62e8..39e4a29cab50 100644 >> --- a/arch/arm64/kernel/module.c >> +++ b/arch/arm64/kernel/module.c >> @@ -128,7 +128,7 @@ static int reloc_insn_movw(enum aarch64_reloc_op op, void *place, u64 val, > > #define AARCH64_INSN_IMM_MOVNZ AARCH64_INSN_IMM_MAX > #define AARCH64_INSN_IMM_MOVK AARCH64_INSN_IMM_16 > > /* ... */ > > if (imm_type == AARCH64_INSN_IMM_MOVNZ) { > > /* ... */ > >> * immediate is less than zero. >> */ >> insn &= ~(3 << 29); >> - if ((s64)imm >= 0) { >> + if (sval >= 0) { >> /* >=0: Set the instruction to MOVZ (opcode 10b). */ >> insn |= 2 << 29; >> } else { > > I _think_ this may be correct, but... > > } > imm_type = AARCH64_INSN_IMM_MOVK; > } > > /* Update the instruction with the new encoding. */ > insn = aarch64_insn_encode_immediate(imm_type, insn, imm); > > /* ... */ > > leaves imm_type as either AARCH64_INSN_IMM_16 or AARCH64_INSN_IMM_MOVK. > > But because AARCH64_INSN_IMM_16 == AARCH64_INSN_IMM_MOVK (required for , the negative > overflow fudge is never applied, no? > > if (imm_type != AARCH64_INSN_IMM_16) { > sval++; > limit++; > } > > > I'm wondering whether there is a less confusing way to do all this... > I hadn't spotted that AARCH64_INSN_IMM_16 == AARCH64_INSN_IMM_MOVK, so you are correct that my single change is not sufficient to fix this code. Let me try to come up with something better .. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] arm64: fix relocation of movz instruction with negative immediate 2016-01-04 17:33 ` Ard Biesheuvel @ 2016-01-04 18:00 ` Dave Martin 0 siblings, 0 replies; 5+ messages in thread From: Dave Martin @ 2016-01-04 18:00 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jan 04, 2016 at 06:33:03PM +0100, Ard Biesheuvel wrote: > On 4 January 2016 at 18:21, Dave Martin <Dave.Martin@arm.com> wrote: > > On Mon, Jan 04, 2016 at 05:09:22PM +0100, Ard Biesheuvel wrote: > >> The test whether a movz instruction with a signed immediate should be > >> turned into a movn instruction (i.e., when the immediate is negative) > >> is flawed, since the value of imm is always positive. So check sval > >> instead. > >> > >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >> --- > >> arch/arm64/kernel/module.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c > >> index f4bc779e62e8..39e4a29cab50 100644 > >> --- a/arch/arm64/kernel/module.c > >> +++ b/arch/arm64/kernel/module.c > >> @@ -128,7 +128,7 @@ static int reloc_insn_movw(enum aarch64_reloc_op op, void *place, u64 val, > > > > #define AARCH64_INSN_IMM_MOVNZ AARCH64_INSN_IMM_MAX > > #define AARCH64_INSN_IMM_MOVK AARCH64_INSN_IMM_16 > > > > /* ... */ > > > > if (imm_type == AARCH64_INSN_IMM_MOVNZ) { > > > > /* ... */ > > > >> * immediate is less than zero. > >> */ > >> insn &= ~(3 << 29); > >> - if ((s64)imm >= 0) { > >> + if (sval >= 0) { > >> /* >=0: Set the instruction to MOVZ (opcode 10b). */ > >> insn |= 2 << 29; > >> } else { > > > > I _think_ this may be correct, but... > > > > } > > imm_type = AARCH64_INSN_IMM_MOVK; > > } > > > > /* Update the instruction with the new encoding. */ > > insn = aarch64_insn_encode_immediate(imm_type, insn, imm); > > > > /* ... */ > > > > leaves imm_type as either AARCH64_INSN_IMM_16 or AARCH64_INSN_IMM_MOVK. > > > > But because AARCH64_INSN_IMM_16 == AARCH64_INSN_IMM_MOVK (required for , the negative > > overflow fudge is never applied, no? > > > > if (imm_type != AARCH64_INSN_IMM_16) { > > sval++; > > limit++; > > } > > > > > > I'm wondering whether there is a less confusing way to do all this... > > > > I hadn't spotted that AARCH64_INSN_IMM_16 == AARCH64_INSN_IMM_MOVK, so > you are correct that my single change is not sufficient to fix this > code. > > Let me try to come up with something better .. I think a simpler flow might go something like u64 uval = (u64)do_reloc(op, place, val); if (is a SABS or PREL reloc that is not _NC, && (uval & (1 << 63))) { change insn to MOVN; uval = ~uval; } /* encode the insn using the bottom 16 bits of uval */ if (not an _NC reloc && (uval >> 16) > 0) return -ERANGE; ...or something along those lines. I haven't tried to implement that yet, though. Cheers ---Dave ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] arm64: fix relocation of movz instruction with negative immediate 2016-01-04 17:21 ` Dave Martin 2016-01-04 17:33 ` Ard Biesheuvel @ 2016-01-04 17:48 ` Will Deacon 1 sibling, 0 replies; 5+ messages in thread From: Will Deacon @ 2016-01-04 17:48 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jan 04, 2016 at 05:21:12PM +0000, Dave Martin wrote: > On Mon, Jan 04, 2016 at 05:09:22PM +0100, Ard Biesheuvel wrote: > > The test whether a movz instruction with a signed immediate should be > > turned into a movn instruction (i.e., when the immediate is negative) > > is flawed, since the value of imm is always positive. So check sval > > instead. > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > --- > > arch/arm64/kernel/module.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c > > index f4bc779e62e8..39e4a29cab50 100644 > > --- a/arch/arm64/kernel/module.c > > +++ b/arch/arm64/kernel/module.c > > @@ -128,7 +128,7 @@ static int reloc_insn_movw(enum aarch64_reloc_op op, void *place, u64 val, > > #define AARCH64_INSN_IMM_MOVNZ AARCH64_INSN_IMM_MAX > #define AARCH64_INSN_IMM_MOVK AARCH64_INSN_IMM_16 > > /* ... */ > > if (imm_type == AARCH64_INSN_IMM_MOVNZ) { > > /* ... */ > > > * immediate is less than zero. > > */ > > insn &= ~(3 << 29); > > - if ((s64)imm >= 0) { > > + if (sval >= 0) { > > /* >=0: Set the instruction to MOVZ (opcode 10b). */ > > insn |= 2 << 29; > > } else { > > I _think_ this may be correct, but... Yeah, I think this is the right thing to do. > } > imm_type = AARCH64_INSN_IMM_MOVK; > } > > /* Update the instruction with the new encoding. */ > insn = aarch64_insn_encode_immediate(imm_type, insn, imm); > > /* ... */ > > leaves imm_type as either AARCH64_INSN_IMM_16 or AARCH64_INSN_IMM_MOVK. > > But because AARCH64_INSN_IMM_16 == AARCH64_INSN_IMM_MOVK (required for , the negative > overflow fudge is never applied, no? > > if (imm_type != AARCH64_INSN_IMM_16) { > sval++; > limit++; > } Hmm, that's a bug introduced by the refactoring of the insn encoding stuff in c84fced8d990 ("arm64: move encode_insn_immediate() from module.c to insn.c"). I've restored the old behaviour below. > I'm wondering whether there is a less confusing way to do all this... Patches welcome! I didn't have an ELF spec when I wrote the original code, so it might be easier now. It would also be handy to have a test module that uses lots of relocs... Will --->8 diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c index 266e7490e85c..6546032bb83b 100644 --- a/arch/arm64/kernel/module.c +++ b/arch/arm64/kernel/module.c @@ -140,11 +140,10 @@ static int reloc_insn_movw(enum aarch64_reloc_op op, void *place, u64 val, */ imm = ~imm; } - imm_type = AARCH64_INSN_IMM_MOVK; } /* Update the instruction with the new encoding. */ - insn = aarch64_insn_encode_immediate(imm_type, insn, imm); + insn = aarch64_insn_encode_immediate(AARCH64_INSN_IMM_16, insn, imm); *(u32 *)place = cpu_to_le32(insn); /* Shift out the immediate field. */ ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-01-04 18:00 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-01-04 16:09 [PATCH] arm64: fix relocation of movz instruction with negative immediate Ard Biesheuvel 2016-01-04 17:21 ` Dave Martin 2016-01-04 17:33 ` Ard Biesheuvel 2016-01-04 18:00 ` Dave Martin 2016-01-04 17:48 ` Will Deacon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).