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 <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Dave Hansen <[email protected]>
> Cc: Fenghua Yu <[email protected]>
> Cc: H. Peter Anvin <[email protected]>
> Cc: Linus Torvalds <[email protected]>
> Cc: Oleg Nesterov <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Yu-cheng Yu <[email protected]>
> Cc: Fenghua Yu <[email protected]>
> Signed-off-by: Ingo Molnar <[email protected]>
> ---
> 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
