linux-kernel - Re: [PATCH 11/14] x86/fpu: Split copy_user_to_xstate() into copy_kernel_to_xstate() & copy_user_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:   Fri, 27 Jan 2017 11:54:33 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andy Lutomirski <luto@...capital.net>,
        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>,
        Yu-cheng Yu <yu-cheng.yu@...el.com>
Subject: Re: [PATCH 11/14] x86/fpu: Split copy_user_to_xstate() into
 copy_kernel_to_xstate() & copy_user_to_xstate()

On Thu, Jan 26, 2017 at 11:22:56AM +0100, Ingo Molnar wrote:
> Similar to:
> 
>   x86/fpu: Split copy_xstate_to_user() into copy_xstate_to_kernel() & copy_xstate_to_user()
> 
> No change in functionality.

...

> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 7d24fe305d4b..aed9b2cb4d3d 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1083,7 +1083,71 @@ int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i
>  }
>  
>  /*
> - * Convert from a ptrace standard-format buffer to kernel XSAVES format
> + * Convert from a ptrace standard-format kernel buffer to kernel XSAVES format
> + * and copy to the target thread. This is called from xstateregs_set() and
> + * there we check the CPU has XSAVES and a whole standard-sized buffer
> + * exists.
> + */
> +int copy_kernel_to_xstate(const void *kbuf, const void __user *ubuf,
> +		     struct xregs_state *xsave)

I'm wondering if we could avoid that code duplication in a cleaner way.
So that function is doing two things conceptually:

* get xfeatures
* iterate over them

It is being called by xstateregs_set(), for example. So you could do:

xstateregs_set:

	if (kbuf)
		copy_kernel_to_xstate()
	else
		copy_user_to_xstate()

Now those two could assign the copying function pointers:

	copy_fn = memcpy;

and
	copy_fn = __copy_from_user;

or a wrapper or whatever.

And then call a "workhorse" function:

	/* get_xfeatures */
	__copy_x_to_xstate(xstate, xfeatures, buf, copy_fn);

The buf thing would have to be cast to (void *) to drop the __user
annotation, though. Would that still be ok for sparse, I dunno?

Hmmm?

> +{
> +	unsigned int offset, size;
> +	int i;
> +	u64 xfeatures;
> +	u64 allowed_features;
> +
> +	offset = offsetof(struct xregs_state, header);
> +	size = sizeof(xfeatures);
> +
> +	if (kbuf) {
> +		memcpy(&xfeatures, kbuf + offset, size);
> +	} else {
> +		if (__copy_from_user(&xfeatures, ubuf + offset, size))
> +			return -EFAULT;
> +	}
> +
> +	/*
> +	 * Reject if the user sets any disabled or supervisor features:
> +	 */
> +	allowed_features = xfeatures_mask & ~XFEATURE_MASK_SUPERVISOR;
> +
> +	if (xfeatures & ~allowed_features)
> +		return -EINVAL;
> +
> +	for (i = 0; i < XFEATURE_MAX; i++) {
> +		u64 mask = ((u64)1 << i);
> +
> +		if (xfeatures & mask) {

So this mask is redundant - this could be done this way:

		if (xfeatures & BIT_ULL(i))

> +			void *dst = __raw_xsave_addr(xsave, 1 << i);

This thing looks yucky too:

we have a bit number, we convert it to a bit mask, in __raw_xsave_addr()
we convert it *back* to a bit number... are we testing the bitops speed
of the CPU or what's going on?

AFAICT, this interface could simply be converted to accepting bit
numbers and the callers of get_xsave_addr() could be simplified too.
>From looking at fill_xsave() and load_xsave() in kvm, they already
generate the bit indices for CPUID - no need to generate the masks too.

Btw, __raw_xsave_addr() can be static too.

Btw 2,

void *get_xsave_addr(struct xregs_state *xsave, int xstate_feature)

	...

        WARN_ONCE(!(xfeatures_mask & xstate_feature),
                  "get of unsupported state");
		  ^^^^^^^^^^^^^^^^^^^^^^^^^

that's funny english.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.