| 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
| ||
|
Message-ID: <CAAhV-H4QM6xHNfNcnbYX4M+PMuBcp7x9do97Ak9_XksFA9mM2Q@mail.gmail.com>
Date: Thu, 4 Jan 2024 16:58:21 +0800
From: Huacai Chen <chenhuacai@...nel.org>
To: Xi Ruoyao <xry111@...111.site>
Cc: WANG Xuerui <kernel@...0n.name>, Jiaxun Yang <jiaxun.yang@...goat.com>,
Eric Biederman <ebiederm@...ssion.com>, Kees Cook <keescook@...omium.org>,
Tiezhu Yang <yangtiezhu@...ngson.cn>, Jinyang He <hejinyang@...ngson.cn>,
loongarch@...ts.linux.dev, linux-mm@...ck.org, linux-kernel@...r.kernel.org,
stable@...r.kernel.org, linux-mips@...r.kernel.org
Subject: Re: [PATCH v3] LoongArch: Fix and simplify fcsr initialization on execve
Queued for loongarch-next, thanks.
Huacai
On Tue, Jan 2, 2024 at 8:38 PM Xi Ruoyao <xry111@...111.site> wrote:
>
> There has been a lingering bug in LoongArch Linux systems causing some
> GCC tests to intermittently fail (see Closes link). I've made a minimal
> reproducer:
>
> zsh% cat measure.s
> .align 4
> .globl _start
> _start:
> movfcsr2gr $a0, $fcsr0
> bstrpick.w $a0, $a0, 16, 16
> beqz $a0, .ok
> break 0
> .ok:
> li.w $a7, 93
> syscall 0
> zsh% cc mesaure.s -o measure -nostdlib
> zsh% echo $((1.0/3))
> 0.33333333333333331
> zsh% while ./measure; do ; done
>
> This while loop should not stop as POSIX is clear that execve must set
> fenv to the default, where FCSR should be zero. But in fact it will
> just stop after running for a while (normally less than 30 seconds).
> Note that "$((1.0/3))" is needed to reproduce the issue because it
> raises FE_INVALID and makes fcsr0 non-zero.
>
> The problem is we are relying on SET_PERSONALITY2 to reset
> current->thread.fpu.fcsr. But SET_PERSONALITY2 is executed before
> start_thread which calls lose_fpu(0). We can see if kernel preempt is
> enabled, we may switch to another thread after SET_PERSONALITY2 but
> before lose_fpu(0). Then bad thing happens: during the thread switch
> the value of the fcsr0 register is stored into current->thread.fpu.fcsr,
> making it dirty again.
>
> The issue can be fixed by setting current->thread.fpu.fcsr after
> lose_fpu(0) because lose_fpu clears TIF_USEDFPU, then the thread
> switch won't touch current->thread.fpu.fcsr.
>
> The only other architecture setting FCSR in SET_PERSONALITY2 is MIPS.
> I've ran a similar test on MIPS with mainline kernel and it turns out
> MIPS is buggy too. Anyway MIPS do this for supporting different FP
> flavors (NaN encodings etc.) which do not exist on LoongArch. So for
> LoongArch, we can simply remove the current->thread.fpu.fcsr setting
> from SET_PERSONALITY2 and do it in start_thread, after lose_fpu(0).
> I'll leave the job to fix MIPS for MIPS maintainers.
>
> The while loop failing with the mainline kernel has survived one hour
> after this change on LoongArch.
>
> Closes: https://github.com/loongson-community/discussions/issues/7
> Fixes: 803b0fc5c3f2 ("LoongArch: Add process management")
> Link: https://lore.kernel.org/linux-mips/7a6aa1bbdbbe2e63ae96ff163fab0349f58f1b9e.camel@xry111.site/
> Cc: stable@...r.kernel.org
> Cc: linux-mips@...r.kernel.org
> Signed-off-by: Xi Ruoyao <xry111@...111.site>
> ---
>
> v2 -> v3:
> - Update the commit message to mention MIPS is buggy too.
> - Replace tabs in the commit message with whitespaces.
> - No code change.
>
> v1 -> v2:
> - Still set current->thread.fpu.fcsr to boot_cpu_data.fpu_csr0 instead
> of constant 0.
>
> arch/loongarch/include/asm/elf.h | 5 -----
> arch/loongarch/kernel/elf.c | 5 -----
> arch/loongarch/kernel/process.c | 1 +
> 3 files changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/arch/loongarch/include/asm/elf.h b/arch/loongarch/include/asm/elf.h
> index 9b16a3b8e706..f16bd42456e4 100644
> --- a/arch/loongarch/include/asm/elf.h
> +++ b/arch/loongarch/include/asm/elf.h
> @@ -241,8 +241,6 @@ void loongarch_dump_regs64(u64 *uregs, const struct pt_regs *regs);
> do { \
> current->thread.vdso = &vdso_info; \
> \
> - loongarch_set_personality_fcsr(state); \
> - \
> if (personality(current->personality) != PER_LINUX) \
> set_personality(PER_LINUX); \
> } while (0)
> @@ -259,7 +257,6 @@ do { \
> clear_thread_flag(TIF_32BIT_ADDR); \
> \
> current->thread.vdso = &vdso_info; \
> - loongarch_set_personality_fcsr(state); \
> \
> p = personality(current->personality); \
> if (p != PER_LINUX32 && p != PER_LINUX) \
> @@ -340,6 +337,4 @@ extern int arch_elf_pt_proc(void *ehdr, void *phdr, struct file *elf,
> extern int arch_check_elf(void *ehdr, bool has_interpreter, void *interp_ehdr,
> struct arch_elf_state *state);
>
> -extern void loongarch_set_personality_fcsr(struct arch_elf_state *state);
> -
> #endif /* _ASM_ELF_H */
> diff --git a/arch/loongarch/kernel/elf.c b/arch/loongarch/kernel/elf.c
> index 183e94fc9c69..0fa81ced28dc 100644
> --- a/arch/loongarch/kernel/elf.c
> +++ b/arch/loongarch/kernel/elf.c
> @@ -23,8 +23,3 @@ int arch_check_elf(void *_ehdr, bool has_interpreter, void *_interp_ehdr,
> {
> return 0;
> }
> -
> -void loongarch_set_personality_fcsr(struct arch_elf_state *state)
> -{
> - current->thread.fpu.fcsr = boot_cpu_data.fpu_csr0;
> -}
> diff --git a/arch/loongarch/kernel/process.c b/arch/loongarch/kernel/process.c
> index 767d94cce0de..3f9cae615f52 100644
> --- a/arch/loongarch/kernel/process.c
> +++ b/arch/loongarch/kernel/process.c
> @@ -92,6 +92,7 @@ void start_thread(struct pt_regs *regs, unsigned long pc, unsigned long sp)
> clear_used_math();
> regs->csr_era = pc;
> regs->regs[3] = sp;
> + current->thread.fpu.fcsr = boot_cpu_data.fpu_csr0;
> }
>
> void flush_thread(void)
> --
> 2.43.0
>
Powered by blists - more mailing lists