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 <[email protected]>
---
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 [email protected]
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
On Fri, 3 Nov 2006, Stephen Rothwell wrote:
> Signed-off-by: Stephen Rothwell <[email protected]>
Acked-by: Christoph Lameter <[email protected]>
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 [email protected] Thu Nov 2 19:46:17 2006
Date: Fri, 3 Nov 2006 14:42:43 +1100
From: Stephen Rothwell <[email protected]>
To: Christoph Lameter <[email protected]>
Message-Id: <[email protected]>
Cc: Andrew Morton <[email protected]>, [email protected], LKML <[email protected]>, [email protected]
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 <[email protected]>
Acked-by: Christoph Lameter <[email protected]>
Signed-off-by: Chris Wright <[email protected]>
---
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 [email protected] are
queue-2.6.18/fix-sys_move_pages-when-a-null-node-list-is-passed.patch
On Fri, 3 Nov 2006 14:42:43 +1100
Stephen Rothwell <[email protected]> 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
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.
On Tue, 7 Nov 2006 18:01:11 -0800 (PST)
Christoph Lameter <[email protected]> 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
On Wed, 8 Nov 2006 11:13:41 +0900 KAMEZAWA Hiroyuki <[email protected]> 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 [email protected]
http://www.canb.auug.org.au/~sfr/
On Wed, 8 Nov 2006 13:47:44 +1100
Stephen Rothwell <[email protected]> wrote:
> On Wed, 8 Nov 2006 11:13:41 +0900 KAMEZAWA Hiroyuki <[email protected]> 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