Re: [PATCH 09/14] x86/fpu: Change 'size_total' parameter to unsigned and standardize the size checks in copy_xstate_to_*()
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
> Cc: Andy Lutomirski
> Cc: Borislav Petkov
> Cc: Dave Hansen
> Cc: Fenghua Yu
> Cc: H. Peter Anvin
> Cc: Linus Torvalds
> Cc: Oleg Nesterov
> Cc: Thomas Gleixner
> Cc: Yu-cheng Yu
> Cc: Fenghua Yu
> Signed-off-by: Ingo Molnar
> ---
> 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
Re: [PATCH 09/14] x86/fpu: Change 'size_total' parameter to unsigned and standardize the size checks in copy_xstate_to_*()
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
> Cc: Andy Lutomirski
> Cc: Borislav Petkov
> Cc: Dave Hansen
> Cc: Fenghua Yu
> Cc: H. Peter Anvin
> Cc: Linus Torvalds
> Cc: Oleg Nesterov
> Cc: Thomas Gleixner
> Cc: Yu-cheng Yu
> Cc: Fenghua Yu
> Signed-off-by: Ingo Molnar
> ---
> 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
[PATCH 09/14] x86/fpu: Change 'size_total' parameter to unsigned and standardize the size checks in copy_xstate_to_*()
'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
Cc: Andy Lutomirski
Cc: Borislav Petkov
Cc: Dave Hansen
Cc: Fenghua Yu
Cc: H. Peter Anvin
Cc: Linus Torvalds
Cc: Oleg Nesterov
Cc: Thomas Gleixner
Cc: Yu-cheng Yu
Cc: Fenghua Yu
Signed-off-by: Ingo Molnar
---
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;
+
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;
}
}
--
2.7.4
[PATCH 09/14] x86/fpu: Change 'size_total' parameter to unsigned and standardize the size checks in copy_xstate_to_*()
'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
Cc: Andy Lutomirski
Cc: Borislav Petkov
Cc: Dave Hansen
Cc: Fenghua Yu
Cc: H. Peter Anvin
Cc: Linus Torvalds
Cc: Oleg Nesterov
Cc: Thomas Gleixner
Cc: Yu-cheng Yu
Cc: Fenghua Yu
Signed-off-by: Ingo Molnar
---
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;
+
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;
}
}
--
2.7.4
