Re: [PATCH 09/14] x86/fpu: Change 'size_total' parameter to unsigned and standardize the size checks in copy_xstate_to_*()

[Yu-cheng Yu]

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@xxxxxxxxxx>
> Cc: Andy Lutomirski <luto@xxxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxxxx>
> Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> Cc: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> Cc: H. Peter Anvin <hpa@xxxxxxxxx>
> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Cc: Oleg Nesterov <oleg@xxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx>
> Cc: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> Signed-off-by: Ingo Molnar <mingo@xxxxxxxxxx>
> ---
>  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