arm64: fix relocation of movz instruction with negative immediate

PATCH New Ard Biesheuvel 3 participants 5 messages Jan 4, 2016 arm64

Share & tools

Ard Biesheuvel

author

2016-01-04 16:09 UTC #0 lore raw

PATCH 1/1 arm64: fix relocation of movz instruction with negative immediate

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 <[email protected]>

---
 arch/arm64/kernel/module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/kernel/module.c +1 -1

index f4bc779e62e8..39e4a29cab50 100644
@@ -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 {

Signature · 1 line

2.5.0

Dave Martin

2016-01-04 17:21 UTC #1 lore raw

PATCH 1/1 arm64: fix relocation of movz instruction with negative immediate

arch/arm64/kernel/module.c static int reloc_insn_movw(enum aarch64_reloc_op op, void *place, u64 val,

Quoted 15 lines · Ard Biesheuvel

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 <[email protected]>
---
 arch/arm64/kernel/module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/kernel/module.c +0 -0
index f4bc779e62e8..39e4a29cab50 100644
@@ -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) {

/* ... */

arch/arm64/kernel/module.c static int reloc_insn_movw(enum aarch64_reloc_op op, void *place, u64 val, · line 134

Quoted 8 lines

arch/arm64/kernel/module.c +1 -1

 		 * 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

Ard Biesheuvel

author

2016-01-04 17:33 UTC #2 lore raw

PATCH 1/1 arm64: fix relocation of movz instruction with negative immediate

Quoted 51 lines · Dave Martin

arch/arm64/kernel/module.c static int reloc_insn_movw(enum aarch64_reloc_op op, void *place, u64 val,
Quoted 15 lines · Ard Biesheuvel
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 <[email protected]>
---
 arch/arm64/kernel/module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/kernel/module.c +0 -0
index f4bc779e62e8..39e4a29cab50 100644
@@ -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) {
/* ... */
arch/arm64/kernel/module.c static int reloc_insn_movw(enum aarch64_reloc_op op, void *place, u64 val, · line 134
Quoted 8 lines
arch/arm64/kernel/module.c +1 -1
               * 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?

Quoted 8 lines

        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 ..

Dave Martin

2016-01-04 18:00 UTC #3 lore raw

PATCH 1/1 arm64: fix relocation of movz instruction with negative immediate

Quoted 58 lines · Ard Biesheuvel

Quoted 57 lines · Dave Martin
arch/arm64/kernel/module.c static int reloc_insn_movw(enum aarch64_reloc_op op, void *place, u64 val,
Quoted 15 lines · Ard Biesheuvel
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 <[email protected]>
---
 arch/arm64/kernel/module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/kernel/module.c +0 -0
index f4bc779e62e8..39e4a29cab50 100644
@@ -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) {
/* ... */
arch/arm64/kernel/module.c static int reloc_insn_movw(enum aarch64_reloc_op op, void *place, u64 val, · line 134
Quoted 8 lines
arch/arm64/kernel/module.c +1 -1
               * 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++;
        }

Quoted 8 lines

Quoted 2 lines
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

Will Deacon

2016-01-04 17:48 UTC #4 lore raw

PATCH 1/1 arm64: fix relocation of movz instruction with negative immediate

In reply to Dave Martin

Quoted 26 lines · Dave Martin

arch/arm64/kernel/module.c static int reloc_insn_movw(enum aarch64_reloc_op op, void *place, u64 val,
Quoted 15 lines · Ard Biesheuvel
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 <[email protected]>
---
 arch/arm64/kernel/module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/kernel/module.c +0 -0
index f4bc779e62e8..39e4a29cab50 100644
@@ -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) {
/* ... */

Quoted 10 lines

arch/arm64/kernel/module.c static int reloc_insn_movw(enum aarch64_reloc_op op, void *place, u64 val, · line 134
Quoted 8 lines
arch/arm64/kernel/module.c +1 -1
 		 * 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.

Quoted 11 lines

		}
		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.

Quoted 7 lines

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.

Quoted 1 line

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

arch/arm64/kernel/module.c +1 -2

index 266e7490e85c..6546032bb83b 100644
@@ -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. */