[PATCH] powerpc/prom: Avoid reference to potentially freed memory

of_get_property() is used inside the loop, but then the reference to the
node is dropped before dereferencing the prop pointer, which could by then
point to junk if the node has been freed.

Instead use of_property_read_u32() to actually read the property
value before dropping the reference.

Signed-off-by: Christophe JAILLET <redacted>
---
*** UNTESTED ***
---
 arch/powerpc/kernel/prom.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index bef76c5..dc4f6a4 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c

@@ -783,14 +783,13 @@ void __init early_get_first_memblock_info(void *params, phys_addr_t *size)
 int of_get_ibm_chip_id(struct device_node *np)
 {
 	of_node_get(np);
-	while(np) {
+	while (np) {
 		struct device_node *old = np;
-		const __be32 *prop;
+		u32 chip_id;
 
-		prop = of_get_property(np, "ibm,chip-id", NULL);
-		if (prop) {
+		if (!of_property_read_u32(np, "ibm,chip-id", &chip_id))
 			of_node_put(np);
-			return be32_to_cpup(prop);
+			return chip_id;
 		}
 		np = of_get_parent(np);
 		of_node_put(old);

-- 
2.1.4

Hi Christophe,

[auto build test ERROR on powerpc/next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Christophe-JAILLET/powerpc-prom-Avoid-reference-to-potentially-freed-memory/20151016-141714
config: powerpc-defconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

   arch/powerpc/kernel/prom.c: In function 'of_get_ibm_chip_id':

quoted

arch/powerpc/kernel/prom.c:787:23: error: unused variable 'old' [-Werror=unused-variable]

      struct device_node *old = np;
                          ^

quoted

arch/powerpc/kernel/prom.c:795:15: error: 'old' undeclared (first use in this function)

      of_node_put(old);
                  ^
   arch/powerpc/kernel/prom.c:795:15: note: each undeclared identifier is reported only once for each function it appears in
   arch/powerpc/kernel/prom.c: At top level:

quoted

arch/powerpc/kernel/prom.c:797:2: error: expected identifier or '(' before 'return'

     return -1;
     ^

quoted

arch/powerpc/kernel/prom.c:798:1: error: expected identifier or '(' before '}' token

    }
    ^
   arch/powerpc/kernel/prom.c: In function 'of_get_ibm_chip_id':
   arch/powerpc/kernel/prom.c:796:2: error: control reaches end of non-void function [-Werror=return-type]
     }
     ^
   cc1: all warnings being treated as errors

vim +/old +787 arch/powerpc/kernel/prom.c

b37193b7 Benjamin Herrenschmidt 2013-07-15  781   * be found.
b37193b7 Benjamin Herrenschmidt 2013-07-15  782   */
b37193b7 Benjamin Herrenschmidt 2013-07-15  783  int of_get_ibm_chip_id(struct device_node *np)
b37193b7 Benjamin Herrenschmidt 2013-07-15  784  {
b37193b7 Benjamin Herrenschmidt 2013-07-15  785  	of_node_get(np);
b37193b7 Benjamin Herrenschmidt 2013-07-15  786  	while (np) {
b37193b7 Benjamin Herrenschmidt 2013-07-15 @787  		struct device_node *old = np;
12540384 Christophe JAILLET     2015-10-16  788  		u32 chip_id;
b37193b7 Benjamin Herrenschmidt 2013-07-15  789  
12540384 Christophe JAILLET     2015-10-16  790  		if (!of_property_read_u32(np, "ibm,chip-id", &chip_id))
b37193b7 Benjamin Herrenschmidt 2013-07-15  791  			of_node_put(np);
12540384 Christophe JAILLET     2015-10-16  792  			return chip_id;
b37193b7 Benjamin Herrenschmidt 2013-07-15  793  		}
b37193b7 Benjamin Herrenschmidt 2013-07-15  794  		np = of_get_parent(np);
b37193b7 Benjamin Herrenschmidt 2013-07-15 @795  		of_node_put(old);
b37193b7 Benjamin Herrenschmidt 2013-07-15  796  	}
b37193b7 Benjamin Herrenschmidt 2013-07-15 @797  	return -1;
b37193b7 Benjamin Herrenschmidt 2013-07-15 @798  }
b130e7c0 Dan Streetman          2015-05-07  799  EXPORT_SYMBOL(of_get_ibm_chip_id);
b37193b7 Benjamin Herrenschmidt 2013-07-15  800  
3eb906c6 Michael Ellerman       2013-11-20  801  /**

:::::: The code at line 787 was first introduced by commit
:::::: b37193b71846858d816e152d3a5db010d7b73f5e powerpc/powernv: Add helper to get ibm,chip-id of a node

:::::: TO: Benjamin Herrenschmidt [off-list ref]
:::::: CC: Benjamin Herrenschmidt [off-list ref]

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

On Fri, 2015-10-16 at 08:14 +0200, Christophe JAILLET wrote:

As the kbuild robot detected you have left an extra "}" here.

I don't mind too much if you send patches that aren't compile tested, but you
might save yourself some time by compiling them.

There are x86->powerpc cross compilers here:

https://www.kernel.org/pub/tools/crosstool/files/bin/x86_64/4.9.0/x86_64-gcc-4.9.0-nolibc_powerpc64-linux.tar.gz

Or if you're running on Ubuntu you can just do:

$ apt-get install gcc-powerpc-linux-gnu

I think there's a package for Fedora too but I don't know the name off the top
of my head.

cheers

Le 16/10/2015 12:02, Michael Ellerman a écrit :

As the kbuild robot detected you have left an extra "}" here.

I don't mind too much if you send patches that aren't compile tested, but you
might save yourself some time by compiling them.

Sorry about it, and thanks for your patience.
IMHO, this should never happen and patches should be at least 
compile-tested.

I will be more careful and compile-test any new patch I submit.

CJ

On Fri, 2015-10-16 at 22:09 +0200, Christophe JAILLET wrote:

Le 16/10/2015 12:02, Michael Ellerman a écrit :

quoted

As the kbuild robot detected you have left an extra "}" here.

I don't mind too much if you send patches that aren't compile tested, but you
might save yourself some time by compiling them.

Sorry about it, and thanks for your patience.
IMHO, this should never happen and patches should be at least 
compile-tested.

Preferably yes. But for these sort of cleanup patches, where you're touching
lots of different arches, I realise it can be tedious to find all the various
cross compilers. So I'm willing to cut you a bit of slack :)

I will be more careful and compile-test any new patch I submit.

Thanks.

cheers

of_get_property() is used inside the loop, but then the reference to the
node is dropped before dereferencing the prop pointer, which could by then
point to junk if the node has been freed.

Instead use of_property_read_u32() to actually read the property
value before dropping the reference.

Signed-off-by: Christophe JAILLET <redacted>
---
v2: Fix missing '{'
*** COMPILE-TESTED ONLY ***
---
 arch/powerpc/kernel/prom.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index bef76c5..dc4f6a4 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c

@@ -783,14 +783,13 @@ void __init early_get_first_memblock_info(void *params, phys_addr_t *size)
 int of_get_ibm_chip_id(struct device_node *np)
 {
 	of_node_get(np);
-	while(np) {
+	while (np) {
 		struct device_node *old = np;
-		const __be32 *prop;
+		u32 chip_id;
 
-		prop = of_get_property(np, "ibm,chip-id", NULL);
-		if (prop) {
+		if (!of_property_read_u32(np, "ibm,chip-id", &chip_id)) {
 			of_node_put(np);
-			return be32_to_cpup(prop);
+			return chip_id;
 		}
 		np = of_get_parent(np);
 		of_node_put(old);

-- 
2.1.4

On Fri, 2015-10-16 at 23:38 +0200, Christophe JAILLET wrote:

of_get_property() is used inside the loop, but then the reference to the
node is dropped before dereferencing the prop pointer, which could by then
point to junk if the node has been freed.

Instead use of_property_read_u32() to actually read the property
value before dropping the reference.

Signed-off-by: Christophe JAILLET <redacted>
---
v2: Fix missing '{'
*** COMPILE-TESTED ONLY ***

Thanks, this looks good. I'll test it on real hardware.

Can you send me a follow up which does the of_get_next_parent() conversion?

cheers

of_get_property() is used inside the loop, but then the reference to the
node is dropped before dereferencing the prop pointer, which could by then
point to junk if the node has been freed.
Instead use of_property_read_u32() to actually read the property
value before dropping the reference.

Use of_get_next_parent to simplify code.

Signed-off-by: Christophe JAILLET <redacted>
---
v2: Fix missing '{'
v3: Use of_get_next_parent to simply code
*** COMPILE-TESTED ONLY ***
---
 arch/powerpc/kernel/prom.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index bef76c5..ba29c0d 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c

@@ -783,17 +783,14 @@ void __init early_get_first_memblock_info(void *params, phys_addr_t *size)
 int of_get_ibm_chip_id(struct device_node *np)
 {
 	of_node_get(np);
-	while(np) {
-		struct device_node *old = np;
-		const __be32 *prop;
+	while (np) {
+		u32 chip_id;
 
-		prop = of_get_property(np, "ibm,chip-id", NULL);
-		if (prop) {
+		if (!of_property_read_u32(np, "ibm,chip-id", &chip_id)) {
 			of_node_put(np);
-			return be32_to_cpup(prop);
+			return chip_id;
 		}
-		np = of_get_parent(np);
-		of_node_put(old);
+		np = of_get_next_parent(np);
 	}
 	return -1;
 }

-- 
2.1.4

On Fri, 2015-16-10 at 21:38:45 UTC, Christophe Jaillet wrote:

of_get_property() is used inside the loop, but then the reference to the
node is dropped before dereferencing the prop pointer, which could by then
point to junk if the node has been freed.

Instead use of_property_read_u32() to actually read the property
value before dropping the reference.

Signed-off-by: Christophe JAILLET <redacted>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/1856f50c66dff0afb4a6a3e2

cheers