linux-kernel - Re: [PATCH] arm64: Fix off-by-one vdso trampoline return value

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201112185136.GA585063@google.com>
Date:   Thu, 12 Nov 2020 18:51:36 +0000
From:   William Mcvicker <willmcvicker@...gle.com>
To:     Will Deacon <will@...nel.org>
Cc:     Catalin Marinas <catalin.marinas@....com>,
        Nathan Chancellor <natechancellor@...il.com>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Vincenzo Frascino <vincenzo.frascino@....com>,
        Andrei Vagin <avagin@...il.com>,
        Dmitry Safonov <0x7f454c46@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        clang-built-linux@...glegroups.com, kernel-team@...roid.com
Subject: Re: [PATCH] arm64: Fix off-by-one vdso trampoline return value

Hi Nick,

Regarding llvm-nm, this extra thumb +1 is noticed after porting
https://lore.kernel.org/linux-arm-kernel/20201013033947.2257501-1-natechancellor@gmail.com/
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 will@...nel.org  arm64: compat: Always use sigpage for sigreturn trampoline
a39060b009ca0 will@...nel.org  arm64: compat: Allow 32-bit vdso and sigpage to co-exist
1d09094aa6205 mark.rutland@....com  arm64: vdso: use consistent 'map' nomenclature
d3418f3839b66 mark.rutland@....com  arm64: vdso: use consistent 'abi' nomenclature
3ee16ff3437ca mark.rutland@....com  arm64: vdso: simplify arch_vdso_type ifdeffery
74fc72e77dc5c mark.rutland@....com  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

On 11/12/2020, Will Deacon wrote:
> On Thu, Nov 12, 2020 at 12:14:22AM +0000, Will McVicker wrote:
> > 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 <willmcvicker@...gle.com>
> > ---
> >  arch/arm64/include/asm/vdso.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/include/asm/vdso.h b/arch/arm64/include/asm/vdso.h
> > index f99dcb94b438..a7384379e8e1 100644
> > --- a/arch/arm64/include/asm/vdso.h
> > +++ b/arch/arm64/include/asm/vdso.h
> > @@ -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