[PATCH] fs/proc/: fix input sanity check.

[PATCH] fs/proc/: fix input sanity check.

[Vincent Li]

fix fs/proc/task_mmu.c clear_refs_write(), fs/proc/base.c proc_fault_inject_write()
 and proc_fault_inject_operations() input sanity check by following the disccusion of
http://marc.info/?l=linux-mm&m=124938168905463&w=2.

Signed-off-by: Vincent Li <macli@brc.ubc.ca>
---
 fs/proc/base.c     |   20 ++++++++------------
 fs/proc/task_mmu.c |   11 +++++------
 2 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 04d29a0..44054d2 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1190,17 +1190,16 @@ static ssize_t proc_fault_inject_write(struct file * file,
 		count = sizeof(buffer) - 1;
 	if (copy_from_user(buffer, buf, count))
 		return -EFAULT;
-	make_it_fail = simple_strtol(buffer, &end, 0);
-	if (*end == '\n')
-		end++;
+	make_it_fail = simple_strtol(strstrip(buffer), &end, 0);
+	if (*end)
+		return -EINVAL;
 	task = get_proc_task(file->f_dentry->d_inode);
 	if (!task)
 		return -ESRCH;
 	task->make_it_fail = make_it_fail;
 	put_task_struct(task);
-	if (end - buffer == 0)
-		return -EIO;
-	return end - buffer;
+
+	return count;
 }
 
 static const struct file_operations proc_fault_inject_operations = {
@@ -2253,18 +2252,15 @@ static ssize_t proc_coredump_filter_write(struct file *file,
 		goto out_no_task;
 
 	ret = -EINVAL;
-	val = (unsigned int)simple_strtoul(buffer, &end, 0);
-	if (*end == '\n')
-		end++;
-	if (end - buffer == 0)
-		goto out_no_task;
+	val = (unsigned int)simple_strtoul(strstrip(buffer), &end, 0);
+	if (*end)
+		return ret;
 
 	ret = -ESRCH;
 	task = get_proc_task(file->f_dentry->d_inode);
 	if (!task)
 		goto out_no_task;
 
-	ret = end - buffer;
 	mm = get_task_mm(task);
 	if (!mm)
 		goto out_no_mm;
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 6f61b7c..957b266 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -477,10 +477,10 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
 		count = sizeof(buffer) - 1;
 	if (copy_from_user(buffer, buf, count))
 		return -EFAULT;
-	if (!simple_strtol(buffer, &end, 0))
+	if (!simple_strtol(strstrip(buffer), &end, 0))
+		return -EINVAL;
+	if (*end)
 		return -EINVAL;
-	if (*end == '\n')
-		end++;
 	task = get_proc_task(file->f_path.dentry->d_inode);
 	if (!task)
 		return -ESRCH;
@@ -502,9 +502,8 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
 		mmput(mm);
 	}
 	put_task_struct(task);
-	if (end - buffer == 0)
-		return -EIO;
-	return end - buffer;
+
+	return count;
 }
 
 const struct file_operations proc_clear_refs_operations = {
-- 
1.6.0.4

Re: [PATCH] fs/proc/: fix input sanity check.

[Andrew Morton]

On Tue, 11 Aug 2009 14:34:26 -0700
Vincent Li <macli@brc.ubc.ca> wrote:

> fix fs/proc/task_mmu.c clear_refs_write(), fs/proc/base.c proc_fault_inject_write()
>  and proc_fault_inject_operations() input sanity check by following the disccusion of
> http://marc.info/?l=linux-mm&m=124938168905463&w=2.

Would prefer that the changelog not consist of a link to another patch
which is already quite poorly changelogged, please!

What, precisely was wrong with the old code and how does the patch
change its behaviour.


This matters.  The patch potentially changes the kernel<->userspace
interface and we must fully understand the nature of that change so as
to minimise the risk of breaking existing userspace application code.

Thanks.

Re: [PATCH] fs/proc/: fix input sanity check.

[Vincent Li]

On Tue, 11 Aug 2009, Andrew Morton wrote:

> On Tue, 11 Aug 2009 14:34:26 -0700
> Vincent Li <macli@brc.ubc.ca> wrote:
> 
> > fix fs/proc/task_mmu.c clear_refs_write(), fs/proc/base.c proc_fault_inject_write()
> >  and proc_fault_inject_operations() input sanity check by following the disccusion of
> > http://marc.info/?l=linux-mm&m=124938168905463&w=2.
> 
> Would prefer that the changelog not consist of a link to another patch
> which is already quite poorly changelogged, please!

It make sense to me only after you point it out to me, That is process of 
learning :)

> 
> What, precisely was wrong with the old code and how does the patch
> change its behaviour.
>

It is after I read your comment about the strange handling of userspace 
string input http://marc.info/?l=linux-mm&m=124890923527186&w=2 and 
Kosaki's "fix oom_adjust_write() input sanity check" patch to match your 
comment http://marc.info/?l=linux-mm&m=124938168905463&w=2.

I re-read the clear_refs_write() in fs/proc/task_mmu.c, If I understand it 
correctly, The old code takes  non-zero string 
from userspace and convert it to unsigned long interger value. for 
example,It accepts:

echo 1 > /proc/pid/clear_refs
echo 12foo > /proc/pid/clear_refs

The new code add the stripping of leading and trailing whitespace, remove 
the obfuscated check for zero-length input at the end of the function as 
you commented. Will this new code break the existing application 
behaviour? David Rientjes is the author of clear_refs, I cced him to see 
if he has any comment.
     
I have not read the proc_fault_inject_write() and 
proc_fault_inject_operations() yet, I will drop the changes to those two 
functions

> 
> This matters.  The patch potentially changes the kernel<->userspace
> interface and we must fully understand the nature of that change so as
> to minimise the risk of breaking existing userspace application code.
> 

Point taken, I am in a rush to post the patch without fully understanding 
what the functions do and the impact of patch changes.

> Thanks.

You are welcome.

Vincent Li
Biomedical Research Center
University of British Columbia

Re: [PATCH] fs/proc/: fix input sanity check.

[David Rientjes]

On Wed, 12 Aug 2009, Vincent Li wrote:

> I re-read the clear_refs_write() in fs/proc/task_mmu.c, If I understand it 
> correctly, The old code takes  non-zero string 
> from userspace and convert it to unsigned long interger value. for 

strict_strtol() converts to long.

> example,It accepts:
> 
> echo 1 > /proc/pid/clear_refs
> echo 12foo > /proc/pid/clear_refs
> 

http://userweb.kernel.org/~akpm/mmotm/broken-out/pagemap-clear_refs-modify-to-specify-anon-or-mapped-vma-clearing.patch
changes that, so you'll need to rebase on mmotm.