Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1248363 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] powerpc/prom: Avoid reference to potentially freed memory Christophe JAILLET <christophe.jaillet@wanadoo.fr> - 2015-10-16 08:20 +0200
    Re: [PATCH] powerpc/prom: Avoid reference to potentially freed memory kbuild test robot <lkp@intel.com> - 2015-10-16 10:00 +0200
    Re: [PATCH] powerpc/prom: Avoid reference to potentially freed  memory Michael Ellerman <mpe@ellerman.id.au> - 2015-10-16 12:10 +0200
      Re: [PATCH] powerpc/prom: Avoid reference to potentially freed memory Christophe JAILLET <christophe.jaillet@wanadoo.fr> - 2015-10-16 22:20 +0200
    [PATCH v2] powerpc/prom: Avoid reference to potentially freed memory Christophe JAILLET <christophe.jaillet@wanadoo.fr> - 2015-10-16 23:40 +0200

#1248363 — [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 <christophe.jaillet@wanadoo.fr>
---
*** 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1248442

[Multipart message — attachments visible in raw view] — view raw

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':
>> arch/powerpc/kernel/prom.c:787:23: error: unused variable 'old' [-Werror=unused-variable]
      struct device_node *old = np;
                          ^
>> 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:
>> arch/powerpc/kernel/prom.c:797:2: error: expected identifier or '(' before 'return'
     return -1;
     ^
>> 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 <benh@kernel.crashing.org>
:::::: CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>

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

[toc] | [prev] | [next] | [standalone]

#1248547 — Re: [PATCH] powerpc/prom: Avoid reference to potentially freed memory

On Fri, 2015-10-16 at 08:14 +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 <christophe.jaillet@wanadoo.fr>
> ---
> *** 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;
>  		}


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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1249133

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1249159 — [PATCH v2] 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 <christophe.jaillet@wanadoo.fr>
---
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web