arm64: Fix off-by-one vdso trampoline return value

PATCH In review Will McVicker 3 participants 7 messages Nov 12, 2020 – Nov 16, 2020 arm64

Share & tools

Will McVicker

author

2020-11-12 00:15 UTC #0 lore raw

PATCH 1/1 arm64: Fix off-by-one vdso trampoline return value

Depending on your host nm version, the generated header include/generated/vdso32-offsets.h may have the bottom bit set for the thumb vdso offset addresses (as observed when using llvm-nm). This results in an additional +1 for thumb vdso trampoline return values since compat_setup_return() already includes vdso_trampoline + thumb. As a result, I see a SIGBUS error when running the LTP test syscalls.rt_sigaction01. To fix this, let's clear the bottom bit of the vdso_offset in the VDSO_SYMBOL macro.

Test: LTP test syscalls.rt_sigaction01

Fixes f01703b3d2e6 ("arm64: compat: Get sigreturn trampolines from vDSO")

Signed-off-by Will McVicker <[email protected]>

---
 arch/arm64/include/asm/vdso.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/include/asm/vdso.h +1 -1

index f99dcb94b438..a7384379e8e1 100644
@@ -23,7 +23,7 @@
 
 #define VDSO_SYMBOL(base, name)						   \
 ({									   \
-	(void *)(vdso_offset_##name - VDSO_LBASE + (unsigned long)(base)); \
+	(void *)((vdso_offset_##name & ~1UL) - VDSO_LBASE + (unsigned long)(base)); \
 })
 
 #endif /* !__ASSEMBLY__ */

Signature · 7 lines

2.29.2.299.gdc1121823c-goog


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Nick Desaulniers

2020-11-12 01:01 UTC #1 lore raw

Quoted 4 lines · Will McVicker

Depending on your host nm version, the generated header include/generated/vdso32-offsets.h may have the bottom bit set for the thumb vdso offset addresses (as observed when using llvm-nm). This

Sorry, the commit message seems to imply a bug in llvm-nm, but I don't think that's the case. If it is, please, send us a bugreport.

$ aarch64-linux-gnu-nm arch/arm64/kernel/vdso32/vdso.so.raw | grep thumb
00000968 T __kernel_rt_sigreturn_thumb
00000960 T __kernel_sigreturn_thumb
00000968 t VDSO_compat_rt_sigreturn_thumb
00000960 t VDSO_compat_sigreturn_thumb
$ llvm-nm arch/arm64/kernel/vdso32/vdso.so.raw | grep thumb
00000968 t VDSO_compat_rt_sigreturn_thumb
00000960 t VDSO_compat_sigreturn_thumb
00000968 T __kernel_rt_sigreturn_thumb
00000960 T __kernel_sigreturn_thumb
$ /usr/bin/nm arch/arm64/kernel/vdso32/vdso.so.raw | grep thumb
00000969 T __kernel_rt_sigreturn_thumb
00000961 T __kernel_sigreturn_thumb
00000969 t VDSO_compat_rt_sigreturn_thumb
00000961 t VDSO_compat_sigreturn_thumb
$ /usr/bin/nm --version
GNU nm (GNU Binutils for Debian) 2.35.1

Would you mind amending the commit message to not imply that llvm-nm is broken?

It might be of interest to find out why the host nm was invoked, rather than $(NM)/$(CROSS_COMPILE)nm.

Quoted 29 lines

results in an additional +1 for thumb vdso trampoline return values since compat_setup_return() already includes vdso_trampoline + thumb. As a result, I see a SIGBUS error when running the LTP test syscalls.rt_sigaction01. To fix this, let's clear the bottom bit of the vdso_offset in the VDSO_SYMBOL macro.
Test: LTP test syscalls.rt_sigaction01
Fixes f01703b3d2e6 ("arm64: compat: Get sigreturn trampolines from vDSO")
Signed-off-by Will McVicker <[email protected]>
---
 arch/arm64/include/asm/vdso.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/include/asm/vdso.h +1 -2
index f99dcb94b438..a7384379e8e1 100644
@@ -23,7 +23,7 @@
 
 #define VDSO_SYMBOL(base, name)                                                   \
 ({                                                                        \
-       (void *)(vdso_offset_##name - VDSO_LBASE + (unsigned long)(base)); \
+       (void *)((vdso_offset_##name & ~1UL) - VDSO_LBASE + (unsigned long)(base)); \
 })
 
 #endif /* !__ASSEMBLY__ */
--
2.29.2.299.gdc1121823c-goog

Signature · 7 lines

Thanks,
~Nick Desaulniers

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Nick Desaulniers

2020-11-12 01:08 UTC #2 lore raw

Quoted 19 lines · Nick Desaulniers

Quoted 4 lines · Will McVicker
Depending on your host nm version, the generated header include/generated/vdso32-offsets.h may have the bottom bit set for the thumb vdso offset addresses (as observed when using llvm-nm). This
Sorry, the commit message seems to imply a bug in llvm-nm, but I don't think that's the case. If it is, please, send us a bugreport.
$ aarch64-linux-gnu-nm arch/arm64/kernel/vdso32/vdso.so.raw | grep thumb
00000968 T __kernel_rt_sigreturn_thumb
00000960 T __kernel_sigreturn_thumb
00000968 t VDSO_compat_rt_sigreturn_thumb
00000960 t VDSO_compat_sigreturn_thumb
$ llvm-nm arch/arm64/kernel/vdso32/vdso.so.raw | grep thumb
00000968 t VDSO_compat_rt_sigreturn_thumb
00000960 t VDSO_compat_sigreturn_thumb
00000968 T __kernel_rt_sigreturn_thumb

Quoted 8 lines

00000960 T __kernel_sigreturn_thumb
$ /usr/bin/nm arch/arm64/kernel/vdso32/vdso.so.raw | grep thumb
00000969 T __kernel_rt_sigreturn_thumb
00000961 T __kernel_sigreturn_thumb
00000969 t VDSO_compat_rt_sigreturn_thumb
00000961 t VDSO_compat_sigreturn_thumb
$ /usr/bin/nm --version
GNU nm (GNU Binutils for Debian) 2.35.1

(Noting that my host's GNU binutils are configured to target x86):

$ /usr/bin/nm -h
...
elf64-x86-64 elf32-i386 elf32-iamcu elf32-x86-64 pei-i386 pei-x86-64
elf64-l1om elf64-k1om elf64-little elf64-big elf32-little elf32-big
pe-x86-64 pe-bigobj-x86-64 pe-i386 srec symbolsrec verilog tekhex
binary ihex plugin

So it would seem when binutils is configured for x86, then it will mistakenly decode thumb instructions as being off by one.

(Note to no one in particular: verilog? really?)

Quoted 40 lines

Would you mind amending the commit message to not imply that llvm-nm is broken?
It might be of interest to find out why the host nm was invoked, rather than $(NM)/$(CROSS_COMPILE)nm.
Quoted 27 lines
results in an additional +1 for thumb vdso trampoline return values since compat_setup_return() already includes vdso_trampoline + thumb. As a result, I see a SIGBUS error when running the LTP test syscalls.rt_sigaction01. To fix this, let's clear the bottom bit of the vdso_offset in the VDSO_SYMBOL macro.
Test: LTP test syscalls.rt_sigaction01
Fixes f01703b3d2e6 ("arm64: compat: Get sigreturn trampolines from vDSO")
Signed-off-by Will McVicker <[email protected]>
---
 arch/arm64/include/asm/vdso.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/include/asm/vdso.h +1 -2
index f99dcb94b438..a7384379e8e1 100644
@@ -23,7 +23,7 @@
 
 #define VDSO_SYMBOL(base, name)                                                   \
 ({                                                                        \
-       (void *)(vdso_offset_##name - VDSO_LBASE + (unsigned long)(base)); \
+       (void *)((vdso_offset_##name & ~1UL) - VDSO_LBASE + (unsigned long)(base)); \
 })
 
 #endif /* !__ASSEMBLY__ */
--
Quoted 2 lines
2.29.2.299.gdc1121823c-goog
--
Thanks,
~Nick Desaulniers

Signature · 7 lines

Thanks,
~Nick Desaulniers

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Nick Desaulniers

2020-11-12 02:16 UTC #3 lore raw

In reply to Nick Desaulniers

Quoted 28 lines · Nick Desaulniers

Quoted 4 lines · Will McVicker
Depending on your host nm version, the generated header include/generated/vdso32-offsets.h may have the bottom bit set for the thumb vdso offset addresses (as observed when using llvm-nm). This
Sorry, the commit message seems to imply a bug in llvm-nm, but I don't think that's the case. If it is, please, send us a bugreport.
$ aarch64-linux-gnu-nm arch/arm64/kernel/vdso32/vdso.so.raw | grep thumb
00000968 T __kernel_rt_sigreturn_thumb
00000960 T __kernel_sigreturn_thumb
00000968 t VDSO_compat_rt_sigreturn_thumb
00000960 t VDSO_compat_sigreturn_thumb
$ llvm-nm arch/arm64/kernel/vdso32/vdso.so.raw | grep thumb
00000968 t VDSO_compat_rt_sigreturn_thumb
00000960 t VDSO_compat_sigreturn_thumb
00000968 T __kernel_rt_sigreturn_thumb
00000960 T __kernel_sigreturn_thumb
$ /usr/bin/nm arch/arm64/kernel/vdso32/vdso.so.raw | grep thumb
00000969 T __kernel_rt_sigreturn_thumb
00000961 T __kernel_sigreturn_thumb
00000969 t VDSO_compat_rt_sigreturn_thumb
00000961 t VDSO_compat_sigreturn_thumb
$ /usr/bin/nm --version
GNU nm (GNU Binutils for Debian) 2.35.1

Quoted 1 line

Would you mind amending the commit message to not imply that llvm-nm is broken?

Testing another set of configs:

$ aarch64-linux-android-nm arch/arm64/kernel/vdso32/vdso.so.dbg | grep thumb
00000950 T __kernel_rt_sigreturn_thumb
00000948 T __kernel_sigreturn_thumb
00000951 t VDSO_compat_rt_sigreturn_thumb
00000949 t VDSO_compat_sigreturn_thumb
$ /path/to/older/aarch64-linux-gnu-nm
arch/arm64/kernel/vdso32/vdso.so.dbg | grep thumb
00000950 T __kernel_rt_sigreturn_thumb
00000948 T __kernel_sigreturn_thumb
00000951 t VDSO_compat_rt_sigreturn_thumb
00000949 t VDSO_compat_sigreturn_thumb
$ /usr/bin/nm out/android-4.19-stable/common/arch/arm64/kernel/vdso32/vdso.so.dbg
| grep thumb
00000951 T __kernel_rt_sigreturn_thumb
00000949 T __kernel_sigreturn_thumb
00000951 t VDSO_compat_rt_sigreturn_thumb
00000949 t VDSO_compat_sigreturn_thumb
$ llvm-nm out/android-4.19-stable/common/arch/arm64/kernel/vdso32/vdso.so.dbg
| grep thumb
00000951 t VDSO_compat_rt_sigreturn_thumb
00000949 t VDSO_compat_sigreturn_thumb
00000950 T __kernel_rt_sigreturn_thumb
00000948 T __kernel_sigreturn_thumb

(That llvm-nm sorts the output makes this trickier to follow). But shows that only host GNU nm differs.

Quoted 3 lines

It might be of interest to find out why the host nm was invoked, rather than $(NM)/$(CROSS_COMPILE)nm.

Possibly commit 7b7891c7bdfd ("arm64: vdso32: Fix '--prefix=' value for newer versions of clang") missing from your tree, but I fail to see how that would mess up or invoke the incorrect $(NM).

Quoted 35 lines

Quoted 28 lines
results in an additional +1 for thumb vdso trampoline return values since compat_setup_return() already includes vdso_trampoline + thumb. As a result, I see a SIGBUS error when running the LTP test syscalls.rt_sigaction01. To fix this, let's clear the bottom bit of the vdso_offset in the VDSO_SYMBOL macro.
Test: LTP test syscalls.rt_sigaction01
Fixes f01703b3d2e6 ("arm64: compat: Get sigreturn trampolines from vDSO")
Signed-off-by Will McVicker <[email protected]>
---
 arch/arm64/include/asm/vdso.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/include/asm/vdso.h +1 -2
index f99dcb94b438..a7384379e8e1 100644
@@ -23,7 +23,7 @@
 
 #define VDSO_SYMBOL(base, name)                                                   \
 ({                                                                        \
-       (void *)(vdso_offset_##name - VDSO_LBASE + (unsigned long)(base)); \
+       (void *)((vdso_offset_##name & ~1UL) - VDSO_LBASE + (unsigned long)(base)); \
 })
 
 #endif /* !__ASSEMBLY__ */
--
Quoted 2 lines
2.29.2.299.gdc1121823c-goog
--
Thanks,
~Nick Desaulniers

Signature · 7 lines

Thanks,
~Nick Desaulniers

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Will Deacon

maintainer

2020-11-12 10:12 UTC #4 lore raw

In reply to Will McVicker

Quoted 21 lines · Will McVicker

Depending on your host nm version, the generated header include/generated/vdso32-offsets.h may have the bottom bit set for the thumb vdso offset addresses (as observed when using llvm-nm). This results in an additional +1 for thumb vdso trampoline return values since compat_setup_return() already includes vdso_trampoline + thumb. As a result, I see a SIGBUS error when running the LTP test syscalls.rt_sigaction01. To fix this, let's clear the bottom bit of the vdso_offset in the VDSO_SYMBOL macro.
Test: LTP test syscalls.rt_sigaction01
Fixes f01703b3d2e6 ("arm64: compat: Get sigreturn trampolines from vDSO")
Signed-off-by Will McVicker <[email protected]>
---
 arch/arm64/include/asm/vdso.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/include/asm/vdso.h +0 -0
index f99dcb94b438..a7384379e8e1 100644

arch/arm64/include/asm/vdso.h line 26

Quoted 5 lines

arch/arm64/include/asm/vdso.h +1 -1

@@ -23,7 +23,7 @@
 
 #define VDSO_SYMBOL(base, name)						   \
 ({									   \
-	(void *)(vdso_offset_##name - VDSO_LBASE + (unsigned long)(base)); \
+	(void *)((vdso_offset_##name & ~1UL) - VDSO_LBASE + (unsigned long)(base)); \

I don't think we need this in mainline, because the sigreturn trampoline is just a bunch of .byte directives and I removed the sigreturn code from the compat vdso in 2d071968a405 ("arm64: compat: Remove 32-bit sigreturn code from the vDSO").

Might be needed in some stable kernels though (or we just backport the patch I mentioned above)

Will

William Mcvicker

author

2020-11-12 18:51 UTC #5 lore raw

Hi Nick,

Regarding llvm-nm, this extra thumb +1 is noticed after porting
https://lore.kernel.org/linux-arm-kernel/[email protected]/
to the Android Common Kernel android-4.19-stable. I'm not sure why using ld.lld causes this difference, but this proposed patch ensures that we don't rely on the nm tool used.

Will D.,
Regarding applying this to some stable kernels vs backporting 2d071968a405 ("arm64: compat: Remove 32-bit sigreturn code from the vDSO"), I am hesitant to backport commit 2d071968a405 due it's dependencies. For 4.19 at least, I would also need to backport these:

8e411be6aad13 [email protected] arm64: compat: Always use sigpage for sigreturn trampoline a39060b009ca0 [email protected] arm64: compat: Allow 32-bit vdso and sigpage to co-exist

1d09094aa6205 [email protected]  arm64: vdso: use consistent 'map' nomenclature
d3418f3839b66 [email protected]  arm64: vdso: use consistent 'abi' nomenclature
3ee16ff3437ca [email protected]  arm64: vdso: simplify arch_vdso_type ifdeffery
74fc72e77dc5c [email protected]  arm64: vdso: remove aarch32_vdso_pages[]

I have done this in my local tree and verified it fixes the SIGBUS error I'm seeing; however, it seems a lot cleaner and safer to just patch the VDSO_SYMBOL macro. Please let me know what route you prefer. I'm happy to backport all of these if that's the recommended approach.

Thanks,
Will

Quoted 37 lines · Will Deacon

Quoted 21 lines · Will McVicker
Depending on your host nm version, the generated header include/generated/vdso32-offsets.h may have the bottom bit set for the thumb vdso offset addresses (as observed when using llvm-nm). This results in an additional +1 for thumb vdso trampoline return values since compat_setup_return() already includes vdso_trampoline + thumb. As a result, I see a SIGBUS error when running the LTP test syscalls.rt_sigaction01. To fix this, let's clear the bottom bit of the vdso_offset in the VDSO_SYMBOL macro.
Test: LTP test syscalls.rt_sigaction01
Fixes f01703b3d2e6 ("arm64: compat: Get sigreturn trampolines from vDSO")
Signed-off-by Will McVicker <[email protected]>
---
 arch/arm64/include/asm/vdso.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/include/asm/vdso.h +0 -0
index f99dcb94b438..a7384379e8e1 100644
arch/arm64/include/asm/vdso.h line 26
Quoted 5 lines
arch/arm64/include/asm/vdso.h +1 -1
@@ -23,7 +23,7 @@
 
 #define VDSO_SYMBOL(base, name)						   \
 ({									   \
-	(void *)(vdso_offset_##name - VDSO_LBASE + (unsigned long)(base)); \
+	(void *)((vdso_offset_##name & ~1UL) - VDSO_LBASE + (unsigned long)(base)); \
I don't think we need this in mainline, because the sigreturn trampoline is just a bunch of .byte directives and I removed the sigreturn code from the compat vdso in 2d071968a405 ("arm64: compat: Remove 32-bit sigreturn code from the vDSO").
Might be needed in some stable kernels though (or we just backport the patch I mentioned above)
Will

William Mcvicker

author

2020-11-16 22:56 UTC #6 lore raw

Hi All,

After digging into this even deeper with Nick, we found that the underlying issue is with ld.lld not carrying over the st_type for the VDSO_compat* symbol assignments in arch/arm64/kernel/vdso32/vdso.lds.S.

From my Android Common Kernel 4.19 build:

$ llvm-readelf -s arch/arm64/kernel/vdso32/vdso.so.raw | grep thumb | sort
    26: 0000094d     0 NOTYPE  LOCAL  DEFAULT    11 VDSO_compat_sigreturn_thumb
    28: 00000955     0 NOTYPE  LOCAL  DEFAULT    11 VDSO_compat_rt_sigreturn_thumb
    37: 0000094d     4 FUNC    GLOBAL DEFAULT    11 __kernel_sigreturn_thumb
    38: 00000955     4 FUNC    GLOBAL DEFAULT    11 __kernel_rt_sigreturn_thumb
     8: 0000094d     4 FUNC    GLOBAL DEFAULT    11 __kernel_sigreturn_thumb@@LINUX_2.6
     9: 00000955     4 FUNC    GLOBAL DEFAULT    11 __kernel_rt_sigreturn_thumb@@LINUX_2.6

Fortunately, this has been fixed by llvm here: https://reviews.llvm.org/D86263.
So for kernels that use ld.lld and the VDSO compat sigreturn trampoline, they need to make sure to upgrade their toolchain.

I hope this thread helps anyone running into this issue in the future.

Thanks,
Will

Quoted 67 lines · William Mcvicker

Hi Nick,
Regarding llvm-nm, this extra thumb +1 is noticed after porting
https://lore.kernel.org/linux-arm-kernel/[email protected]/
to the Android Common Kernel android-4.19-stable. I'm not sure why using ld.lld causes this difference, but this proposed patch ensures that we don't rely on the nm tool used.
Will D.,
Regarding applying this to some stable kernels vs backporting 2d071968a405 ("arm64: compat: Remove 32-bit sigreturn code from the vDSO"), I am hesitant to backport commit 2d071968a405 due it's dependencies. For 4.19 at least, I would also need to backport these:
8e411be6aad13 [email protected] arm64: compat: Always use sigpage for sigreturn trampoline a39060b009ca0 [email protected] arm64: compat: Allow 32-bit vdso and sigpage to co-exist
1d09094aa6205 [email protected]  arm64: vdso: use consistent 'map' nomenclature
d3418f3839b66 [email protected]  arm64: vdso: use consistent 'abi' nomenclature
3ee16ff3437ca [email protected]  arm64: vdso: simplify arch_vdso_type ifdeffery
74fc72e77dc5c [email protected]  arm64: vdso: remove aarch32_vdso_pages[]
I have done this in my local tree and verified it fixes the SIGBUS error I'm seeing; however, it seems a lot cleaner and safer to just patch the VDSO_SYMBOL macro. Please let me know what route you prefer. I'm happy to backport all of these if that's the recommended approach.
Thanks,
Will
Quoted 37 lines · Will Deacon
Quoted 21 lines · Will McVicker
Depending on your host nm version, the generated header include/generated/vdso32-offsets.h may have the bottom bit set for the thumb vdso offset addresses (as observed when using llvm-nm). This results in an additional +1 for thumb vdso trampoline return values since compat_setup_return() already includes vdso_trampoline + thumb. As a result, I see a SIGBUS error when running the LTP test syscalls.rt_sigaction01. To fix this, let's clear the bottom bit of the vdso_offset in the VDSO_SYMBOL macro.
Test: LTP test syscalls.rt_sigaction01
Fixes f01703b3d2e6 ("arm64: compat: Get sigreturn trampolines from vDSO")
Signed-off-by Will McVicker <[email protected]>
---
 arch/arm64/include/asm/vdso.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/include/asm/vdso.h +0 -0
index f99dcb94b438..a7384379e8e1 100644
arch/arm64/include/asm/vdso.h line 26
Quoted 5 lines
arch/arm64/include/asm/vdso.h +1 -1
@@ -23,7 +23,7 @@
 
 #define VDSO_SYMBOL(base, name)						   \
 ({									   \
-	(void *)(vdso_offset_##name - VDSO_LBASE + (unsigned long)(base)); \
+	(void *)((vdso_offset_##name & ~1UL) - VDSO_LBASE + (unsigned long)(base)); \
I don't think we need this in mainline, because the sigreturn trampoline is just a bunch of .byte directives and I removed the sigreturn code from the compat vdso in 2d071968a405 ("arm64: compat: Remove 32-bit sigreturn code from the vDSO").
Might be needed in some stable kernels though (or we just backport the patch I mentioned above)
Will