[PATCH] random: Fix crashes with sparse node ids

On a system with sparse node ids, eg. a powerpc system with 4 nodes
numbered like so:

  node   0: [mem 0x0000000000000000-0x00000007ffffffff]
  node   1: [mem 0x0000000800000000-0x0000000fffffffff]
  node  16: [mem 0x0000001000000000-0x00000017ffffffff]
  node  17: [mem 0x0000001800000000-0x0000001fffffffff]

The code in rand_initialize() will allocate 4 pointers for the pool
array, and initialise them correctly.

However when go to use the pool, in eg. extract_crng(), we use the
numa_node_id() to index into the array. For the higher numbered node ids
this leads to random memory corruption, depending on what was kmalloc'ed
adjacent to the pool array.

Fix it by using nr_node_ids to size the pool array.

Fixes: 1e7f583af67b ("random: make /dev/urandom scalable for silly userspace programs")
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 drivers/char/random.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 7f0622426b97..ea03dfe2f21c 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c

@@ -249,6 +249,7 @@
 #include <linux/genhd.h>
 #include <linux/interrupt.h>
 #include <linux/mm.h>
+#include <linux/nodemask.h>
 #include <linux/spinlock.h>
 #include <linux/kthread.h>
 #include <linux/percpu.h>

@@ -1656,7 +1657,6 @@ static int rand_initialize(void)
 {
 #ifdef CONFIG_NUMA
 	int i;
-	int num_nodes = num_possible_nodes();
 	struct crng_state *crng;
 	struct crng_state **pool;
 #endif

@@ -1666,7 +1666,7 @@ static int rand_initialize(void)
 	crng_initialize(&primary_crng);
 
 #ifdef CONFIG_NUMA
-	pool = kmalloc(num_nodes * sizeof(void *),
+	pool = kmalloc(nr_node_ids * sizeof(void *),
 		       GFP_KERNEL|__GFP_NOFAIL|__GFP_ZERO);
 	for_each_online_node(i) {
 		crng = kmalloc_node(sizeof(struct crng_state),

-- 
2.7.4

On Sat, Jul 30, 2016 at 7:23 AM, Michael Ellerman [off-list ref] wrote:

 #ifdef CONFIG_NUMA
-       pool = kmalloc(num_nodes * sizeof(void *),
+       pool = kmalloc(nr_node_ids * sizeof(void *),
                       GFP_KERNEL|__GFP_NOFAIL|__GFP_ZERO);
        for_each_online_node(i) {
                crng = kmalloc_node(sizeof(struct crng_state),

Ugh. Can we please also just change that kmalloc to kcalloc()? Get rid
of the odd multiplication and the unusual GFP mask bit crud?

And instead of using "sizeof(void *)", just use the pool entry size,
ie "sizeof(*pool)". Yes, we have other places where we depend on void
pointers having the same size as others, but it's the RightThing(tm)
to do anyway, and it makes more sense when you grep things ("Oh, we're
allocating 'nr_node_id' copes of *pool entries" even without knowing
what type is behind the "pool" pointer).

IOW, can you confirm that you could just use

        pool = kcalloc(nr_node_ids, sizeof(*pool), GFP_KERNEL);

instead? I'd much rather apply that patch.

               Linus

Linus Torvalds [off-list ref] writes:

On Sat, Jul 30, 2016 at 7:23 AM, Michael Ellerman [off-list ref] wrote:

quoted

 #ifdef CONFIG_NUMA
-       pool = kmalloc(num_nodes * sizeof(void *),
+       pool = kmalloc(nr_node_ids * sizeof(void *),
                       GFP_KERNEL|__GFP_NOFAIL|__GFP_ZERO);
        for_each_online_node(i) {
                crng = kmalloc_node(sizeof(struct crng_state),

Ugh. Can we please also just change that kmalloc to kcalloc()? Get rid
of the odd multiplication and the unusual GFP mask bit crud?

And instead of using "sizeof(void *)", just use the pool entry size,
ie "sizeof(*pool)". Yes, we have other places where we depend on void
pointers having the same size as others, but it's the RightThing(tm)
to do anyway, and it makes more sense when you grep things ("Oh, we're
allocating 'nr_node_id' copes of *pool entries" even without knowing
what type is behind the "pool" pointer).

IOW, can you confirm that you could just use

        pool = kcalloc(nr_node_ids, sizeof(*pool), GFP_KERNEL);

instead? I'd much rather apply that patch.

Dropping NOFAIL means we need to handle allocation failures, which makes
the patch a bit bigger, and less of a pure fix.

Here's a separate patch to do those cleanups, which you can squash with
the first if you prefer.

I did test the allocation failure case for both the whole pool and
individual nodes.

cheers

From 2f5ab5fa2b9e4997fe01053fc12f689fb2117f45 Mon Sep 17 00:00:00 2001

From: Michael Ellerman <mpe@ellerman.id.au>
Date: Sun, 31 Jul 2016 12:54:40 +1000
Subject: [PATCH] random: Clean up NUMA allocations

Use kcalloc(), rather than doing the multiply by hand. Use sizeof(*pool)
rather than assuming it's == sizeof(void *). kcalloc() zeroes by default
so we don't need __GFP_ZERO.

Drop the __GFP_NOFAILs, we can easily handle allocation failures, the
code is already written to cope with a NULL crng_node_pool, or a NULL
entry for a given node in the pool.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 drivers/char/random.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index ea03dfe2f21c..22c8ac173666 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c

@@ -1666,11 +1666,15 @@ static int rand_initialize(void)
 	crng_initialize(&primary_crng);
 
 #ifdef CONFIG_NUMA
-	pool = kmalloc(nr_node_ids * sizeof(void *),
-		       GFP_KERNEL|__GFP_NOFAIL|__GFP_ZERO);
+	pool = kcalloc(nr_node_ids, sizeof(*pool), GFP_KERNEL);
+	if (!pool)
+		return 0;
+
 	for_each_online_node(i) {
-		crng = kmalloc_node(sizeof(struct crng_state),
-				    GFP_KERNEL | __GFP_NOFAIL, i);
+		crng = kmalloc_node(sizeof(struct crng_state), GFP_KERNEL, i);
+		if (!crng)
+			continue;
+
 		spin_lock_init(&crng->lock);
 		crng_initialize(crng);
 		pool[i] = crng;

-- 
2.7.4

On Sat, Jul 30, 2016 at 8:27 PM, Michael Ellerman [off-list ref] wrote:

Dropping NOFAIL means we need to handle allocation failures, which makes
the patch a bit bigger, and less of a pure fix.

Hmm. If you get allocation failures for something like this at init
time, I think you're basically screwed anyway.

And I really only meant for the initial array allocation, and the
__GFP_ZERO thing.

Yes, __GFP_ZERO does work for kmalloc() too, but unlike the other GPF
flags, we do have special zalloc versions for zeroing that are
generally preferred.

              Linus