[PATCH] Fix seq_read dead loop and trigger memory allocation failure.

[PATCH] Fix seq_read dead loop and trigger memory allocation failure.

[Fengwei Yin]

When dump /proc/xxx/maps, if d_path return error in seq_path, the
buffer will be exhaust and trigger dead loop in seq_read. Till
kmalloc fails with -ENOMEM.

Saving and restoring the m->count to avoid the dead loop in seq_read
if d_path return error.

Signed-off-by: Fengwei Yin <yfw.kernel@gmail.com>
---
 fs/proc/task_mmu.c   | 10 +++++++++-
 fs/proc/task_nommu.c | 10 +++++++++-
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 442177b..a080531 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -295,8 +295,16 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
 	 * special [heap] marker for the heap:
 	 */
 	if (file) {
+		size_t sz;
 		seq_pad(m, ' ');
-		seq_path(m, &file->f_path, "\n");
+		/* Save current count. Once seq_path return negtive value,
+		 * we need to restore saved count. Otherwise, seq_path will
+		 * exhaust the buffer and make seq_read dead loop till
+		 * m->buff allocation failure.
+		 */
+		sz = m->count;
+		if (seq_path(m, &file->f_path, "\n") < 0)
+			m->count = sz;
 		goto done;
 	}
 
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 678455d..0d4d6e0 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -160,8 +160,16 @@ static int nommu_vma_show(struct seq_file *m, struct vm_area_struct *vma,
 		   MAJOR(dev), MINOR(dev), ino);
 
 	if (file) {
+		size_t sz;
 		seq_pad(m, ' ');
-		seq_path(m, &file->f_path, "");
+		/* Save current count. Once seq_path return negtive value,
+		 * we need to restore saved count. Otherwise, seq_path will
+		 * exhaust the buffer and make seq_read dead loop till
+		 * m->buff allocation failure.
+		 */
+		sz = m->count;
+		if (seq_path(m, &file->f_path, "\n") < 0)
+			m->count = sz;
 	} else if (mm) {
 		pid_t tid = vm_is_stack(priv->task, vma, is_pid);
 
-- 
1.8.3.2

Re: [PATCH] Fix seq_read dead loop and trigger memory allocation failure.

[Al Viro]

On Mon, Apr 21, 2014 at 10:12:42PM +0800, Fengwei Yin wrote:
> When dump /proc/xxx/maps, if d_path return error in seq_path, the
> buffer will be exhaust and trigger dead loop in seq_read. Till
> kmalloc fails with -ENOMEM.

*WHAT* d_path error?  -ENAMETOOLONG, aka. "you've got too little space"?

> @@ -295,8 +295,16 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
>  	 * special [heap] marker for the heap:
>  	 */
>  	if (file) {
> +		size_t sz;
>  		seq_pad(m, ' ');
> -		seq_path(m, &file->f_path, "\n");
> +		/* Save current count. Once seq_path return negtive value,
> +		 * we need to restore saved count. Otherwise, seq_path will
> +		 * exhaust the buffer and make seq_read dead loop till
> +		 * m->buff allocation failure.
> +		 */
> +		sz = m->count;
> +		if (seq_path(m, &file->f_path, "\n") < 0)
> +			m->count = sz;

NAK.  No way in hell.  Any code playing with m->count that way is broken.
Post the reproducer for that infinite loop; then we'll be able to see
what triggers an impossible error from d_path().  _That_ is where the bug
is, assuming it exists at all.

Re: [PATCH] Fix seq_read dead loop and trigger memory allocation failure.

[Fengwei Yin]

On Thu, Apr 24, 2014 at 5:58 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, Apr 21, 2014 at 10:12:42PM +0800, Fengwei Yin wrote:
>> When dump /proc/xxx/maps, if d_path return error in seq_path, the
>> buffer will be exhaust and trigger dead loop in seq_read. Till
>> kmalloc fails with -ENOMEM.
>
> *WHAT* d_path error?  -ENAMETOOLONG, aka. "you've got too little space"?
>
I could check it and get you back. But I suppose it's not this one
because it still  fails even I have buffer with 4M size.

>> @@ -295,8 +295,16 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
>>        * special [heap] marker for the heap:
>>        */
>>       if (file) {
>> +             size_t sz;
>>               seq_pad(m, ' ');
>> -             seq_path(m, &file->f_path, "\n");
>> +             /* Save current count. Once seq_path return negtive value,
>> +              * we need to restore saved count. Otherwise, seq_path will
>> +              * exhaust the buffer and make seq_read dead loop till
>> +              * m->buff allocation failure.
>> +              */
>> +             sz = m->count;
>> +             if (seq_path(m, &file->f_path, "\n") < 0)
>> +                     m->count = sz;
>
> NAK.  No way in hell.  Any code playing with m->count that way is broken.
> Post the reproducer for that infinite loop; then we'll be able to see
> what triggers an impossible error from d_path().  _That_ is where the bug
> is, assuming it exists at all.

Thanks a lot for checking this.
When I play the Android x86_64 emulator (with 64bit kernel) and cat
the /proc/xxxx/maps (xxxx is a 32bit process id), seq_read return
-ENOMEM.

I tried to reproduce the same issue on a native environment. But
couldn't reproduce it.

I will collect more info and post here.

Regards
Yin, Fengwei

Re: [PATCH] Fix seq_read dead loop and trigger memory allocation failure.

[Al Viro]

On Thu, Apr 24, 2014 at 10:26:50PM +0800, Fengwei Yin wrote:
> On Thu, Apr 24, 2014 at 5:58 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Mon, Apr 21, 2014 at 10:12:42PM +0800, Fengwei Yin wrote:
> >> When dump /proc/xxx/maps, if d_path return error in seq_path, the
> >> buffer will be exhaust and trigger dead loop in seq_read. Till
> >> kmalloc fails with -ENOMEM.
> >
> > *WHAT* d_path error?  -ENAMETOOLONG, aka. "you've got too little space"?
> >
> I could check it and get you back. But I suppose it's not this one
> because it still  fails even I have buffer with 4M size.

Please, do.  One thing to watch out for is bogus ->d_dname() return
value; instances of ->d_dname() are only allowed to return valid pointers or
ERR_PTR(-ENAMETOOLONG), the latter - only if the buffer really is too short
(i.e. disappearing on sufficiently large one).  That holds in mainline, but
that's the most likely vector by which the breakage could be introduced in
some out-of-tree code.

Here's the braindump on that bunch (basically, everything in fs/dcache.c
from prepend() to dentry_path()):
	* their char * arguments are never ERR_PTR(...)
	* their char ** arguments never point to ERR_PTR(...) - not on entry
to function and not on return from it, regardless of return value.
	* prepend(), prepend_name() and prepend_unreachable() always return
either 0 or -ENAMETOOLONG.
	* return value of prepend_path() and path_with_deleted() can only be
0, 1, 2 or -ENAMETOOLONG.
	* __d_path() returns NULL, a pointer to string or
ERR_PTR(-ENAMETOOLONG).
	* d_absolute_path() returns a pointer to string, ERR_PTR(-ENAMETOOLONG)
or ERR_PTR(-EINVAL), the last one being for the case when its path argument
points into an unmounted vfsmount.
	* d_path(), __dentry_path(), dentry_path_raw(), dentry_path() and
->d_dname() instances return a pointer to string or ERR_PTR(-ENAMETOOLONG).
	* all in-tree instances of ->d_dname() are either simple_dname() or
trivial wrappers for dynamic_dname(), so for mainline it's enough to check
those two helpers; out-of-tree code providing ->d_dname() instances needs
to be checked, of course.
	* given sufficiently large buffer ->d_dname() should succeed.
Persistent ERR_PTR(-ENAMETOOLONG) from it is a bug.  Note that use of
dynamic_dname() with format that might exceed 64 characters of output
is wrong; that's the reason why e.g. mm/shmem.c uses simple_dname() instead.
AFAICS, all remaining callers of dynamic_dname() in mainline are guaranteed
to stay within its limitations (either <short string>[<unsigned long decimal>]
or anon_inode:<short string passed to anon_inode_get{file,fd}>).  Out-of-tree
code needs to be checked, of course.

Re: [PATCH] Fix seq_read dead loop and trigger memory allocation failure.

[Fengwei Yin]

On Fri, Apr 25, 2014 at 12:29 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Apr 24, 2014 at 10:26:50PM +0800, Fengwei Yin wrote:
>> On Thu, Apr 24, 2014 at 5:58 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> > On Mon, Apr 21, 2014 at 10:12:42PM +0800, Fengwei Yin wrote:
>> >> When dump /proc/xxx/maps, if d_path return error in seq_path, the
>> >> buffer will be exhaust and trigger dead loop in seq_read. Till
>> >> kmalloc fails with -ENOMEM.
>> >
>> > *WHAT* d_path error?  -ENAMETOOLONG, aka. "you've got too little space"?
>> >
>> I could check it and get you back. But I suppose it's not this one
>> because it still  fails even I have buffer with 4M size.
>
> Please, do.  One thing to watch out for is bogus ->d_dname() return
> value; instances of ->d_dname() are only allowed to return valid pointers or
> ERR_PTR(-ENAMETOOLONG), the latter - only if the buffer really is too short
> (i.e. disappearing on sufficiently large one).  That holds in mainline, but
> that's the most likely vector by which the breakage could be introduced in
> some out-of-tree code.
>
> Here's the braindump on that bunch (basically, everything in fs/dcache.c
> from prepend() to dentry_path()):
>         * their char * arguments are never ERR_PTR(...)
>         * their char ** arguments never point to ERR_PTR(...) - not on entry
> to function and not on return from it, regardless of return value.
>         * prepend(), prepend_name() and prepend_unreachable() always return
> either 0 or -ENAMETOOLONG.
>         * return value of prepend_path() and path_with_deleted() can only be
> 0, 1, 2 or -ENAMETOOLONG.
>         * __d_path() returns NULL, a pointer to string or
> ERR_PTR(-ENAMETOOLONG).
>         * d_absolute_path() returns a pointer to string, ERR_PTR(-ENAMETOOLONG)
> or ERR_PTR(-EINVAL), the last one being for the case when its path argument
> points into an unmounted vfsmount.
>         * d_path(), __dentry_path(), dentry_path_raw(), dentry_path() and
> ->d_dname() instances return a pointer to string or ERR_PTR(-ENAMETOOLONG).
>         * all in-tree instances of ->d_dname() are either simple_dname() or
> trivial wrappers for dynamic_dname(), so for mainline it's enough to check
> those two helpers; out-of-tree code providing ->d_dname() instances needs
> to be checked, of course.
>         * given sufficiently large buffer ->d_dname() should succeed.
> Persistent ERR_PTR(-ENAMETOOLONG) from it is a bug.  Note that use of
> dynamic_dname() with format that might exceed 64 characters of output
> is wrong; that's the reason why e.g. mm/shmem.c uses simple_dname() instead.
> AFAICS, all remaining callers of dynamic_dname() in mainline are guaranteed
> to stay within its limitations (either <short string>[<unsigned long decimal>]
> or anon_inode:<short string passed to anon_inode_get{file,fd}>).  Out-of-tree
> code needs to be checked, of course.

Cool. I just found that you already fixed the issue (long name in
ashmem) by change
to simple_dname from dynamic_dname on tip. Android still stick to old
kernel and hit
this issue.

Thanks a lot.