[ELF] - Change value checks to checkUInt for R_386_8/R_386_PC8/R_386_16/R_386_PC16 relocations.

[grimar]

Summary

Recent changes which introduced checks broked linkage of linux kernel in part of
applying these relocations:

arch/x86/realmode/rm/wakeup_asm.S:135: relocation R_386_PC16 out of range

I checked that at the point of call:

checkInt<16>(Loc, Val, Type);

Val was 0x000000000000fda3
so looks checkUInt should be used here.

Patch updates testcases and checks. BFD likes and accepts updated inputs FWIW.

[ruiu]

Can you describe why this is correct? Relaxing the error checks until the kernel links is not a good idea.

I believe you should use checkUInt for R_386_{8,16} and checkInt for R_386_PC{8,16}.

[davide]

Sorry to say that, but this doesn't seem quite right to me. We should really understand why that check fails and not silencing it.
It could be anything from a linker bug (including, the broken check itself) from the assembler emitting a broken object (BTW, which assembler are you using?)

[grimar]

In D29392#663412, @ruiu wrote:

Can you describe why this is correct? Relaxing the error checks until the kernel links is not a good idea.

I believe you should use checkUInt for R_386_{8,16} and checkInt for R_386_PC{8,16}.

Reproduce I have fails on R_386_PC16 relocation with value 0x000000000000fda3.

