Skip to content

Commit

Permalink
arm64 replace call code use X27 register instead of X17 to avoid crash
Browse files Browse the repository at this point in the history
  • Loading branch information
@pkujhd
pkujhd committed Jul 1, 2023
1 parent 58bb448 commit dd5dc6f
Showing 1 changed file with 3 additions and 6 deletions.
9 changes: 3 additions & 6 deletions asm_bytes.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,10 @@ const (
// arm/arm64
var (
armReplaceCallCode = []byte{0x04, 0xF0, 0x1F, 0xE5} //LDR PC, [PC, #-4]
// X16 and X17 are the IP0 and IP1 intra-procedure-call corruptible registers -
// since Go only uses them for the stack prologue and epilogue calculations,
// and we should already be clear of that by the time we hit a R_CALLARM64,
// so we should be able to safely use them for far jumps
// register X27 reserved for liblink. see:^src/cmd/objfile/obj/arm64/a.out.go

This comment has been minimized.

Sign in to view

Copy link
@eh-steve

eh-steve Jul 2, 2023 •
edited
Loading

@pkujhd Have you actually observed a crash caused by use of X17 here?

This comment has been minimized.

Sign in to view

Copy link
@eh-steve

eh-steve Jul 2, 2023 •
edited
Loading

(It should only be used by external linker, stack prologues and duffcopy (<1.13), whereas there seem to be a lot more things that use X27)

This comment has been minimized.

Sign in to view

Copy link
@pkujhd

pkujhd Jul 3, 2023 •
edited
Loading

Author Owner

@pkujhd Have you actually observed a crash caused by use of X17 here?

on golang 1.8 ~ 1.12, the testcase base.go panic on call fmt.Println
jhd@ubuntu:~/go/goloader$ ./data/bin/1.12/loader -o ./data/objs/1.12/base.o

base init
{1 2}
{1 2} &{1 2} {1 0} {0 0}
base.Vertex{X:1, Y:2} &base.Vertex{X:1, Y:2} base.Vertex{X:1, Y:0} base.Vertex{X:0, Y:0}
print &{1 2}
unexpected fault address 0x458ca8
fatal error: fault
[signal SIGSEGV: segmentation violation code=0x2 addr=0x458ca8 pc=0x458cac]

goroutine 1 [running]:
runtime.throw(0x759ba0, 0x5)
        /home/jhd/programs/go.1.12/src/runtime/panic.go:617 +0x54 fp=0x4000201580 sp=0x4000201550 pc=0x42cd0c
runtime.sigpanic()
        /home/jhd/programs/go.1.12/src/runtime/signal_unix.go:397 +0x438 fp=0x40002015b0 sp=0x4000201580 pc=0x442090
runtime.duffcopy()
        /home/jhd/programs/go.1.12/src/runtime/duff_arm64.s:256 +0x1e4 fp=0x40002015c0 sp=0x40002015c0 pc=0x458cac
main.main()
        /home/jhd/go/src/github.com/pkujhd/goloader/examples/base/base.go:47 +0x38c fp=0x4000201850 sp=0x40002015c0 pc=0xffff987afe24
main.main()
        /home/jhd/go/src/github.com/pkujhd/goloader/examples/loader/loader.go:92 +0x708 fp=0x4000201f80 sp=0x4000201850 pc=0x6994e0
runtime.main()
        /home/jhd/programs/go.1.12/src/runtime/proc.go:200 +0x23c fp=0x4000201fd0 sp=0x4000201f80 pc=0x42e5f4
runtime.goexit()
        /home/jhd/programs/go.1.12/src/runtime/asm_arm64.s:1128 +0x4 fp=0x4000201fd0 sp=0x4000201fd0 pc=0x45886c
jhd@ubuntu:~/go/goloader$ uname -a
Linux ubuntu 4.15.0-76-generic #86-Ubuntu SMP Fri Jan 17 17:25:58 UTC 2020 aarch64 aarch64 aarch64 GNU/Linux

This comment has been minimized.

Sign in to view

Copy link
@pkujhd

pkujhd Jul 3, 2023

Author Owner

(It should only be used by external linker, stack prologues and duffcopy (<1.20), whereas there seem to be a lot more things that use X27)

X27 as a temporary register, I think the data stored in X27 is useless when the call instruction occurs, but I'm not sure

This comment has been minimized.

Sign in to view

Copy link
@eh-steve

eh-steve Jul 4, 2023 •
edited
Loading

Ah very interesting (I just saw this now)... I think the asm implementation of runtime·duffcopy switched to using X20/X21 in 1.13 for this exact reason (i.e. it's actually a bug in Go < 1.13, and X17 is supposed to be safe to use for linker generated trampolines, as that's what these scratch registers were intended for in the arm64 ABI).

Looking at the usages of arm64.REGTMP in ssaGenValue, I see it's used in a few atomic operations and lowered moves, but the main risk I see in asm7.go is that x27 will be clobbered during pre-emption so it's important that the amended pcvalue data marks this call-site and the extra segment of relocation epilogue as always unsafe. Currently it just uses the "unsafety" value at the relocation offset, which may or may not be -2 (PCDATA_UnsafePointUnsafe). I think you'll need to make this change if you continue using X27, or risk random faults from branches to random addresses if a pre-emption hits midway through this call.

Since my fork only supports >= 1.18 where Go's incorrect use of the IP0/IP1 registers in duffcopy is fixed, I won't bother making this change on my side.

This comment has been minimized.

Sign in to view

Copy link
@pkujhd

pkujhd Jul 5, 2023

Author Owner

yes, on golang >= 1.13, the bug is not exist. so you don't make this change on your fork.
arm64ReplaceCALLCode = []byte{
0x51, 0x00, 0x00, 0x58, // LDR X17 [PC+8] - read 64 bit address from PC+8 into X17
0x20, 0x02, 0x1f, 0xd6, // BR X17 - jump to address in X17
0x5B, 0x00, 0x00, 0x58, // LDR X27 [PC+8] - read 64 bit address from PC+8 into X27
0x60, 0x03, 0x1F, 0xD6, // BR X27 - jump to address in X27
}
arm64Bcode = []byte{0x00, 0x00, 0x00, 0x14} // B [PC+0x0]
arm64LDRcode = []byte{0x00, 0x00, 0x40, 0xF9} // LDR XX [XX]
Expand Down

0 comments on commit dd5dc6f

Please sign in to comment.