[PATCH] Fix sys_move_pages when a NULL node list is passed.

[PATCH] Fix sys_move_pages when a NULL node list is passed.

[Stephen Rothwell]

sys_move_pages() uses vmalloc() to allocate an array of structures
that is fills with information passed from user mode and then passes to
do_stat_pages() (in the case the node list is NULL).  do_stat_pages()
depends on a marker in the node field of the structure to decide how large
the array is and this marker is correctly inserted into the last element
of the array.  However, vmalloc() doesn't zero the memory it allocates
and if the user passes NULL for the node list, then the node fields are
not filled in (except for the end marker).  If the memory the vmalloc()
returned happend to have a word with the marker value in it in just the
right place, do_pages_stat will fail to fill the status field of part
of the array and we will return (random) kernel data to user mode.

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 mm/migrate.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

This has been tested in PowerPC (after wiring up sys_move_pages).  This
should go into 2.6.19 as it leaks kernel memory.  It should also be
submitted to the 2.6.18 stable tree (as sys_move_pages was introduced
before 2.6.18-rc2).

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

diff --git a/mm/migrate.c b/mm/migrate.c
index ba2453f..b4979d4 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -952,7 +952,8 @@ asmlinkage long sys_move_pages(pid_t pid
 				goto out;
 
 			pm[i].node = node;
-		}
+		} else
+			pm[i].node = 0;	/* anything to not match MAX_NUMNODES */
 	}
 	/* End marker */
 	pm[nr_pages].node = MAX_NUMNODES;
-- 
1.4.3.2

Re: [PATCH] Fix sys_move_pages when a NULL node list is passed.

[Christoph Lameter]

On Fri, 3 Nov 2006, Stephen Rothwell wrote:

> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>

Acked-by: Christoph Lameter <clameter@sgi.com>

patch fix-sys_move_pages-when-a-null-node-list-is-passed.patch queued to -stable tree

[chrisw]

This is a note to let you know that we have just queued up the patch titled

     Subject: Fix sys_move_pages when a NULL node list is passed.

to the 2.6.18-stable tree.  Its filename is

     fix-sys_move_pages-when-a-null-node-list-is-passed.patch

A git repo of this tree can be found at 
    http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary


>From stable-bounces@linux.kernel.org  Thu Nov  2 19:46:17 2006
Date: Fri, 3 Nov 2006 14:42:43 +1100
From: Stephen Rothwell <sfr@canb.auug.org.au>
To: Christoph Lameter <clameter@sgi.com>
Message-Id: <20061103144243.4601ba76.sfr@canb.auug.org.au>
Cc: Andrew Morton <akpm@osdl.org>, linux-mm@kvack.org, LKML <linux-kernel@vger.kernel.org>, stable@kernel.org
Subject: Fix sys_move_pages when a NULL node list is passed.

sys_move_pages() uses vmalloc() to allocate an array of structures
that is fills with information passed from user mode and then passes to
do_stat_pages() (in the case the node list is NULL).  do_stat_pages()
depends on a marker in the node field of the structure to decide how large
the array is and this marker is correctly inserted into the last element
of the array.  However, vmalloc() doesn't zero the memory it allocates
and if the user passes NULL for the node list, then the node fields are
not filled in (except for the end marker).  If the memory the vmalloc()
returned happend to have a word with the marker value in it in just the
right place, do_pages_stat will fail to fill the status field of part
of the array and we will return (random) kernel data to user mode.

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
Acked-by: Christoph Lameter <clameter@sgi.com>
Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---
 mm/migrate.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- linux-2.6.18.2.orig/mm/migrate.c
+++ linux-2.6.18.2/mm/migrate.c
@@ -950,7 +950,8 @@ asmlinkage long sys_move_pages(pid_t pid
 				goto out;
 
 			pm[i].node = node;
-		}
+		} else
+			pm[i].node = 0;	/* anything to not match MAX_NUMNODES */
 	}
 	/* End marker */
 	pm[nr_pages].node = MAX_NUMNODES;


Patches currently in stable-queue which might be from sfr@canb.auug.org.au are

queue-2.6.18/fix-sys_move_pages-when-a-null-node-list-is-passed.patch

Re: [PATCH] Fix sys_move_pages when a NULL node list is passed.

[KAMEZAWA Hiroyuki]

On Fri, 3 Nov 2006 14:42:43 +1100
Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> +		} else
> +			pm[i].node = 0;	/* anything to not match MAX_NUMNODES */
>  	}
>  	/* End marker */
>  	pm[nr_pages].node = MAX_NUMNODES;

I think node0 is always online...but this should be

pm[i].node = first_online_node; // /* any online node */

maybe.

-Kame

Re: [PATCH] Fix sys_move_pages when a NULL node list is passed.

[Christoph Lameter]

On Wed, 8 Nov 2006, KAMEZAWA Hiroyuki wrote:

> >  	pm[nr_pages].node = MAX_NUMNODES;
> 
> I think node0 is always online...but this should be
> 
> pm[i].node = first_online_node; // /* any online node */

No it is a marker. The use of any node that is online could lead to a 
false determination of the endpoint of the list.

Re: [PATCH] Fix sys_move_pages when a NULL node list is passed.

[KAMEZAWA Hiroyuki]

On Tue, 7 Nov 2006 18:01:11 -0800 (PST)
Christoph Lameter <clameter@sgi.com> wrote:

> On Wed, 8 Nov 2006, KAMEZAWA Hiroyuki wrote:
> 
> > >  	pm[nr_pages].node = MAX_NUMNODES;
> > 
> > I think node0 is always online...but this should be
> > 
> > pm[i].node = first_online_node; // /* any online node */
> 
> No it is a marker. The use of any node that is online could lead to a 
> false determination of the endpoint of the list.
> 
Ah.. I'm mentioning to this.
==
+			pm[i].node = 0;	/* anything to not match MAX_NUMNODES */
==
Sorry for my bad cut & paste.

It seems that this 0 will be passed to alloc_pages_node().
alloc_pages_node() doesn't check whether a node is online or not before using 
NODE_DATA().

-Kame

Re: [PATCH] Fix sys_move_pages when a NULL node list is passed.

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

On Wed, 8 Nov 2006 11:13:41 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
>
> Ah.. I'm mentioning to this.
> ==
> +			pm[i].node = 0;	/* anything to not match MAX_NUMNODES */
> ==
> Sorry for my bad cut & paste.
>
> It seems that this 0 will be passed to alloc_pages_node().
> alloc_pages_node() doesn't check whether a node is online or not before using
> NODE_DATA().

Actually, it won't.  If you do that assignment, then the nodes parameter
was NULL and you will only call do_pages_stat() and so never call
alloc_pages_node().

--
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Fix sys_move_pages when a NULL node list is passed.

[KAMEZAWA Hiroyuki]

On Wed, 8 Nov 2006 13:47:44 +1100
Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> On Wed, 8 Nov 2006 11:13:41 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> >
> > Ah.. I'm mentioning to this.
> > ==
> > +			pm[i].node = 0;	/* anything to not match MAX_NUMNODES */
> > ==
> > Sorry for my bad cut & paste.
> >
> > It seems that this 0 will be passed to alloc_pages_node().
> > alloc_pages_node() doesn't check whether a node is online or not before using
> > NODE_DATA().
> 
> Actually, it won't.  If you do that assignment, then the nodes parameter
> was NULL and you will only call do_pages_stat() and so never call
> alloc_pages_node().
> 
Ah..Okay, I'm sorry for noise.

-Kame