x86/fpu: copyout_from_xsaves & copyin_to_xsaves fixes

arch/x86/kernel/fpu/xstate.c int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf, · line 1001

That's not a robust way to do a bounds check either - what if 'offset' is so large that it overflows and offset + size falls within the 'sane' 0..count range?

Also, what about partial copies?

Plus, to add insult to injury, xstate_copyout() is a totally unnecessary obfuscation to begin with:

- 'start_pos' is always 0

- 'end_pos' is always 'count'

- both are const for no good reason: they are not pointers

- both are signed for no good reason: they are derived from unsigned types and I don't see how negative values can ever be valid here.

So this code:

static inline int xstate_copyout(unsigned int pos, unsigned int count,
void *kbuf, void __user *ubuf, const void *data, const int start_pos, const int end_pos)

{
        if ((count == 0) || (pos < start_pos))
                return 0;

        if (end_pos < 0 || pos < end_pos) {
                unsigned int copy = (end_pos < 0 ? count : min(count, end_pos - pos));

                if (kbuf) {
                        memcpy(kbuf + pos, data, copy);
                } else {
                        if (__copy_to_user(ubuf + pos, data, copy))
                                return -EFAULT;
                }
        }
        return 0;
}

Is, after all the cleanups and fixes is in reality equivalent to:

static inline int
__copy_xstate_to_kernel(void *kbuf, const void *data,
unsigned int offset, unsigned int size)

{
        memcpy(kbuf + offset, data, size);

        return 0;
}

!!!

So the real fix is to get rid of xstate_copyout() altogether and just do the memcpy directly - the regset obfuscation actively hid a real bug...

Thanks,

Ingo

arch/x86/kernel/fpu/xstate.c int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf, · line 1017

So this u64 copy copies both i387.mxcsr and i387.mxcsr_mask, which only works because the two fields are next to each other and there's no hole inbetween in the structure, right?

That fact should at minimum be commented upon.

arch/x86/kernel/fpu/xstate.c int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,

Also, I clearly wasn't paying enough attention when I merged the commit that introduced these ptrace conversion bits:

91c3dba7dbc1 ("x86/fpu/xstate: Fix PTRACE frames for XSAVES")

1)

the 'copyin/copyout' nomenclature needlessly departs from what the modern FPU code uses, which is:

copy_fpregs_to_fpstate()
copy_fpstate_to_sigframe()
copy_fregs_to_user()
copy_fxregs_to_kernel()
copy_fxregs_to_user()
copy_kernel_to_fpregs()
copy_kernel_to_fregs()
copy_kernel_to_fxregs()
copy_kernel_to_xregs()
copy_user_to_fregs()
copy_user_to_fxregs()
copy_user_to_xregs()
copy_xregs_to_kernel()
copy_xregs_to_user()

I.e. according to this pattern, the following rename should be done:

copyin_to_xsaves() -> copy_user_to_xstate()
copyout_from_xsaves() -> copy_xstate_to_user()

or, if we want to be pedantic, denote that that the user-space format is ptrace:

copyin_to_xsaves() -> copy_user_ptrace_to_xstate()
copyout_from_xsaves() -> copy_xstate_to_user_ptrace()

(But I'd suggest the shorter, non-pedantic name.)

But there's other problems:

2)

The copy_user_to_xstate() parameter order departs from the regular memcpy() pattern we try to follow:

int copy_user_to_xstate(const void *kbuf, const void __user *ubuf,

                     struct xregs_state *xsave);

it should be the other way around:

int copy_user_to_xstate(struct xregs_state *xsave, const void *kbuf, const void __user *ubuf)

3)

But there's worse problems - the 'kbuf' parameter in both APIs, for example in copy_xstate_to_user():

        if (kbuf) {
                memcpy(&xfeatures, kbuf + offset, size);
        } else {
                if (__copy_from_user(&xfeatures, ubuf + offset, size))
                        return -EFAULT;
	}

WTF: memory copy API semantics dependent on argument presence? Whether it's truly a 'user' copy depends on whether 'kbuf' is NULL??

This should be split into four APIs:

copy_xstate_to_user()
copy_xstate_to_kernel()
copy_user_to_xstate()
copy_kernel_to_xstate()

This decoupling would remove the weird 'kbuf, ubuf, xstate' triple argument dependence and turn them into regular two-argument memcpy() variant APIs:

copy_xstate_to_user (ubuf, xstate)
copy_xstate_to_kernel (kbuf, xstate)
copy_user_to_xstate (xstate, ubuf)
copy_kernel_to_xstate (xstate, kbuf)

... and would restore the type cleanliness/robustness of these APIs as well.

4)

arch/x86/kernel/fpu/xstate.c int copyin_to_xsaves(const void *kbuf, const void __user *ubuf, · line 1094

So this pattern is used twice and it's quite a mouthful. How about introducing such a helper:

/*
* Weird legacy quirk: indicate whether the MXCSR/MXCSR_MASK part of the FP state
* is used, even though the xfeatures flag lies about it being unused:
*/
static inline bool xfeatures_fp_mxcsr_used(u64 xfeatures)

{
	if (!(xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM)))
		return 0;

	if (xfeatures & XFEATURE_MASK_FP)
		return 0;

	return 1;
}

?

5)

While at it I noticed this code:

                u64 mask = ((u64)1 << i);

instead of the ugly type cast, cannot that be written as:

		u64 mask = 1ULL << i;

which is shorter and cleaner?

I.e. this code needs some serious love and I'm not surprised it had bugs in it...

But hindsight is 20/20 and I merged it myself and all that, so I'm not really complaining - but let's not repeat the mistake, ok?

Thanks,

Ingo

arch/x86/kernel/fpu/xstate.c int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf, · line 1012

Patch looks functionally good to me. One nit on the comment, though:
Usually XSAVE "state" refers to the state in the CPU while the "area" refers to the buffer in memory. Probably best to talk about the "legacy FP area", not state.

Maybe something like this for the first paragraph:

MXCSR and MXCSR_MASK are part of the SSE and YMM states and are saved/restored along with those states. However, they are located in the legacy FP *area* of the XSAVE buffer.

arch/x86/kernel/fpu/xstate.c int copyin_to_xsaves(const void *kbuf, const void __user *ubuf, · line 1092

In xstateregs_set(), we enforced the starting pos must be from (0), which in XSAVE time, was probably for this reason. The real mistake here, I think, is allowing skipping of xstate[0] and xstate[1]. Both should have been there even for XSAVES compacted-format. Would it be a simpler fix just making sure xstate[0] and xstate[1] are copied?

Yu-cheng