Re: [PATCH 1/2] x86: math-emu: check __copy_from_user result

[Kees Cook]

On Tue, Oct 01, 2019 at 04:23:34PM +0200, Arnd Bergmann wrote:
> The new __must_check annotation on __copy_from_user successfully
> identified some code that has lacked the check since at least
> linux-2.1.73:
> 
> arch/x86/math-emu/reg_ld_str.c:88:2: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
>         __copy_from_user(sti_ptr, s, 10);
>         ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~
> arch/x86/math-emu/reg_ld_str.c:1129:2: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
>         __copy_from_user(register_base + offset, s, other);
>         ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> arch/x86/math-emu/reg_ld_str.c:1131:3: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
>                 __copy_from_user(register_base, s + other, offset);
>                 ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> In addition, the get_user/put_user helpers do not enforce a return value
> check, but actually still require one. These have been missing
> for even longer.
> 
> Change the internal wrappers around get_user/put_user to force
> a signal and add a corresponding wrapper around __copy_from_user
> to check all such cases.
> 
> Fixes: 257e458057e5 ("Import 2.1.73")
> Fixes: 9dd819a15162 ("uaccess: add missing __must_check attributes")
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>

Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>

Notes below...

> ---
>  arch/x86/math-emu/fpu_system.h | 6 ++++--
>  arch/x86/math-emu/reg_ld_str.c | 6 +++---
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/math-emu/fpu_system.h b/arch/x86/math-emu/fpu_system.h
> index f98a0c956764..9b41391867dc 100644
> --- a/arch/x86/math-emu/fpu_system.h
> +++ b/arch/x86/math-emu/fpu_system.h
> @@ -107,6 +107,8 @@ static inline bool seg_writable(struct desc_struct *d)
>  #define FPU_access_ok(y,z)	if ( !access_ok(y,z) ) \
>  				math_abort(FPU_info,SIGSEGV)
>  #define FPU_abort		math_abort(FPU_info, SIGSEGV)
> +#define FPU_copy_from_user(to, from, n)	\
> +		do { if (copy_from_user(to, from, n)) FPU_abort; } while (0)
>  
>  #undef FPU_IGNORE_CODE_SEGV
>  #ifdef FPU_IGNORE_CODE_SEGV
> @@ -122,7 +124,7 @@ static inline bool seg_writable(struct desc_struct *d)
>  #define	FPU_code_access_ok(z) FPU_access_ok((void __user *)FPU_EIP,z)
>  #endif
>  
> -#define FPU_get_user(x,y)       get_user((x),(y))
> -#define FPU_put_user(x,y)       put_user((x),(y))
> +#define FPU_get_user(x,y) do { if (get_user((x),(y))) FPU_abort; } while (0)
> +#define FPU_put_user(x,y) do { if (put_user((x),(y))) FPU_abort; } while (0)
>  
>  #endif
> diff --git a/arch/x86/math-emu/reg_ld_str.c b/arch/x86/math-emu/reg_ld_str.c
> index f3779743d15e..fe6246ff9887 100644
> --- a/arch/x86/math-emu/reg_ld_str.c
> +++ b/arch/x86/math-emu/reg_ld_str.c
> @@ -85,7 +85,7 @@ int FPU_load_extended(long double __user *s, int stnr)
>  
>  	RE_ENTRANT_CHECK_OFF;
>  	FPU_access_ok(s, 10);
> -	__copy_from_user(sti_ptr, s, 10);
> +	FPU_copy_from_user(sti_ptr, s, 10);

These access_ok() checks seem redundant everywhere in this file (after
your switch from __copy* to copy*. I mean, I guess, just leave them, but
*shrug*

-Kees

>  	RE_ENTRANT_CHECK_ON;
>  
>  	return FPU_tagof(sti_ptr);
> @@ -1126,9 +1126,9 @@ void frstor(fpu_addr_modes addr_modes, u_char __user *data_address)
>  	/* Copy all registers in stack order. */
>  	RE_ENTRANT_CHECK_OFF;
>  	FPU_access_ok(s, 80);
> -	__copy_from_user(register_base + offset, s, other);
> +	FPU_copy_from_user(register_base + offset, s, other);
>  	if (offset)
> -		__copy_from_user(register_base, s + other, offset);
> +		FPU_copy_from_user(register_base, s + other, offset);
>  	RE_ENTRANT_CHECK_ON;
>  
>  	for (i = 0; i < 8; i++) {
> -- 
> 2.20.0
> 

-- 
Kees Cook