Re: [RFC] fs/kcore: change copy_to_user to copy_in_user
On Thu, Aug 20, 2015 at 1:59 AM, yalin wang wrote: > - > - n = copy_to_user(buffer, (char *)start, tsz); > + if ((start + tsz < tsz) || > + (start + tsz) > TASK_SIZE) > + return -EFAULT; This is wrong. You apparently want to have if (!access_ok(start, tsz)) return -EFAULT; > + set_fs(KERNEL_DS); > + n = copy_in_user(buffer, (char *)start, tsz); > + set_fs(USER_DS); .. and this is actually worse and even less portable than what we have now, in that it's actively wrong on platforms that may have a user address and a kernel address with the same value (ie they have explicitly separate kernel/user address spaces). Now, that's admittedly unusual, but I think sparc32 actually can do that. Anyway, I absolutely detest this patch. It replaces one piece of code that admittedly doesn't work on all architectures because kernel memory is accessed without testing, with another hack that happens to work on other architectures and is fragile and prone to be a security issue. In other words, I think the end result is _worse_ than the current situation. You probably want to use "probe_kernel_read()" and do it into a temporary buffer, and then just do the copy_to_user() from the temporary buffer. Sure, it's less efficient, but at least it's not actively wrong and a possible security problem in the long run. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] fs/kcore: change copy_to_user to copy_in_user
> On Aug 20, 2015, at 17:58, Frans Klaver wrote: > > On Thu, Aug 20, 2015 at 10:59 AM, yalin wang wrote: >> the copy_to_user() here expect can fix the fault on both kernel and >> user address, this is not true on other platforms except x86, >> change to user copy_in_user() so that can detect the page fault, >> work as expected. > > Could you rephrase this into multiple sentences in comprehensible > English? What is the expected behavior, what is the unexpected > behavior and what can people do to trigger it? > ok, i will send a V2 patch. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] fs/kcore: change copy_to_user to copy_in_user
On Thu, Aug 20, 2015 at 10:59 AM, yalin wang wrote:
> the copy_to_user() here expect can fix the fault on both kernel and
> user address, this is not true on other platforms except x86,
> change to user copy_in_user() so that can detect the page fault,
> work as expected.
Could you rephrase this into multiple sentences in comprehensible
English? What is the expected behavior, what is the unexpected
behavior and what can people do to trigger it?
> Signed-off-by: yalin wang
> ---
> fs/proc/kcore.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
> index 92e6726..4f28deb 100644
> --- a/fs/proc/kcore.c
> +++ b/fs/proc/kcore.c
> @@ -515,8 +515,12 @@ read_kcore(struct file *file, char __user *buffer,
> size_t buflen, loff_t *fpos)
> } else {
> if (kern_addr_valid(start)) {
> unsigned long n;
> -
> - n = copy_to_user(buffer, (char *)start, tsz);
> + if ((start + tsz < tsz) ||
> + (start + tsz) > TASK_SIZE)
> + return -EFAULT;
> + set_fs(KERNEL_DS);
> + n = copy_in_user(buffer, (char *)start, tsz);
> + set_fs(USER_DS);
> /*
> * We cannot distinguish between fault on
> source
> * and fault on destination. When this happens
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC] fs/kcore: change copy_to_user to copy_in_user
the copy_to_user() here expect can fix the fault on both kernel and
user address, this is not true on other platforms except x86,
change to user copy_in_user() so that can detect the page fault,
work as expected.
Signed-off-by: yalin wang
---
fs/proc/kcore.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 92e6726..4f28deb 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -515,8 +515,12 @@ read_kcore(struct file *file, char __user *buffer, size_t
buflen, loff_t *fpos)
} else {
if (kern_addr_valid(start)) {
unsigned long n;
-
- n = copy_to_user(buffer, (char *)start, tsz);
+ if ((start + tsz < tsz) ||
+ (start + tsz) > TASK_SIZE)
+ return -EFAULT;
+ set_fs(KERNEL_DS);
+ n = copy_in_user(buffer, (char *)start, tsz);
+ set_fs(USER_DS);
/*
* We cannot distinguish between fault on source
* and fault on destination. When this happens
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] fs/kcore: change copy_to_user to copy_in_user
On Thu, Aug 20, 2015 at 10:59 AM, yalin wang yalin.wang2...@gmail.com wrote:
the copy_to_user() here expect can fix the fault on both kernel and
user address, this is not true on other platforms except x86,
change to user copy_in_user() so that can detect the page fault,
work as expected.
Could you rephrase this into multiple sentences in comprehensible
English? What is the expected behavior, what is the unexpected
behavior and what can people do to trigger it?
Signed-off-by: yalin wang yalin.wang2...@gmail.com
---
fs/proc/kcore.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 92e6726..4f28deb 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -515,8 +515,12 @@ read_kcore(struct file *file, char __user *buffer,
size_t buflen, loff_t *fpos)
} else {
if (kern_addr_valid(start)) {
unsigned long n;
-
- n = copy_to_user(buffer, (char *)start, tsz);
+ if ((start + tsz tsz) ||
+ (start + tsz) TASK_SIZE)
+ return -EFAULT;
+ set_fs(KERNEL_DS);
+ n = copy_in_user(buffer, (char *)start, tsz);
+ set_fs(USER_DS);
/*
* We cannot distinguish between fault on
source
* and fault on destination. When this happens
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC] fs/kcore: change copy_to_user to copy_in_user
the copy_to_user() here expect can fix the fault on both kernel and
user address, this is not true on other platforms except x86,
change to user copy_in_user() so that can detect the page fault,
work as expected.
Signed-off-by: yalin wang yalin.wang2...@gmail.com
---
fs/proc/kcore.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 92e6726..4f28deb 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -515,8 +515,12 @@ read_kcore(struct file *file, char __user *buffer, size_t
buflen, loff_t *fpos)
} else {
if (kern_addr_valid(start)) {
unsigned long n;
-
- n = copy_to_user(buffer, (char *)start, tsz);
+ if ((start + tsz tsz) ||
+ (start + tsz) TASK_SIZE)
+ return -EFAULT;
+ set_fs(KERNEL_DS);
+ n = copy_in_user(buffer, (char *)start, tsz);
+ set_fs(USER_DS);
/*
* We cannot distinguish between fault on source
* and fault on destination. When this happens
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] fs/kcore: change copy_to_user to copy_in_user
On Aug 20, 2015, at 17:58, Frans Klaver franskla...@gmail.com wrote: On Thu, Aug 20, 2015 at 10:59 AM, yalin wang yalin.wang2...@gmail.com wrote: the copy_to_user() here expect can fix the fault on both kernel and user address, this is not true on other platforms except x86, change to user copy_in_user() so that can detect the page fault, work as expected. Could you rephrase this into multiple sentences in comprehensible English? What is the expected behavior, what is the unexpected behavior and what can people do to trigger it? ok, i will send a V2 patch. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] fs/kcore: change copy_to_user to copy_in_user
On Thu, Aug 20, 2015 at 1:59 AM, yalin wang yalin.wang2...@gmail.com wrote: - - n = copy_to_user(buffer, (char *)start, tsz); + if ((start + tsz tsz) || + (start + tsz) TASK_SIZE) + return -EFAULT; This is wrong. You apparently want to have if (!access_ok(start, tsz)) return -EFAULT; + set_fs(KERNEL_DS); + n = copy_in_user(buffer, (char *)start, tsz); + set_fs(USER_DS); .. and this is actually worse and even less portable than what we have now, in that it's actively wrong on platforms that may have a user address and a kernel address with the same value (ie they have explicitly separate kernel/user address spaces). Now, that's admittedly unusual, but I think sparc32 actually can do that. Anyway, I absolutely detest this patch. It replaces one piece of code that admittedly doesn't work on all architectures because kernel memory is accessed without testing, with another hack that happens to work on other architectures and is fragile and prone to be a security issue. In other words, I think the end result is _worse_ than the current situation. You probably want to use probe_kernel_read() and do it into a temporary buffer, and then just do the copy_to_user() from the temporary buffer. Sure, it's less efficient, but at least it's not actively wrong and a possible security problem in the long run. Linus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
