* [PATCH] powerpc/prom: Avoid reference to potentially freed memory
@ 2015-10-16 6:14 Christophe JAILLET
2015-10-16 7:50 ` kbuild test robot
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Christophe JAILLET @ 2015-10-16 6:14 UTC (permalink / raw)
To: benh, paulus, mpe
Cc: linuxppc-dev, linux-kernel, kernel-janitors, Christophe JAILLET
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] powerpc/prom: Avoid reference to potentially freed memory
2015-10-16 6:14 [PATCH] powerpc/prom: Avoid reference to potentially freed memory Christophe JAILLET
@ 2015-10-16 7:50 ` kbuild test robot
2015-10-16 10:02 ` Michael Ellerman
2015-10-16 21:38 ` [PATCH v2] " Christophe JAILLET
2 siblings, 0 replies; 9+ messages in thread
From: kbuild test robot @ 2015-10-16 7:50 UTC (permalink / raw)
To: Christophe JAILLET
Cc: kbuild-all, benh, paulus, mpe, linuxppc-dev, linux-kernel,
kernel-janitors, Christophe JAILLET
[-- Attachment #1: Type: text/plain, Size: 3467 bytes --]
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
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 21508 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] powerpc/prom: Avoid reference to potentially freed memory
2015-10-16 6:14 [PATCH] powerpc/prom: Avoid reference to potentially freed memory Christophe JAILLET
2015-10-16 7:50 ` kbuild test robot
@ 2015-10-16 10:02 ` Michael Ellerman
2015-10-16 20:09 ` Christophe JAILLET
2015-10-16 21:38 ` [PATCH v2] " Christophe JAILLET
2 siblings, 1 reply; 9+ messages in thread
From: Michael Ellerman @ 2015-10-16 10:02 UTC (permalink / raw)
To: Christophe JAILLET, benh, paulus
Cc: linuxppc-dev, linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] powerpc/prom: Avoid reference to potentially freed memory
2015-10-16 10:02 ` Michael Ellerman
@ 2015-10-16 20:09 ` Christophe JAILLET
2015-10-19 9:32 ` Michael Ellerman
0 siblings, 1 reply; 9+ messages in thread
From: Christophe JAILLET @ 2015-10-16 20:09 UTC (permalink / raw)
To: linuxppc-dev; +Cc: linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] powerpc/prom: Avoid reference to potentially freed memory
2015-10-16 6:14 [PATCH] powerpc/prom: Avoid reference to potentially freed memory Christophe JAILLET
2015-10-16 7:50 ` kbuild test robot
2015-10-16 10:02 ` Michael Ellerman
@ 2015-10-16 21:38 ` Christophe JAILLET
2015-10-19 9:27 ` Michael Ellerman
2015-10-21 11:41 ` [v2] " Michael Ellerman
2 siblings, 2 replies; 9+ messages in thread
From: Christophe JAILLET @ 2015-10-16 21:38 UTC (permalink / raw)
To: benh, paulus, mpe
Cc: linuxppc-dev, linux-kernel, kernel-janitors, Christophe JAILLET
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] powerpc/prom: Avoid reference to potentially freed memory
2015-10-16 21:38 ` [PATCH v2] " Christophe JAILLET
@ 2015-10-19 9:27 ` Michael Ellerman
2015-10-21 4:36 ` [PATCH v3] " Christophe JAILLET
2015-10-21 11:41 ` [v2] " Michael Ellerman
1 sibling, 1 reply; 9+ messages in thread
From: Michael Ellerman @ 2015-10-19 9:27 UTC (permalink / raw)
To: Christophe JAILLET, benh, paulus
Cc: linuxppc-dev, linux-kernel, kernel-janitors
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 <christophe.jaillet@wanadoo.fr>
> ---
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] powerpc/prom: Avoid reference to potentially freed memory
2015-10-16 20:09 ` Christophe JAILLET
@ 2015-10-19 9:32 ` Michael Ellerman
0 siblings, 0 replies; 9+ messages in thread
From: Michael Ellerman @ 2015-10-19 9:32 UTC (permalink / raw)
To: Christophe JAILLET, linuxppc-dev; +Cc: kernel-janitors, linux-kernel
On Fri, 2015-10-16 at 22:09 +0200, Christophe JAILLET wrote:
> 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.
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] powerpc/prom: Avoid reference to potentially freed memory
2015-10-19 9:27 ` Michael Ellerman
@ 2015-10-21 4:36 ` Christophe JAILLET
0 siblings, 0 replies; 9+ messages in thread
From: Christophe JAILLET @ 2015-10-21 4:36 UTC (permalink / raw)
To: benh, paulus, mpe
Cc: linuxppc-dev, linux-kernel, kernel-janitors, Christophe JAILLET
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 <christophe.jaillet@wanadoo.fr>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [v2] powerpc/prom: Avoid reference to potentially freed memory
2015-10-16 21:38 ` [PATCH v2] " Christophe JAILLET
2015-10-19 9:27 ` Michael Ellerman
@ 2015-10-21 11:41 ` Michael Ellerman
1 sibling, 0 replies; 9+ messages in thread
From: Michael Ellerman @ 2015-10-21 11:41 UTC (permalink / raw)
To: Christophe Jaillet, benh, paulus
Cc: kernel-janitors, Christophe JAILLET, linuxppc-dev, linux-kernel
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 <christophe.jaillet@wanadoo.fr>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/1856f50c66dff0afb4a6a3e2
cheers
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-10-21 11:41 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-16 6:14 [PATCH] powerpc/prom: Avoid reference to potentially freed memory Christophe JAILLET
2015-10-16 7:50 ` kbuild test robot
2015-10-16 10:02 ` Michael Ellerman
2015-10-16 20:09 ` Christophe JAILLET
2015-10-19 9:32 ` Michael Ellerman
2015-10-16 21:38 ` [PATCH v2] " Christophe JAILLET
2015-10-19 9:27 ` Michael Ellerman
2015-10-21 4:36 ` [PATCH v3] " Christophe JAILLET
2015-10-21 11:41 ` [v2] " Michael Ellerman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).