linux-kernel - Re: [PATCH 09/14] x86/fpu: Change 'size_total' parameter to unsigned and standardize the size checks in copy_xstate_to

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]

Date:   Mon, 30 Jan 2017 09:11:59 -0800
From:   Yu-cheng Yu <yu-cheng.yu@...el.com>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andy Lutomirski <luto@...capital.net>,
        Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        "H . Peter Anvin" <hpa@...or.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Oleg Nesterov <oleg@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Rik van Riel <riel@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 09/14] x86/fpu: Change 'size_total' parameter to unsigned
 and standardize the size checks in copy_xstate_to_*()

On Thu, Jan 26, 2017 at 11:22:54AM +0100, Ingo Molnar wrote:
> 'size_total' is derived from an unsigned input parameter - and then converted
> to 'int' and checked for negative ranges:
> 
> 	if (size_total < 0 || offset < size_total) {
> 
> This conversion and the checks are unnecessary obfuscation, reject overly
> large requested copy sizes outright and simplify the underlying code.
> 
> Reported-by: Rik van Riel <riel@...hat.com>
> Cc: Andy Lutomirski <luto@...nel.org>
> Cc: Borislav Petkov <bp@...en8.de>
> Cc: Dave Hansen <dave.hansen@...ux.intel.com>
> Cc: Fenghua Yu <fenghua.yu@...el.com>
> Cc: H. Peter Anvin <hpa@...or.com>
> Cc: Linus Torvalds <torvalds@...ux-foundation.org>
> Cc: Oleg Nesterov <oleg@...hat.com>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Yu-cheng Yu <yu-cheng.yu@...el.com>
> Cc: Fenghua Yu <fenghua.yu@...el.com>
> Signed-off-by: Ingo Molnar <mingo@...nel.org>
> ---
>  arch/x86/kernel/fpu/xstate.c | 32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 8f9da89015e6..cceabca485c8 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -924,15 +924,11 @@ int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
>   * the source data pointer or increment pos, count, kbuf, and ubuf.
>   */
>  static inline int
> -__copy_xstate_to_kernel(void *kbuf,
> -			const void *data,
> -			unsigned int offset, unsigned int size, int size_total)
> +__copy_xstate_to_kernel(void *kbuf, const void *data,
> +			unsigned int offset, unsigned int size, unsigned int size_total)
>  {
> -	if (!size)
> -		return 0;
> -
> -	if (size_total < 0 || offset < size_total) {
> -		unsigned int copy = size_total < 0 ? size : min(size, size_total - offset);
> +	if (offset < size_total) {
> +		unsigned int copy = min(size, size_total - offset);
>  
>  		memcpy(kbuf + offset, data, copy);
>  	}
> @@ -985,12 +981,13 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of
>  			offset = xstate_offsets[i];
>  			size = xstate_sizes[i];
>  
> +			/* The next component has to fit fully into the output buffer: */
> +			if (offset + size > size_total)
> +				break;

This makes sense, but would be different from the non-compacted format path where this
rule is not enforced.  Do we want to unify both?

Yu-cheng