In code that place looks like:
(https://github.com/torvalds/linux/blob/master/arch/x86/realmode/rm/wakeup_asm.S#L135)

jmp	trampoline_start

disasm of LLD linked is:

125a:	e9 a3 fd f4 eb       	jmp    ebf51002 <intcall+0xebf4bdaa>

BFD linked:

125a:	e9 a3 fd f4 eb       	jmp    ebf51002 <end_signature+0xebf4bdae>

So as you see output is equal.
But BFD accepts the input, LLD - not. That unfortunately all usefull info I have now.

I switched to minimal kernel configuration yesterday and it looks does not use this object anymore (it does not generate it at all).
So we can return to that question a bit later, when I'll be able to check that place separatelly.

(I really doubt there is something wrong with that place. All I can do is try to change it and see if that is breaks anything, for my eye now
it looks just like a regular jump).

In D29392#663429, @davide wrote:

Sorry to say that, but this doesn't seem quite right to me. We should really understand why that check fails and not silencing it.
It could be anything from a linker bug (including, the broken check itself) from the assembler emitting a broken object (BTW, which assembler are you using?)

Hard to say what is right here, ABI says "The R_386_16, and R_386_8 relocations truncate the computed value to 16-bits and 8-bits respectively.",
doing any checks is not what corresponds the ABI and gnu linkers have different behavior here.

I will try to check the place above and I just guess if we want do checks and don't want to follow ABI,
we probably will want to be compatible with BFD. Will see.

I use:
as -V
GNU assembler version 2.27 (x86_64-linux-gnu) using BFD version (GNU Binutils for Ubuntu) 2.27
for working on kernel linkage.

[grimar]

So, I found that my minimal kernel configuration is also affected by that issue some point.

I am observing R_386_PC8 relocation which is fails with new check because has value 358 == 0x166.

BFD output for that place (so it is just truncated to 8 bits and BFD does not complain about):

00000200 <_start>:
 200:	eb 66                	jmp    268 <start_of_setup>

If I comment out that check, we produce the same.

The code that produces this relocation is:
(http://lxr.free-electrons.com/source/arch/x86/boot/header.S#L292)

.globl	_start
_start:
		# Explicitly enter this as bytes, or the assembler
		# tries to generate a 3-byte jump here, which causes
		# everything else to push off to the wrong offset.
		.byte	0xeb		# short (2-byte) jump
		.byte	start_of_setup-1f

I'll investigate that place deeper tomorrow.

[ruiu]

As I wrote, don't you need to use checkUInt for R_386_{8,16} and checkInt for R_386_PC{8,16}, do you?

[grimar]

In D29392#664879, @ruiu wrote:

As I wrote, don't you need to use checkUInt for R_386_{8,16} and checkInt for R_386_PC{8,16}, do you?

As I also wrote, "Reproduce I have fails on R_386_PC16 relocation with value 0x000000000000fda3.", so checkInt already does not work for this relocation.

And I also metioned "I am observing R_386_PC8 relocation which is fails with new check because has value 358 == 0x166." so both types of checks will not work here in current code, no ?

[ruiu]

I'm sorry I didn't understand that.

So, back to the example. You have this expression in assembly. This expression clearly needs overflow checking in some form for its operand because if it overflows (i.e. the jump target is too far), this jump instruction jumps to a wrong address. Is this OK?

jmp trampoline_start

[grimar]

In D29392#665226, @ruiu wrote:

I'm sorry I didn't understand that.

So, back to the example. You have this expression in assembly. This expression clearly needs overflow checking in some form for its operand because if it overflows (i.e. the jump target is too far), this jump instruction jumps to a wrong address. Is this OK?

jmp trampoline_start

I think issue here is the same as D29490. Just need to signextend the addend. Output is correct:

125a:	e9 a3 fd f4 eb       	jmp    ebf51002 <intcall+0xebf4bdaa>

Above is 3 bytes relative jump (disasm just reads it as 5 bytes, but we know it is 3 bytes here), so it is:
JMP rel16, where rel16 = 0xFDA3 = -605.

0x125a - 605 + 3 bytes = 0xFFD + 3 = 0x1000
And at this adress I see:

00001000 <pa_trampoline_start>:
    1000:	fa                   	cli

So we produce correct output, but error out because of wrong addend,
I'll prepare a patch for addend, the same like for PC8 (D29490).

[grimar]

Its incorrect. D29490 and D29493 were posted instead.

ELF/Target.cpp

 void X86TargetInfo::relocateOne(uint8_t *Loc, uint32_t Type,
                                 uint64_t Val) const {
   // R_386_{PC,}{8,16} are not part of the i386 psABI, but they are
   // being used for some 16-bit programs such as boot loaders, so
   // we want to support them.
   switch (Type) {
   case R_386_8:
   case R_386_PC8:
-    checkInt<8>(Loc, Val, Type);
+    checkUInt<8>(Loc, Val, Type);
     *Loc = Val;
     break;
   case R_386_16:
   case R_386_PC16:
-    checkInt<16>(Loc, Val, Type);
+    checkUInt<16>(Loc, Val, Type);
     write16le(Loc, Val);
     break;
   default:
     checkInt<32>(Loc, Val, Type);
     write32le(Loc, Val);
   }
 }
 

test/ELF/i386-pc16.test

 # REQUIRES: x86
 
 # RUN: yaml2obj %s -o %t.o
 # RUN: ld.lld -Ttext 0x0 %t.o -o %t.exe
 # RUN: llvm-objdump -s -section=.text %t.exe 2>&1 | FileCheck %s
 
 # CHECK:      Contents of section .text:
-# CHECK-NEXT:  0000 45231111 41231111
+# CHECK-NEXT:  0000 11ff1111 0dff1111
 
 !ELF
 FileHeader:
   Class:           ELFCLASS32
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_386
 Sections:
Relocations:
       - Offset:          4
         Symbol:          _start
         Type:            R_386_PC16
 Symbols:
   Global:
     - Name:     _start
       Type:     STT_FUNC
       Section:  .text
-      Value:    0x1234
+      Value:    0xEE00
       Size:     4

test/ELF/i386-pc8.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=i386-pc-linux-gnu %s -o %t1.o
 # RUN: llvm-mc -filetype=obj -triple=i386-pc-linux-gnu %S/Inputs/i386-pc8.s -o %t2.o
 # RUN: ld.lld -Ttext 0x0 %t1.o %t2.o -o %t.out
 # RUN: llvm-objdump -s -section=.text %t.out | FileCheck %s
 
 # CHECK:      Contents of section .text:
-# CHECK-NEXT:  0000 15253748
+# CHECK-NEXT:  0000 f5f5f7f8
 
-.byte und-.+0x11
-.byte und-.+0x22
-.byte und+0x33
-.byte und+0x44
+.byte und-.+0xF1
+.byte und-.+0xF2
+.byte und+0xF3
+.byte und+0xF4