Re: [PATCH] fs/proc/task_mmu.c v1: fix clear_refs_write() input sanity check

[Amerigo Wang <xiyou.wangcong@gmail.com>]

On Wed, Aug 12, 2009 at 04:29:54PM -0700, Vincent Li wrote:
>v1 fix the compiling errors and keep the type variable name.
>
>Andrew Morton pointed out similar string hacking and obfuscated check for zero-length input
>at the end of the function, David Rientjes suggested to use strict_strtol to replace
>simple_strtol, this patch cover above suggestions, add removing of leading and trailing
>whitespace from user input. It does not change function behavious.
>


Sorry, what real benefits can we get from this patch?

With this patch, we can accept input like:

echo '                    1' > /proc/<pid>/clear_refs

right?

If this is really necessary, why not other files similar to 'clear_refs'?

How about adding strstrip() for simple_strtol()?


>This patch is rebased on mmotm-2009-08-04-14-22.
>
>Signed-off-by: Vincent Li <macli@brc.ubc.ca>
>---
> fs/proc/task_mmu.c |   14 ++++++--------
> 1 files changed, 6 insertions(+), 8 deletions(-)
>
>diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>index 2079969..c7cf8f3 100644
>--- a/fs/proc/task_mmu.c
>+++ b/fs/proc/task_mmu.c
>@@ -491,21 +491,20 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
> 				size_t count, loff_t *ppos)
> {
> 	struct task_struct *task;
>-	char buffer[PROC_NUMBUF], *end;
>+	char buffer[PROC_NUMBUF];
> 	struct mm_struct *mm;
> 	struct vm_area_struct *vma;
>-	int type;
>+	long type;
> 
> 	memset(buffer, 0, sizeof(buffer));
> 	if (count > sizeof(buffer) - 1)
> 		count = sizeof(buffer) - 1;
> 	if (copy_from_user(buffer, buf, count))
> 		return -EFAULT;
>-	type = simple_strtol(buffer, &end, 0);
>+	if (strict_strtol(strstrip(buffer), 10, &type))
>+		return -EINVAL;
> 	if (type < CLEAR_REFS_ALL || type > CLEAR_REFS_MAPPED)
> 		return -EINVAL;
>-	if (*end == '\n')
>-		end++;
> 	task = get_proc_task(file->f_path.dentry->d_inode);
> 	if (!task)
> 		return -ESRCH;
>@@ -541,9 +540,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
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/