* [PATCH] fs/proc/task_mmu.c v1: fix clear_refs_write() input sanity check @ 2009-08-12 23:29 Vincent Li 2009-08-13 2:09 ` Amerigo Wang 2009-08-13 8:36 ` David Rientjes 0 siblings, 2 replies; 6+ messages in thread From: Vincent Li @ 2009-08-12 23:29 UTC (permalink / raw) To: linux-kernel; +Cc: Andrew Morton, David Rientjes, Vincent Li 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. 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs/proc/task_mmu.c v1: fix clear_refs_write() input sanity check 2009-08-12 23:29 [PATCH] fs/proc/task_mmu.c v1: fix clear_refs_write() input sanity check Vincent Li @ 2009-08-13 2:09 ` Amerigo Wang 2009-08-13 5:20 ` Vincent Li 2009-08-13 8:36 ` David Rientjes 1 sibling, 1 reply; 6+ messages in thread From: Amerigo Wang @ 2009-08-13 2:09 UTC (permalink / raw) To: Vincent Li; +Cc: linux-kernel, Andrew Morton, David Rientjes 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/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs/proc/task_mmu.c v1: fix clear_refs_write() input sanity check 2009-08-13 2:09 ` Amerigo Wang @ 2009-08-13 5:20 ` Vincent Li 2009-08-13 8:45 ` Amerigo Wang 0 siblings, 1 reply; 6+ messages in thread From: Vincent Li @ 2009-08-13 5:20 UTC (permalink / raw) To: Amerigo Wang; +Cc: linux-kernel, Andrew Morton, David Rientjes On Thu, 13 Aug 2009, Amerigo Wang wrote: > 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? It removes the redundant and weird zero-length check at the end of the function since the the function already check the returning value of simple_strtol, if it is zero-length input, the function should already return error -EINVAL. Also using strict_strtol would only accept ascii numbers, not something like: echo 1foo > /proc/pid/clear_refs > > With this patch, we can accept input like: > > echo ' 1' > /proc/<pid>/clear_refs > > right? It will accept 'echo 1 > /proc/pid/clear_refs', but I think nobody will do that on purpose, only accidently typed one or two space before or after. > > If this is really necessary, why not other files similar to 'clear_refs'? > I did the same to other functions, but Andrew pointed out making changes without fully understanding the patch impact would be risky and break existing applications, So I need to fully understand what those functions do before I make any changes. > How about adding strstrip() for simple_strtol()? > simple_strtol will accept 'echo 1foo > /proc/pid/clear_refs' which is not ideal. > > >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 > > Vincent Li Biomedical Research Center University of British Columbia ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs/proc/task_mmu.c v1: fix clear_refs_write() input sanity check 2009-08-13 5:20 ` Vincent Li @ 2009-08-13 8:45 ` Amerigo Wang 2009-08-13 20:12 ` David Rientjes 0 siblings, 1 reply; 6+ messages in thread From: Amerigo Wang @ 2009-08-13 8:45 UTC (permalink / raw) To: Vincent Li; +Cc: Amerigo Wang, linux-kernel, Andrew Morton, David Rientjes On Wed, Aug 12, 2009 at 10:20:28PM -0700, Vincent Li wrote: >On Thu, 13 Aug 2009, Amerigo Wang wrote: > >> 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? > >It removes the redundant and weird zero-length check at the end of the >function since the the function already check the returning value of >simple_strtol, if it is zero-length input, the function should already >return error -EINVAL. Also using strict_strtol would only accept ascii >numbers, not something like: >>echo 1foo > /proc/pid/clear_refs > IIRC, many /proc files can accept input like this... I sent a patch some time ago to "fix" this, but Andrew pointed out that things like "1foo" can be hackish. :-/ > >> >> If this is really necessary, why not other files similar to 'clear_refs'? >> > >I did the same to other functions, but Andrew pointed out making changes >without fully understanding the patch impact would be risky and break >existing applications, So I need to fully understand what those functions >do before I make any changes. > I can't find any strong reason why this one is differernt from others in accepting input like "1foo"... ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs/proc/task_mmu.c v1: fix clear_refs_write() input sanity check 2009-08-13 8:45 ` Amerigo Wang @ 2009-08-13 20:12 ` David Rientjes 0 siblings, 0 replies; 6+ messages in thread From: David Rientjes @ 2009-08-13 20:12 UTC (permalink / raw) To: Amerigo Wang; +Cc: Vincent Li, linux-kernel, Andrew Morton On Thu, 13 Aug 2009, Amerigo Wang wrote: > >I did the same to other functions, but Andrew pointed out making changes > >without fully understanding the patch impact would be risky and break > >existing applications, So I need to fully understand what those functions > >do before I make any changes. > > > > > I can't find any strong reason why this one is differernt from others > in accepting input like "1foo"... > As I already pointed out, there's a patch pending in mmotm that limits the write to either 1, 2, or 3, depending on the type of pages the referenced bit shall be cleared for associated with the task. The net effect of Vincent's change is to automatically strip whitespace and to remove a needless -EIO error condition at the end of the write handler. It is rather obvious that Vincent's patch is a clear improvement and your concern is just a waste of bandwidth. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs/proc/task_mmu.c v1: fix clear_refs_write() input sanity check 2009-08-12 23:29 [PATCH] fs/proc/task_mmu.c v1: fix clear_refs_write() input sanity check Vincent Li 2009-08-13 2:09 ` Amerigo Wang @ 2009-08-13 8:36 ` David Rientjes 1 sibling, 0 replies; 6+ messages in thread From: David Rientjes @ 2009-08-13 8:36 UTC (permalink / raw) To: Vincent Li; +Cc: linux-kernel, Andrew Morton On Wed, 12 Aug 2009, 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. > > This patch is rebased on mmotm-2009-08-04-14-22. > > Signed-off-by: Vincent Li <macli@brc.ubc.ca> Acked-by: David Rientjes <rientjes@google.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-08-13 20:12 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-08-12 23:29 [PATCH] fs/proc/task_mmu.c v1: fix clear_refs_write() input sanity check Vincent Li 2009-08-13 2:09 ` Amerigo Wang 2009-08-13 5:20 ` Vincent Li 2009-08-13 8:45 ` Amerigo Wang 2009-08-13 20:12 ` David Rientjes 2009-08-13 8:36 ` David Rientjes
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®