x86/fpu: Clean up ptrace copying functions
cover x86/fpu: Clean up ptrace copying functions
This series deobfuscates the ptrace/regset handling functions of the x86 FPU code. As a side effect it (should ...) fix the bug Rik reported in:
[PATCH 1/2] x86/fpu: move copyout_from_xsaves bounds check before the copy
but it does not handle the other fix yet:
[PATCH 2/2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state
... which needs to be rebased on top of this series.
The code got larger, but it is now a lot more standard and a lot easier to understand as well.
Only very minimally tested: we should also add various ptrace XSAVES testcases to tools/testing/selftests/x86/.
Obviously this will have to be backported as a single group of commits - but I'd rather do that than leave the mess around.
Ingo Molnar (14):
x86/fpu: Rename copyin_to_xsaves()/copyout_from_xsaves() to copy_user_to_xstate()/copy_xstate_to_user() x86/fpu: Split copy_xstate_to_user() into copy_xstate_to_kernel() & copy_xstate_to_user() x86/fpu: Remove 'ubuf' parameter from the copy_xstate_to_kernel() APIs
x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs
x86/fpu: Clean up parameter order in the copy_xstate_to_*() APIs
x86/fpu: Clean up the parameter definitions of copy_xstate_to_*()
x86/fpu: Remove the 'start_pos' parameter from the __copy_xstate_to_*() functions x86/fpu: Clarify parameter names in the copy_xstate_to_*() methods
x86/fpu: Change 'size_total' parameter to unsigned and standardize the size checks in copy_xstate_to_*() x86/fpu: Simplify __copy_xstate_to_kernel() return values
x86/fpu: Split copy_user_to_xstate() into copy_kernel_to_xstate() & copy_user_to_xstate() x86/fpu: Remove 'ubuf' parameter from the copy_kernel_to_xstate() API
x86/fpu: Remove 'kbuf' parameter from the copy_user_to_xstate() API
x86/fpu: Flip the parameter order in copy_*_to_xstate()
arch/x86/include/asm/fpu/xstate.h | 8 +-- arch/x86/kernel/fpu/regset.c | 15 +++-- arch/x86/kernel/fpu/signal.c | 11 ++-- arch/x86/kernel/fpu/xstate.c | 188 ++++++++++++++++++++++++++++++++++++++++++++++------------ 4 files changed, 168 insertions(+), 54 deletions(-)
Signature · 1 line
2.7.4
PATCH 1/14 x86/fpu: Rename copyin_to_xsaves()/copyout_from_xsaves() to copy_user_to_xstate()/copy_xstate_to_user()
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.
--- arch/x86/include/asm/fpu/xstate.h | 4 ++-- arch/x86/kernel/fpu/regset.c | 4 ++-- arch/x86/kernel/fpu/signal.c | 2 +- arch/x86/kernel/fpu/xstate.c | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-)
arch/x86/include/asm/fpu/xstate.h +2 -2
@@ -48,8 +48,8 @@ void fpu__xstate_clear_all_cpu_caps(void);void *get_xsave_addr (struct xregs_state *xsave,int xstate);const void *get_xsave_field_ptr (int xstate_field);int using_compacted_format (void ); -int copyout_from_xsaves (unsigned int pos,unsigned int count,void *kbuf, +int copy_xstate_to_user (unsigned int pos,unsigned int count,void *kbuf,void __user *ubuf,struct xregs_state *xsave); -int copyin_to_xsaves (const void *kbuf,const void __user *ubuf, +int copy_user_to_xstate (const void *kbuf,const void __user *ubuf,struct xregs_state *xsave);#endif
arch/x86/kernel/fpu/regset.c +2 -2
@@ -91,7 +91,7 @@ int xstateregs_get(struct task_struct *target, const struct user_regset *regset,fpu__activate_fpstate_read (fpu);if (using_compacted_format ()) { - ret =copyout_from_xsaves (pos, count, kbuf, ubuf, xsave); + ret =copy_xstate_to_user (pos, count, kbuf, ubuf, xsave); }else {fpstate_sanitize_xstate (fpu);/*
@@ -131,7 +131,7 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,fpu__activate_fpstate_write (fpu);if (boot_cpu_has (X86_FEATURE_XSAVES )) - ret =copyin_to_xsaves (kbuf, ubuf, xsave); + ret =copy_user_to_xstate (kbuf, ubuf, xsave);else ret =user_regset_copyin (&pos, &count, &kbuf, &ubuf, xsave,0 ,-1 );
arch/x86/kernel/fpu/signal.c +1 -1
@@ -324,7 +324,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)fpu__drop (fpu);if (using_compacted_format ()) { - err =copyin_to_xsaves (NULL , buf_fx, + err =copy_user_to_xstate (NULL , buf_fx, &fpu->state.xsave); }else { err = __copy_from_user(&fpu->state.xsave,
arch/x86/kernel/fpu/xstate.c +2 -2
@@ -950,7 +950,7 @@ static inline int xstate_copyout(unsigned int pos, unsigned int count,* zero. This is called from xstateregs_get() and there we check the CPU * has XSAVES. */ -int copyout_from_xsaves (unsigned int pos,unsigned int count,void *kbuf, +int copy_xstate_to_user (unsigned int pos,unsigned int count,void *kbuf,void __user *ubuf,struct xregs_state *xsave) {unsigned int offset, size;
@@ -1022,7 +1022,7 @@ int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf,* there we check the CPU has XSAVES and a whole standard-sized buffer * exists. */ -int copyin_to_xsaves (const void *kbuf,const void __user *ubuf, +int copy_user_to_xstate (const void *kbuf,const void __user *ubuf,struct xregs_state *xsave) {unsigned int offset, size;
Signature · 1 line
2.7.4
PATCH 2/14 x86/fpu: Split copy_xstate_to_user() into copy_xstate_to_kernel() & copy_xstate_to_user()
In reply to Ingo Molnarcopy_xstate_to_user() is a weird API - in part due to a bad API inherited from the regset APIs.
But don't propagate that bad API choice into the FPU code - so as a first step split the API into kernel and user buffer handling routines.
(Also split the xstate_copyout() internal helper.)
The split API is a dumb duplication that should be obviously correct, the real splitting will be done in the next patch.
--- arch/x86/include/asm/fpu/xstate.h | 4 +-- arch/x86/kernel/fpu/regset.c | 5 ++- arch/x86/kernel/fpu/xstate.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 109 insertions(+), 10 deletions(-)
arch/x86/include/asm/fpu/xstate.h +2 -2
@@ -48,8 +48,8 @@ void fpu__xstate_clear_all_cpu_caps(void);void *get_xsave_addr (struct xregs_state *xsave,int xstate);const void *get_xsave_field_ptr (int xstate_field);int using_compacted_format (void ); -int copy_xstate_to_user (unsigned int pos,unsigned int count,void *kbuf, -void __user *ubuf,struct xregs_state *xsave); +int copy_xstate_to_kernel (unsigned int pos,unsigned int count,void *kbuf,void __user *ubuf,struct xregs_state *xsave); +int copy_xstate_to_user (unsigned int pos,unsigned int count,void *kbuf,void __user *ubuf,struct xregs_state *xsave);int copy_user_to_xstate (const void *kbuf,const void __user *ubuf,struct xregs_state *xsave);#endif
arch/x86/kernel/fpu/regset.c +4 -1
@@ -91,7 +91,10 @@ int xstateregs_get(struct task_struct *target, const struct user_regset *regset,fpu__activate_fpstate_read (fpu);if (using_compacted_format ()) { - ret =copy_xstate_to_user (pos, count, kbuf, ubuf, xsave); +if (kbuf) + ret =copy_xstate_to_kernel (pos, count, kbuf, ubuf, xsave); +else + ret =copy_xstate_to_user (pos, count, kbuf, ubuf, xsave); }else {fpstate_sanitize_xstate (fpu);/*
arch/x86/kernel/fpu/xstate.c +103 -7
134 lines not on this page · raw message
Signature · 1 line
2.7.4
PATCH 3/14 x86/fpu: Remove 'ubuf' parameter from the copy_xstate_to_kernel() APIs
In reply to Ingo MolnarThe 'ubuf' parameter is unused in the _kernel() side of the API, remove it.
This simplifies the code and makes it easier to think about.
--- arch/x86/include/asm/fpu/xstate.h | 2 +- arch/x86/kernel/fpu/regset.c | 2 +- arch/x86/kernel/fpu/xstate.c | 21 ++++++--------------- 3 files changed, 8 insertions(+), 17 deletions(-)
arch/x86/include/asm/fpu/xstate.h +1 -1
@@ -48,7 +48,7 @@ void fpu__xstate_clear_all_cpu_caps(void);void *get_xsave_addr (struct xregs_state *xsave,int xstate);const void *get_xsave_field_ptr (int xstate_field);int using_compacted_format (void ); -int copy_xstate_to_kernel (unsigned int pos,unsigned int count,void *kbuf,void __user *ubuf,struct xregs_state *xsave); +int copy_xstate_to_kernel (unsigned int pos,unsigned int count,void *kbuf,struct xregs_state *xsave);int copy_xstate_to_user (unsigned int pos,unsigned int count,void *kbuf,void __user *ubuf,struct xregs_state *xsave);int copy_user_to_xstate (const void *kbuf,const void __user *ubuf,struct xregs_state *xsave);
arch/x86/kernel/fpu/regset.c +1 -1
@@ -92,7 +92,7 @@ int xstateregs_get(struct task_struct *target, const struct user_regset *regset,if (using_compacted_format ()) {if (kbuf) - ret =copy_xstate_to_kernel (pos, count, kbuf, ubuf, xsave); + ret =copy_xstate_to_kernel (pos, count, kbuf, xsave);else ret =copy_xstate_to_user (pos, count, kbuf, ubuf, xsave); }else {
arch/x86/kernel/fpu/xstate.c +6 -15
@@ -925,7 +925,7 @@ int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,*/ static inline int __copy_xstate_to_kernel (unsigned int pos,unsigned int count, -void *kbuf,void __user *ubuf, +void *kbuf,const void *data,const int start_pos,const int end_pos) {
@@ -935,12 +935,7 @@ __copy_xstate_to_kernel(unsigned int pos, unsigned int count,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 ; - } +memcpy (kbuf + pos, data, copy); }return 0 ; }
@@ -952,8 +947,7 @@ __copy_xstate_to_kernel(unsigned int pos, unsigned int count,* It supports partial copy but pos always starts from zero. This is called * from xstateregs_get() and there we check the CPU has XSAVES. */ -int copy_xstate_to_kernel (unsigned int pos,unsigned int count,void *kbuf, -void __user *ubuf,struct xregs_state *xsave) +int copy_xstate_to_kernel (unsigned int pos,unsigned int count,void *kbuf,struct xregs_state *xsave) {unsigned int offset, size;int ret, i;
@@ -978,8 +972,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, offset = offsetof(struct xregs_state , header); size =sizeof (header); - ret =__copy_xstate_to_kernel (offset, size, kbuf, ubuf, &header,0 , count); - + ret =__copy_xstate_to_kernel (offset, size, kbuf, &header,0 , count);if (ret)return ret;
@@ -993,8 +986,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, offset = xstate_offsets[i]; size = xstate_sizes[i]; - ret =__copy_xstate_to_kernel (offset, size, kbuf, ubuf, src,0 , count); - + ret =__copy_xstate_to_kernel (offset, size, kbuf, src,0 , count);if (ret)return ret;
@@ -1010,8 +1002,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, offset = offsetof(struct fxregs_state , sw_reserved); size =sizeof (xstate_fx_sw_bytes); - ret =__copy_xstate_to_kernel (offset, size, kbuf, ubuf, xstate_fx_sw_bytes,0 , count); - + ret =__copy_xstate_to_kernel (offset, size, kbuf, xstate_fx_sw_bytes,0 , count);if (ret)return ret;
Signature · 1 line
2.7.4
PATCH 4/14 x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs
In reply to Ingo MolnarThe 'kbuf' parameter is unused in the _user() side of the API, remove it.
This simplifies the code and makes it easier to think about.
--- arch/x86/include/asm/fpu/xstate.h | 2 +- arch/x86/kernel/fpu/regset.c | 2 +- arch/x86/kernel/fpu/xstate.c | 25 +++++++------------------ 3 files changed, 9 insertions(+), 20 deletions(-)
arch/x86/include/asm/fpu/xstate.h +1 -1
@@ -49,7 +49,7 @@ void *get_xsave_addr(struct xregs_state *xsave, int xstate);const void *get_xsave_field_ptr (int xstate_field);int using_compacted_format (void );int copy_xstate_to_kernel (unsigned int pos,unsigned int count,void *kbuf,struct xregs_state *xsave); -int copy_xstate_to_user (unsigned int pos,unsigned int count,void *kbuf,void __user *ubuf,struct xregs_state *xsave); +int copy_xstate_to_user (unsigned int pos,unsigned int count,void __user *ubuf,struct xregs_state *xsave);int copy_user_to_xstate (const void *kbuf,const void __user *ubuf,struct xregs_state *xsave);#endif
arch/x86/kernel/fpu/regset.c +1 -1
@@ -94,7 +94,7 @@ int xstateregs_get(struct task_struct *target, const struct user_regset *regset,if (kbuf) ret =copy_xstate_to_kernel (pos, count, kbuf, xsave);else - ret =copy_xstate_to_user (pos, count, kbuf, ubuf, xsave); + ret =copy_xstate_to_user (pos, count, ubuf, xsave); }else {fpstate_sanitize_xstate (fpu);/*
arch/x86/kernel/fpu/xstate.c +7 -18
@@ -1010,10 +1010,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, stru }static inline int -__copy_xstate_to_user (unsigned int pos,unsigned int count, -void *kbuf,void __user *ubuf, -const void *data,const int start_pos, -const int end_pos) +__copy_xstate_to_user (unsigned int pos,unsigned int count,void __user *ubuf,const void *data,const int start_pos,const int end_pos) {if ((count ==0 ) || (pos < start_pos))return 0 ;
@@ -1021,12 +1018,8 @@ __copy_xstate_to_user(unsigned int pos, unsigned int count,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 ; - } +if (__copy_to_user (ubuf + pos, data, copy)) +return -EFAULT ; }return 0 ; }
@@ -1037,8 +1030,7 @@ __copy_xstate_to_user(unsigned int pos, unsigned int count,* zero. This is called from xstateregs_get() and there we check the CPU * has XSAVES. */ -int copy_xstate_to_user (unsigned int pos,unsigned int count,void *kbuf, -void __user *ubuf,struct xregs_state *xsave) +int copy_xstate_to_user (unsigned int pos,unsigned int count,void __user *ubuf,struct xregs_state *xsave) {unsigned int offset, size;int ret, i;
@@ -1063,8 +1055,7 @@ int copy_xstate_to_user(unsigned int pos, unsigned int count, void *kbuf, offset = offsetof(struct xregs_state , header); size =sizeof (header); - ret =__copy_xstate_to_user (offset, size, kbuf, ubuf, &header,0 , count); - + ret =__copy_xstate_to_user (offset, size, ubuf, &header,0 , count);if (ret)return ret;
@@ -1078,8 +1069,7 @@ int copy_xstate_to_user(unsigned int pos, unsigned int count, void *kbuf, offset = xstate_offsets[i]; size = xstate_sizes[i]; - ret =__copy_xstate_to_user (offset, size, kbuf, ubuf, src,0 , count); - + ret =__copy_xstate_to_user (offset, size, ubuf, src,0 , count);if (ret)return ret;
@@ -1095,8 +1085,7 @@ int copy_xstate_to_user(unsigned int pos, unsigned int count, void *kbuf, offset = offsetof(struct fxregs_state , sw_reserved); size =sizeof (xstate_fx_sw_bytes); - ret =__copy_xstate_to_user (offset, size, kbuf, ubuf, xstate_fx_sw_bytes,0 , count); - + ret =__copy_xstate_to_user (offset, size, ubuf, xstate_fx_sw_bytes,0 , count);if (ret)return ret;
Signature · 1 line
2.7.4
Borislav Petkov
maintainerRe: [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs
Quoted 3 lines · Ingo Molnar
The 'kbuf' parameter is unused in the
_user()side of the API, remove it.This simplifies the code and makes it easier to think about.
...
arch/x86/kernel/fpu/xstate.c int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, stru · line 1013
Quoted 9 lines
arch/x86/kernel/fpu/xstate.c +1
-4@@ -1010,10 +1010,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, stru }static inline int -__copy_xstate_to_user (unsigned int pos,unsigned int count, -void *kbuf,void __user *ubuf, -const void *data,const int start_pos, -const int end_pos) +__copy_xstate_to_user (unsigned int pos,unsigned int count,void __user *ubuf,const void *data,const int start_pos,const int end_pos)
That and similar lines are insanely long and could be broken.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs
Quoted 10 lines · Borislav Petkov
Quoted 3 lines · Ingo Molnar
The 'kbuf' parameter is unused in the
_user()side of the API, remove it.This simplifies the code and makes it easier to think about.
...
Quoted 3 lines
(quoted diff) +0
-0@@ -1010,10 +1010,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, stru }
Quoted 8 lines
arch/x86/kernel/fpu/xstate.c int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, stru · line 1013
Quoted 6 lines
arch/x86/kernel/fpu/xstate.c +1
-4static inline int -__copy_xstate_to_user (unsigned int pos,unsigned int count, -void *kbuf,void __user *ubuf, -const void *data,const int start_pos, -const int end_pos) +__copy_xstate_to_user (unsigned int pos,unsigned int count,void __user *ubuf,const void *data,const int start_pos,const int end_pos)That and similar lines are insanely long and could be broken.
Yeah, so that's one point of the series: the interface was insanely complex (the original sin is that of the regset interfaces), and one symptom of that complexity are these overly long prototypes - the above one has 7 arguments (!!). Another, far more serious symptom of the complexity were the bugs that Rik found.
The solution was not to break the prototype into multiple lines and thus paper over one symptom of complexity, but to _reduce_ complexity.
So at the end of the series the basic copy_xstate_to_user() prototype looks like this:
static inline int
__copy_xstate_to_user(void __user *ubuf, const void *data, unsigned int offset, unsigned int size, unsigned int size_total)
which is less complex and shorter as well. It could probably be shortened further:
static inline int
__copy_xstate_to_user(void __user *ubuf, const void *data, u32 offset, u32 size, u32 total)
because our regset (and user-copy) APIs are intentionally 32-bit - but this would depart from the existing signature style so I'm not sure we want to do it unilaterally.
Would anyone object to using u32 in these prototypes?
Thanks,
Ingo
Borislav Petkov
maintainerRe: [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs
Quoted 1 line · Ingo Molnar
Would anyone object to using u32 in these prototypes?
Well, would there be any disadvantage to forcing them to u32? Potentially by something else wanting to use those interfaces besides the regset thing and that something else doesn't like u32s?
Otherwise, I don't see a problem.
I mean, if 4G are not enough for xstate dimensions then we have a whole different problem.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs
Quoted 11 lines · Borislav Petkov
Quoted 1 line · Ingo Molnar
Would anyone object to using u32 in these prototypes?
Well, would there be any disadvantage to forcing them to u32? Potentially by something else wanting to use those interfaces besides the regset thing and that something else doesn't like u32s?
Otherwise, I don't see a problem.
I mean, if 4G are not enough for xstate dimensions then we have a whole different problem.
This function pair was intended to be similar to user_regset_copyout(), user_regset_copyin() used for the standard-format XSAVE area copying. I totally agree it is complex and should be simplified. Why don't we do both places?
Yu-cheng
PATCH 5/14 x86/fpu: Clean up parameter order in the copy_xstate_to_*() APIs
In reply to Ingo MolnarParameter ordering is weird:
int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, struct xregs_state *xsave); int copy_xstate_to_user(unsigned int pos, unsigned int count, void __user *ubuf, struct xregs_state *xsave);
'pos' and 'count', which are attributes of the destination buffer, are listed before the destination buffer itself ...
List them after the primary arguments instead.
This makes the code more similar to regular memcpy() variant APIs.
No change in functionality.
--- arch/x86/include/asm/fpu/xstate.h | 4 ++-- arch/x86/kernel/fpu/regset.c | 4 ++-- arch/x86/kernel/fpu/xstate.c | 25 ++++++++++++------------- 3 files changed, 16 insertions(+), 17 deletions(-)
arch/x86/include/asm/fpu/xstate.h +2 -2
@@ -48,8 +48,8 @@ void fpu__xstate_clear_all_cpu_caps(void);void *get_xsave_addr (struct xregs_state *xsave,int xstate);const void *get_xsave_field_ptr (int xstate_field);int using_compacted_format (void ); -int copy_xstate_to_kernel (unsigned int pos,unsigned int count,void *kbuf,struct xregs_state *xsave); -int copy_xstate_to_user (unsigned int pos,unsigned int count,void __user *ubuf,struct xregs_state *xsave); +int copy_xstate_to_kernel (void *kbuf,struct xregs_state *xsave,unsigned int pos,unsigned int count); +int copy_xstate_to_user (void __user *ubuf,struct xregs_state *xsave,unsigned int pos,unsigned int count);int copy_user_to_xstate (const void *kbuf,const void __user *ubuf,struct xregs_state *xsave);#endif
arch/x86/kernel/fpu/regset.c +2 -2
@@ -92,9 +92,9 @@ int xstateregs_get(struct task_struct *target, const struct user_regset *regset,if (using_compacted_format ()) {if (kbuf) - ret =copy_xstate_to_kernel (pos, count, kbuf, xsave); + ret =copy_xstate_to_kernel (kbuf, xsave, pos, count);else - ret =copy_xstate_to_user (pos, count, ubuf, xsave); + ret =copy_xstate_to_user (ubuf, xsave, pos, count); }else {fpstate_sanitize_xstate (fpu);/*
arch/x86/kernel/fpu/xstate.c +12 -13
@@ -924,10 +924,9 @@ 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 (unsigned int pos,unsigned int count, -void *kbuf, -const void *data,const int start_pos, -const int end_pos) +__copy_xstate_to_kernel (void *kbuf, +const void *data, +unsigned int pos,unsigned int count,const int start_pos,const int end_pos) {if ((count ==0 ) || (pos < start_pos))return 0 ;
@@ -947,7 +946,7 @@ __copy_xstate_to_kernel(unsigned int pos, unsigned int count,* It supports partial copy but pos always starts from zero. This is called * from xstateregs_get() and there we check the CPU has XSAVES. */ -int copy_xstate_to_kernel (unsigned int pos,unsigned int count,void *kbuf,struct xregs_state *xsave) +int copy_xstate_to_kernel (void *kbuf,struct xregs_state *xsave,unsigned int pos,unsigned int count) {unsigned int offset, size;int ret, i;
@@ -972,7 +971,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, stru offset = offsetof(struct xregs_state , header); size =sizeof (header); - ret =__copy_xstate_to_kernel (offset, size, kbuf, &header,0 , count); + ret =__copy_xstate_to_kernel (kbuf, &header, offset, size,0 , count);if (ret)return ret;
@@ -986,7 +985,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, stru offset = xstate_offsets[i]; size = xstate_sizes[i]; - ret =__copy_xstate_to_kernel (offset, size, kbuf, src,0 , count); + ret =__copy_xstate_to_kernel (kbuf, src, offset, size,0 , count);if (ret)return ret;
@@ -1002,7 +1001,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, stru offset = offsetof(struct fxregs_state , sw_reserved); size =sizeof (xstate_fx_sw_bytes); - ret =__copy_xstate_to_kernel (offset, size, kbuf, xstate_fx_sw_bytes,0 , count); + ret =__copy_xstate_to_kernel (kbuf, xstate_fx_sw_bytes, offset, size,0 , count);if (ret)return ret;
@@ -1010,7 +1009,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, stru }static inline int -__copy_xstate_to_user (unsigned int pos,unsigned int count,void __user *ubuf,const void *data,const int start_pos,const int end_pos) +__copy_xstate_to_user (void __user *ubuf,const void *data,unsigned int pos,unsigned int count,const int start_pos,const int end_pos) {if ((count ==0 ) || (pos < start_pos))return 0 ;
@@ -1030,7 +1029,7 @@ __copy_xstate_to_user(unsigned int pos, unsigned int count, void __user *ubuf, c* zero. This is called from xstateregs_get() and there we check the CPU * has XSAVES. */ -int copy_xstate_to_user (unsigned int pos,unsigned int count,void __user *ubuf,struct xregs_state *xsave) +int copy_xstate_to_user (void __user *ubuf,struct xregs_state *xsave,unsigned int pos,unsigned int count) {unsigned int offset, size;int ret, i;
@@ -1055,7 +1054,7 @@ int copy_xstate_to_user(unsigned int pos, unsigned int count, void __user *ubuf, offset = offsetof(struct xregs_state , header); size =sizeof (header); - ret =__copy_xstate_to_user (offset, size, ubuf, &header,0 , count); + ret =__copy_xstate_to_user (ubuf, &header, offset, size,0 , count);if (ret)return ret;
@@ -1069,7 +1068,7 @@ int copy_xstate_to_user(unsigned int pos, unsigned int count, void __user *ubuf, offset = xstate_offsets[i]; size = xstate_sizes[i]; - ret =__copy_xstate_to_user (offset, size, ubuf, src,0 , count); + ret =__copy_xstate_to_user (ubuf, src, offset, size,0 , count);if (ret)return ret;
@@ -1085,7 +1084,7 @@ int copy_xstate_to_user(unsigned int pos, unsigned int count, void __user *ubuf, offset = offsetof(struct fxregs_state , sw_reserved); size =sizeof (xstate_fx_sw_bytes); - ret =__copy_xstate_to_user (offset, size, ubuf, xstate_fx_sw_bytes,0 , count); + ret =__copy_xstate_to_user (ubuf, xstate_fx_sw_bytes, offset, size,0 , count);if (ret)return ret;
Signature · 1 line
2.7.4
PATCH 6/14 x86/fpu: Clean up the parameter definitions of copy_xstate_to_*()
In reply to Ingo MolnarRemove pointless 'const' of non-pointer input parameter.
Remove unnecessary parenthesis that shows uncertainty about arithmetic operator precedence.
Clarify copy_xstate_to_user() description.
No change in functionality.
--- arch/x86/kernel/fpu/xstate.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
arch/x86/kernel/fpu/xstate.c +5 -5
@@ -926,13 +926,13 @@ int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,static inline int __copy_xstate_to_kernel (void *kbuf,const void *data, -unsigned int pos,unsigned int count,const int start_pos,const int end_pos) +unsigned int pos,unsigned int count,int start_pos,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)); +unsigned int copy = end_pos <0 ? count :min (count, end_pos - pos);memcpy (kbuf + pos, data, copy); }
@@ -1009,13 +1009,13 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int po }static inline int -__copy_xstate_to_user (void __user *ubuf,const void *data,unsigned int pos,unsigned int count,const int start_pos,const int end_pos) +__copy_xstate_to_user (void __user *ubuf,const void *data,unsigned int pos,unsigned int count,int start_pos,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)); +unsigned int copy = end_pos <0 ? count :min (count, end_pos - pos);if (__copy_to_user (ubuf + pos, data, copy))return -EFAULT ;
@@ -1025,7 +1025,7 @@ __copy_xstate_to_user(void __user *ubuf, const void *data, unsigned int pos, uns/* * Convert from kernel XSAVES compacted format to standard format and copy -* to a ptrace buffer. It supports partial copy but pos always starts from +* to a user-space buffer. It supports partial copy but pos always starts from * zero. This is called from xstateregs_get() and there we check the CPU * has XSAVES. */
Signature · 1 line
2.7.4
PATCH 7/14 x86/fpu: Remove the 'start_pos' parameter from the __copy_xstate_to_*() functions
In reply to Ingo Molnar'start_pos' is always 0, so remove it and remove the pointless check of 'pos < 0' which can not ever be true as 'pos' is unsigned ...
No change in functionality.
--- arch/x86/kernel/fpu/xstate.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
arch/x86/kernel/fpu/xstate.c +10 -10
@@ -926,9 +926,9 @@ int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,static inline int __copy_xstate_to_kernel (void *kbuf,const void *data, -unsigned int pos,unsigned int count,int start_pos,int end_pos) +unsigned int pos,unsigned int count,int end_pos) { -if ((count ==0 ) || (pos < start_pos)) +if (!count)return 0 ;if (end_pos <0 || pos < end_pos) {
@@ -971,7 +971,7 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int po offset = offsetof(struct xregs_state , header); size =sizeof (header); - ret =__copy_xstate_to_kernel (kbuf, &header, offset, size,0 , count); + ret =__copy_xstate_to_kernel (kbuf, &header, offset, size, count);if (ret)return ret;
@@ -985,7 +985,7 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int po offset = xstate_offsets[i]; size = xstate_sizes[i]; - ret =__copy_xstate_to_kernel (kbuf, src, offset, size,0 , count); + ret =__copy_xstate_to_kernel (kbuf, src, offset, size, count);if (ret)return ret;
@@ -1001,7 +1001,7 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int po offset = offsetof(struct fxregs_state , sw_reserved); size =sizeof (xstate_fx_sw_bytes); - ret =__copy_xstate_to_kernel (kbuf, xstate_fx_sw_bytes, offset, size,0 , count); + ret =__copy_xstate_to_kernel (kbuf, xstate_fx_sw_bytes, offset, size, count);if (ret)return ret;
@@ -1009,9 +1009,9 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int po }static inline int -__copy_xstate_to_user (void __user *ubuf,const void *data,unsigned int pos,unsigned int count,int start_pos,int end_pos) +__copy_xstate_to_user (void __user *ubuf,const void *data,unsigned int pos,unsigned int count,int end_pos) { -if ((count ==0 ) || (pos < start_pos)) +if (!count)return 0 ;if (end_pos <0 || pos < end_pos) {
@@ -1054,7 +1054,7 @@ int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i offset = offsetof(struct xregs_state , header); size =sizeof (header); - ret =__copy_xstate_to_user (ubuf, &header, offset, size,0 , count); + ret =__copy_xstate_to_user (ubuf, &header, offset, size, count);if (ret)return ret;
@@ -1068,7 +1068,7 @@ int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i offset = xstate_offsets[i]; size = xstate_sizes[i]; - ret =__copy_xstate_to_user (ubuf, src, offset, size,0 , count); + ret =__copy_xstate_to_user (ubuf, src, offset, size, count);if (ret)return ret;
@@ -1084,7 +1084,7 @@ int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i offset = offsetof(struct fxregs_state , sw_reserved); size =sizeof (xstate_fx_sw_bytes); - ret =__copy_xstate_to_user (ubuf, xstate_fx_sw_bytes, offset, size,0 , count); + ret =__copy_xstate_to_user (ubuf, xstate_fx_sw_bytes, offset, size, count);if (ret)return ret;
Signature · 1 line
2.7.4
PATCH 8/14 x86/fpu: Clarify parameter names in the copy_xstate_to_*() methods
In reply to Ingo MolnarRight now there's a confusing mixture of 'offset' and 'size' parameters:
- __copy_xstate_to_*() input parameter 'end_pos' not not really an offset, but the full size of the copy to be performed.
- input parameter 'count' to copy_xstate_to_*() shadows that of __copy_xstate_to_*()'s 'count' parameter name - but the roles are different: the first one is the total number of bytes to be copied, while the second one is a partial copy size.
To unconfuse all this, use a consistent set of parameter names:
- 'size' is the partial copy size within a single xstate component
- 'size_total' is the total copy requested
- 'offset_start' is the requested starting offset.
- 'offset' is the offset within an xstate component.
No change in functionality.
--- arch/x86/include/asm/fpu/xstate.h | 4 ++-- arch/x86/kernel/fpu/xstate.c | 44 ++++++++++++++++++++++---------------------- 2 files changed, 24 insertions(+), 24 deletions(-)
arch/x86/include/asm/fpu/xstate.h +2 -2
@@ -48,8 +48,8 @@ void fpu__xstate_clear_all_cpu_caps(void);void *get_xsave_addr (struct xregs_state *xsave,int xstate);const void *get_xsave_field_ptr (int xstate_field);int using_compacted_format (void ); -int copy_xstate_to_kernel (void *kbuf,struct xregs_state *xsave,unsigned int pos,unsigned int count); -int copy_xstate_to_user (void __user *ubuf,struct xregs_state *xsave,unsigned int pos,unsigned int count); +int copy_xstate_to_kernel (void *kbuf,struct xregs_state *xsave,unsigned int offset,unsigned int size); +int copy_xstate_to_user (void __user *ubuf,struct xregs_state *xsave,unsigned int offset,unsigned int size);int copy_user_to_xstate (const void *kbuf,const void __user *ubuf,struct xregs_state *xsave);#endif
arch/x86/kernel/fpu/xstate.c +22 -22
130 lines not on this page · raw message
Signature · 1 line
2.7.4
PATCH 9/14 x86/fpu: Change 'size_total' parameter to unsigned and standardize the size checks in copy_xstate_to_*()
In reply to Ingo Molnar'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.
--- arch/x86/kernel/fpu/xstate.c | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-)
arch/x86/kernel/fpu/xstate.c +15 -17
@@ -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 ; + ret =__copy_xstate_to_kernel (kbuf, src, offset, size, size_total);if (ret)return ret; - -if (offset + size >= size_total) -break ; } }
@@ -1009,13 +1006,13 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of }static inline int -__copy_xstate_to_user (void __user *ubuf,const void *data,unsigned int offset,unsigned int size,int size_total) +__copy_xstate_to_user (void __user *ubuf,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);if (__copy_to_user (ubuf + offset, data, copy))return -EFAULT ;
@@ -1068,12 +1065,13 @@ int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i 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 ; + ret =__copy_xstate_to_user (ubuf, src, offset, size, size_total);if (ret)return ret; - -if (offset + size >= size_total) -break ; } }
Signature · 1 line
2.7.4
Re: [PATCH 09/14] x86/fpu: Change 'size_total' parameter to unsigned and standardize the size checks in copy_xstate_to_*()
Quoted 49 lines · Ingo Molnar
'
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(-)arch/x86/kernel/fpu/xstate.c +4
-8@@ -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); }
arch/x86/kernel/fpu/xstate.c int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of · line 986
Quoted 6 lines
arch/x86/kernel/fpu/xstate.c +3
-0@@ -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
PATCH 10/14 x86/fpu: Simplify __copy_xstate_to_kernel() return values
In reply to Ingo Molnar__copy_xstate_to_kernel() can only return 0 (because kernel copies cannot fail), simplify the code throughout.
No change in functionality.
--- arch/x86/kernel/fpu/xstate.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)
arch/x86/kernel/fpu/xstate.c +5 -12
@@ -923,7 +923,7 @@ int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,* This is similar to user_regset_copyout(), but will not add offset to * the source data pointer or increment pos, count, kbuf, and ubuf. */ -static inline int +static inline void __copy_xstate_to_kernel (void *kbuf,const void *data,unsigned int offset,unsigned int size,unsigned int size_total) {
@@ -932,7 +932,6 @@ __copy_xstate_to_kernel(void *kbuf, const void *data,memcpy (kbuf + offset, data, copy); } -return 0 ; }/*
@@ -945,8 +944,8 @@ __copy_xstate_to_kernel(void *kbuf, const void *data,int copy_xstate_to_kernel (void *kbuf,struct xregs_state *xsave,unsigned int offset_start,unsigned int size_total) {unsigned int offset, size; -int ret, i;struct xstate_header header; +int i;/* * Currently copy_regset_to_user() starts from pos 0:
@@ -967,9 +966,7 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of offset = offsetof(struct xregs_state , header); size =sizeof (header); - ret =__copy_xstate_to_kernel (kbuf, &header, offset, size, size_total); -if (ret) -return ret; +__copy_xstate_to_kernel (kbuf, &header, offset, size, size_total);for (i =0 ; i <XFEATURE_MAX ; i++) {/*
@@ -985,9 +982,7 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int ofif (offset + size > size_total)break ; - ret =__copy_xstate_to_kernel (kbuf, src, offset, size, size_total); -if (ret) -return ret; +__copy_xstate_to_kernel (kbuf, src, offset, size, size_total); } }
@@ -998,9 +993,7 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of offset = offsetof(struct fxregs_state , sw_reserved); size =sizeof (xstate_fx_sw_bytes); - ret =__copy_xstate_to_kernel (kbuf, xstate_fx_sw_bytes, offset, size, size_total); -if (ret) -return ret; +__copy_xstate_to_kernel (kbuf, xstate_fx_sw_bytes, offset, size, size_total);return 0 ; }
Signature · 1 line
2.7.4
PATCH 11/14 x86/fpu: Split copy_user_to_xstate() into copy_kernel_to_xstate() & copy_user_to_xstate()
In reply to Ingo MolnarSimilar to:
x86/fpu: Split copy_xstate_to_user() into copy_xstate_to_kernel() & copy_xstate_to_user()
No change in functionality.
--- arch/x86/include/asm/fpu/xstate.h | 4 ++-- arch/x86/kernel/fpu/regset.c | 10 ++++++--- arch/x86/kernel/fpu/xstate.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 74 insertions(+), 6 deletions(-)
arch/x86/include/asm/fpu/xstate.h +2 -2
@@ -50,6 +50,6 @@ const void *get_xsave_field_ptr(int xstate_field);int using_compacted_format (void );int copy_xstate_to_kernel (void *kbuf,struct xregs_state *xsave,unsigned int offset,unsigned int size);int copy_xstate_to_user (void __user *ubuf,struct xregs_state *xsave,unsigned int offset,unsigned int size); -int copy_user_to_xstate (const void *kbuf,const void __user *ubuf, -struct xregs_state *xsave); +int copy_kernel_to_xstate (const void *kbuf,const void __user *ubuf,struct xregs_state *xsave); +int copy_user_to_xstate (const void *kbuf,const void __user *ubuf,struct xregs_state *xsave);#endif
arch/x86/kernel/fpu/regset.c +7 -3
@@ -133,10 +133,14 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,fpu__activate_fpstate_write (fpu); -if (boot_cpu_has (X86_FEATURE_XSAVES )) - ret =copy_user_to_xstate (kbuf, ubuf, xsave); -else +if (boot_cpu_has (X86_FEATURE_XSAVES )) { +if (kbuf) + ret =copy_kernel_to_xstate (kbuf, ubuf, xsave); +else + ret =copy_user_to_xstate (kbuf, ubuf, xsave); + }else { ret =user_regset_copyin (&pos, &count, &kbuf, &ubuf, xsave,0 ,-1 ); + }/* * In case of failure, mark all states as init:
arch/x86/kernel/fpu/xstate.c +65 -1
72 lines not on this page · raw message
Signature · 1 line
2.7.4
Borislav Petkov
maintainerRe: [PATCH 11/14] x86/fpu: Split copy_user_to_xstate() into copy_kernel_to_xstate() & copy_user_to_xstate()
Quoted 5 lines · Ingo Molnar
Similar to:
x86/fpu: Split
copy_xstate_to_user()intocopy_xstate_to_kernel()©_xstate_to_user()No change in functionality.
...
Quoted 6 lines
arch/x86/kernel/fpu/xstate.c +0
-0@@ -1083,7 +1083,71 @@ int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i }
arch/x86/kernel/fpu/xstate.c int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i · line 1092
Quoted 10 lines
arch/x86/kernel/fpu/xstate.c +7
-1/* -* 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?
Quoted 18 lines
arch/x86/kernel/fpu/xstate.c +18
-0+{ +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:
arch/x86/kernel/fpu/xstate.c int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i · line 1120
Quoted 10 lines
arch/x86/kernel/fpu/xstate.c +10
-0+*/ + 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))
arch/x86/kernel/fpu/xstate.c int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i · line 1121
Quoted 1 line
arch/x86/kernel/fpu/xstate.c +1
-0+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.
Quoted 1 line
From looking at
fill_xsave()andload_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.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.PATCH 12/14 x86/fpu: Remove 'ubuf' parameter from the copy_kernel_to_xstate() API
In reply to Ingo MolnarNo change in functionality.
--- arch/x86/include/asm/fpu/xstate.h | 2 +- arch/x86/kernel/fpu/regset.c | 2 +- arch/x86/kernel/fpu/xstate.c | 17 +++-------------- 3 files changed, 5 insertions(+), 16 deletions(-)
arch/x86/include/asm/fpu/xstate.h +1 -1
@@ -50,6 +50,6 @@ const void *get_xsave_field_ptr(int xstate_field);int using_compacted_format (void );int copy_xstate_to_kernel (void *kbuf,struct xregs_state *xsave,unsigned int offset,unsigned int size);int copy_xstate_to_user (void __user *ubuf,struct xregs_state *xsave,unsigned int offset,unsigned int size); -int copy_kernel_to_xstate (const void *kbuf,const void __user *ubuf,struct xregs_state *xsave); +int copy_kernel_to_xstate (const void *kbuf,struct xregs_state *xsave);int copy_user_to_xstate (const void *kbuf,const void __user *ubuf,struct xregs_state *xsave);#endif
arch/x86/kernel/fpu/regset.c +1 -1
@@ -135,7 +135,7 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,if (boot_cpu_has (X86_FEATURE_XSAVES )) {if (kbuf) - ret =copy_kernel_to_xstate (kbuf, ubuf, xsave); + ret =copy_kernel_to_xstate (kbuf, xsave);else ret =copy_user_to_xstate (kbuf, ubuf, xsave); }else {
arch/x86/kernel/fpu/xstate.c +3 -14
@@ -1088,8 +1088,7 @@ int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i* 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) +int copy_kernel_to_xstate (const void *kbuf,struct xregs_state *xsave) {unsigned int offset, size;int i;
@@ -1099,12 +1098,7 @@ int copy_kernel_to_xstate(const void *kbuf, const void __user *ubuf, 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 ; - } +memcpy (&xfeatures, kbuf + offset, size);/* * Reject if the user sets any disabled or supervisor features:
@@ -1123,12 +1117,7 @@ int copy_kernel_to_xstate(const void *kbuf, const void __user *ubuf, offset = xstate_offsets[i]; size = xstate_sizes[i]; -if (kbuf) { -memcpy (dst, kbuf + offset, size); - }else { -if (__copy_from_user (dst, ubuf + offset, size)) -return -EFAULT ; - } +memcpy (dst, kbuf + offset, size); } }
Signature · 1 line
2.7.4
PATCH 13/14 x86/fpu: Remove 'kbuf' parameter from the copy_user_to_xstate() API
In reply to Ingo MolnarNo change in functionality.
--- arch/x86/include/asm/fpu/xstate.h | 2 +- arch/x86/kernel/fpu/regset.c | 2 +- arch/x86/kernel/fpu/signal.c | 11 ++++------- arch/x86/kernel/fpu/xstate.c | 19 +++++-------------- 4 files changed, 11 insertions(+), 23 deletions(-)
arch/x86/include/asm/fpu/xstate.h +1 -1
@@ -51,5 +51,5 @@ int using_compacted_format(void);int copy_xstate_to_kernel (void *kbuf,struct xregs_state *xsave,unsigned int offset,unsigned int size);int copy_xstate_to_user (void __user *ubuf,struct xregs_state *xsave,unsigned int offset,unsigned int size);int copy_kernel_to_xstate (const void *kbuf,struct xregs_state *xsave); -int copy_user_to_xstate (const void *kbuf,const void __user *ubuf,struct xregs_state *xsave); +int copy_user_to_xstate (const void __user *ubuf,struct xregs_state *xsave);#endif
arch/x86/kernel/fpu/regset.c +1 -1
@@ -137,7 +137,7 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,if (kbuf) ret =copy_kernel_to_xstate (kbuf, xsave);else - ret =copy_user_to_xstate (kbuf, ubuf, xsave); + ret =copy_user_to_xstate (ubuf, xsave); }else { ret =user_regset_copyin (&pos, &count, &kbuf, &ubuf, xsave,0 ,-1 ); }
arch/x86/kernel/fpu/signal.c +4 -7
@@ -323,13 +323,10 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)*/ fpu__drop (fpu); -if (using_compacted_format ()) { - err =copy_user_to_xstate (NULL , buf_fx, - &fpu->state.xsave); - }else { - err =__copy_from_user (&fpu->state.xsave, - buf_fx, state_size); - } +if (using_compacted_format ()) + err =copy_user_to_xstate (buf_fx, &fpu->state.xsave); +else + err =__copy_from_user (&fpu->state.xsave, buf_fx, state_size);if (err ||__copy_from_user (&env, buf,sizeof (env))) {fpstate_init (&fpu->state);
arch/x86/kernel/fpu/xstate.c +5 -14
@@ -1141,8 +1141,7 @@ int copy_kernel_to_xstate(const void *kbuf, struct xregs_state *xsave)* there we check the CPU has XSAVES and a whole standard-sized buffer * exists. */ -int copy_user_to_xstate (const void *kbuf,const void __user *ubuf, -struct xregs_state *xsave) +int copy_user_to_xstate (const void __user *ubuf,struct xregs_state *xsave) {unsigned int offset, size;int i;
@@ -1152,12 +1151,8 @@ int copy_user_to_xstate(const void *kbuf, const void __user *ubuf, 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 ; - } +if (__copy_from_user (&xfeatures, ubuf + offset, size)) +return -EFAULT ;/* * Reject if the user sets any disabled or supervisor features:
@@ -1176,12 +1171,8 @@ int copy_user_to_xstate(const void *kbuf, const void __user *ubuf, offset = xstate_offsets[i]; size = xstate_sizes[i]; -if (kbuf) { -memcpy (dst, kbuf + offset, size); - }else { -if (__copy_from_user (dst, ubuf + offset, size)) -return -EFAULT ; - } +if (__copy_from_user (dst, ubuf + offset, size)) +return -EFAULT ; } }
Signature · 1 line
2.7.4
PATCH 14/14 x86/fpu: Flip the parameter order in copy_*_to_xstate()
In reply to Ingo MolnarMake it more consistent with regular memcpy() semantics, where the destination argument comes first.
No change in functionality.
--- arch/x86/include/asm/fpu/xstate.h | 4 ++-- arch/x86/kernel/fpu/regset.c | 4 ++-- arch/x86/kernel/fpu/signal.c | 2 +- arch/x86/kernel/fpu/xstate.c | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-)
arch/x86/include/asm/fpu/xstate.h +2 -2
@@ -50,6 +50,6 @@ const void *get_xsave_field_ptr(int xstate_field);int using_compacted_format (void );int copy_xstate_to_kernel (void *kbuf,struct xregs_state *xsave,unsigned int offset,unsigned int size);int copy_xstate_to_user (void __user *ubuf,struct xregs_state *xsave,unsigned int offset,unsigned int size); -int copy_kernel_to_xstate (const void *kbuf,struct xregs_state *xsave); -int copy_user_to_xstate (const void __user *ubuf,struct xregs_state *xsave); +int copy_kernel_to_xstate (struct xregs_state *xsave,const void *kbuf); +int copy_user_to_xstate (struct xregs_state *xsave,const void __user *ubuf);#endif
arch/x86/kernel/fpu/regset.c +2 -2
@@ -135,9 +135,9 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,if (boot_cpu_has (X86_FEATURE_XSAVES )) {if (kbuf) - ret =copy_kernel_to_xstate (kbuf, xsave); + ret =copy_kernel_to_xstate (xsave, kbuf);else - ret =copy_user_to_xstate (ubuf, xsave); + ret =copy_user_to_xstate (xsave, ubuf); }else { ret =user_regset_copyin (&pos, &count, &kbuf, &ubuf, xsave,0 ,-1 ); }
arch/x86/kernel/fpu/signal.c +1 -1
@@ -324,7 +324,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)fpu__drop (fpu);if (using_compacted_format ()) - err =copy_user_to_xstate (buf_fx, &fpu->state.xsave); + err =copy_user_to_xstate (&fpu->state.xsave, buf_fx);else err =__copy_from_user (&fpu->state.xsave, buf_fx, state_size);
arch/x86/kernel/fpu/xstate.c +2 -2
@@ -1088,7 +1088,7 @@ int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i* there we check the CPU has XSAVES and a whole standard-sized buffer * exists. */ -int copy_kernel_to_xstate (const void *kbuf,struct xregs_state *xsave) +int copy_kernel_to_xstate (struct xregs_state *xsave,const void *kbuf) {unsigned int offset, size;int i;
@@ -1141,7 +1141,7 @@ int copy_kernel_to_xstate(const void *kbuf, struct xregs_state *xsave)* there we check the CPU has XSAVES and a whole standard-sized buffer * exists. */ -int copy_user_to_xstate (const void __user *ubuf,struct xregs_state *xsave) +int copy_user_to_xstate (struct xregs_state *xsave,const void __user *ubuf) {unsigned int offset, size;int i;
Signature · 1 line
2.7.4
Quoted 2 lines · Ingo Molnar
This series deobfuscates the ptrace/regset handling functions of the x86 FPU code. As a side effect it (should ...) fix the bug Rik reported in:
Note that this series can also be fetched via Git from:
git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git WIP.x86/fpu
... which is a WIP branch subject to rebase.
Thanks,
Ingo