linux-kernel - Re: [PATCH 1/4] x86/mce: Fix copyin code to return -EFAULT on machine check.

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20210406192446.GP17806@zn.tnic>
Date:   Tue, 6 Apr 2021 21:24:46 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Tony Luck <tony.luck@...el.com>
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Andy Lutomirski <luto@...nel.org>,
        Aili Yao <yaoaili@...gsoft.com>,
        HORIGUCHI NAOYA( 堀口　直也) 
        <naoya.horiguchi@....com>
Subject: Re: [PATCH 1/4] x86/mce: Fix copyin code to return -EFAULT on
 machine check.

On Thu, Mar 25, 2021 at 05:02:32PM -0700, Tony Luck wrote:
> When copy from user fails due to a machine check on poison reading
> user data it should return an error code.
> 
> ---
> 
> Separate patch just now, but likely needs to be combined with patches
> to iteration code for bisection safety.
> ---
>  arch/x86/lib/copy_user_64.S | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/lib/copy_user_64.S b/arch/x86/lib/copy_user_64.S
> index 77b9b2a3b5c8..2987118c541a 100644
> --- a/arch/x86/lib/copy_user_64.S
> +++ b/arch/x86/lib/copy_user_64.S
> @@ -14,6 +14,7 @@
>  #include <asm/alternative-asm.h>
>  #include <asm/asm.h>
>  #include <asm/smap.h>
> +#include <asm/errno.h>
>  #include <asm/export.h>
>  #include <asm/trapnr.h>
>  
> @@ -237,18 +238,21 @@ SYM_CODE_START_LOCAL(.Lcopy_user_handle_tail)
>  	cmp $X86_TRAP_MC,%eax		/* check if X86_TRAP_MC */
>  	je 3f
>  1:	rep movsb
> -2:	mov %ecx,%eax
> +	mov %ecx,%eax
> +	ASM_CLAC
> +	ret
> +
> +2:
> +	cmp $X86_TRAP_MC,%eax
> +	je 3f
> +	mov %ecx,%eax
>  	ASM_CLAC
>  	ret
>  
>  	/*
> -	 * Return zero to pretend that this copy succeeded. This
> -	 * is counter-intuitive, but needed to prevent the code
> -	 * in lib/iov_iter.c from retrying and running back into
> -	 * the poison cache line again. The machine check handler
> -	 * will ensure that a SIGBUS is sent to the task.
> +	 * Return -EFAULT for the machine check cases
>  	 */
> -3:	xorl %eax,%eax
> +3:	movl $-EFAULT,%eax
>  	ASM_CLAC
>  	ret

Right, looks ok after swapping all that
fault-handlers-rerouting-based-on-labels thing back in. You'd need to
update the blubber in the comment above it that we're returning -EFAULT
now too.

Also, you might wanna converge the exit paths to a single "out" label
if the unconditional JMPs are not that big of a deal (pasting the whole
thing and not as a diff because diff is unreadable):

SYM_CODE_START_LOCAL(.Lcopy_user_handle_tail)
        movl %edx,%ecx
        cmp $X86_TRAP_MC,%eax           /* check if X86_TRAP_MC */
        je 3f

1:      rep movsb
        mov %ecx,%eax
        jmp out

2:      cmp $X86_TRAP_MC,%eax
        je 3f
        mov %ecx,%eax
        jmp out

        /* Return -EFAULT for the machine check cases */
3:      movl $-EFAULT,%eax

out:
        ASM_CLAC
        ret

        _ASM_EXTABLE_CPY(1b, 2b)
SYM_CODE_END(.Lcopy_user_handle_tail)

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette