Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory

[Michael Ellerman]

On Fri, 2015-10-16 at 22:05 +0200, Christophe JAILLET wrote:
> Hi,
> sorry if un-clear.
> 
> What I mean is that in the patch related 
> 'powerpc/sysdev/mpc5xxx_clocks.c', there was no call to 'be32_to_cpup'.
> So in the proposed patch, 'of_property_read_u32' adds it.
> 
> While in the patch against 'powerpc/kernel/prom.c', 'be32_to_cpup' was 
> called explicitly.
> So using 'of_property_read_u32' keep the same logic.

Ah right, I understand now.

> Basically the code from 'mpc5xxx_clocks.c' and from 'prom.c' were 
> written the same way. I found spurious that a call to 'be32_to_cpup' was 
> done in only one case.
> Maybe, it was a missing in 'mpc5xxx_clocks.c'.

Yes it was missing in that code.

But that's not a real bug because that code only ever runs on BE systems.

cheers

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory

[Christophe JAILLET]

Le 16/10/2015 11:49, Michael Ellerman a écrit :

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

Le 15/10/2015 08:36, Michael Ellerman a écrit :

On Thu, 2015-10-15 at 07:56 +0200, Christophe JAILLET wrote:

Use 'of_property_read_u32()' instead of
'of_get_property()'+pointer
dereference in order to avoid access to potentially freed memory.

Use 'of_get_next_parent()' to simplify the while() loop and avoid
the
need of a temp variable.

Signed-off-by: Christophe JAILLET 
---
v2: Use of_property_read_u32 instead of of_get_property+pointer
dereference
*** Untested ***

Thanks.

Can someone with an mpc5xxx test this?

Hi,
I don't think it is an issue, but while looking at another similar
patch, I noticed that the proposed patch adds a call to
be32_to_cpup()
(within of_property_read_u32).
Apparently, powerPC is a BE architecture, so this call should be a no
-op.

Just wanted to point it out, in case of.

Hi Christoph,

I'm not sure I follow.

The device tree is always big endian, but of_property_read_u32() does
the
conversion to CPU endian for you already. That is one of the advantages
of
using it.

cheers



Hi,
sorry if un-clear.

What I mean is that in the patch related 
'powerpc/sysdev/mpc5xxx_clocks.c', there was no call to 'be32_to_cpup'.

So in the proposed patch, 'of_property_read_u32' adds it.

While in the patch against 'powerpc/kernel/prom.c', 'be32_to_cpup' was 
called explicitly.

So using 'of_property_read_u32' keep the same logic.


Basically the code from 'mpc5xxx_clocks.c' and from 'prom.c' were 
written the same way. I found spurious that a call to 'be32_to_cpup' was 
done in only one case.

Maybe, it was a missing in 'mpc5xxx_clocks.c'.


I don't know if it can be an issue or not. I just find it 'strange'.


CJ


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory

[Christophe JAILLET]

Le 15/10/2015 08:36, Michael Ellerman a écrit :

On Thu, 2015-10-15 at 07:56 +0200, Christophe JAILLET wrote:

Use 'of_property_read_u32()' instead of 'of_get_property()'+pointer
dereference in order to avoid access to potentially freed memory.

Use 'of_get_next_parent()' to simplify the while() loop and avoid the
need of a temp variable.

Signed-off-by: Christophe JAILLET 
---
v2: Use of_property_read_u32 instead of of_get_property+pointer dereference
*** Untested ***

Thanks.

Can someone with an mpc5xxx test this?

cheers



Hi,
I don't think it is an issue, but while looking at another similar 
patch, I noticed that the proposed patch adds a call to be32_to_cpup() 
(within of_property_read_u32).

Apparently, powerPC is a BE architecture, so this call should be a no-op.

Just wanted to point it out, in case of.

Best regards,
CJ

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory

[Gabriel Paubert]

On Fri, Oct 16, 2015 at 08:20:13AM +0200, Christophe JAILLET wrote:
> Le 15/10/2015 08:36, Michael Ellerman a écrit :
> >On Thu, 2015-10-15 at 07:56 +0200, Christophe JAILLET wrote:
> >>Use 'of_property_read_u32()' instead of 'of_get_property()'+pointer
> >>dereference in order to avoid access to potentially freed memory.
> >>
> >>Use 'of_get_next_parent()' to simplify the while() loop and avoid the
> >>need of a temp variable.
> >>
> >>Signed-off-by: Christophe JAILLET 
> >>---
> >>v2: Use of_property_read_u32 instead of of_get_property+pointer dereference
> >>*** Untested ***
> >Thanks.
> >
> >Can someone with an mpc5xxx test this?
> >
> >cheers
> >
> 
> Hi,
> I don't think it is an issue, but while looking at another similar
> patch, I noticed that the proposed patch adds a call to
> be32_to_cpup() (within of_property_read_u32).
> Apparently, powerPC is a BE architecture, so this call should be a no-op.

Sadly no more. 32 bit is BE only, but 64 bit can be either BEtter or
LEsser.

Gabriel
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory

[Michael Ellerman]

On Fri, 2015-10-16 at 08:20 +0200, Christophe JAILLET wrote:
> Le 15/10/2015 08:36, Michael Ellerman a écrit :
> > On Thu, 2015-10-15 at 07:56 +0200, Christophe JAILLET wrote:
> > > Use 'of_property_read_u32()' instead of
> > > 'of_get_property()'+pointer
> > > dereference in order to avoid access to potentially freed memory.
> > > 
> > > Use 'of_get_next_parent()' to simplify the while() loop and avoid
> > > the
> > > need of a temp variable.
> > > 
> > > Signed-off-by: Christophe JAILLET 
> > > ---
> > > v2: Use of_property_read_u32 instead of of_get_property+pointer
> > > dereference
> > > *** Untested ***
> > Thanks.
> > 
> > Can someone with an mpc5xxx test this?
> 
> Hi,
> I don't think it is an issue, but while looking at another similar 
> patch, I noticed that the proposed patch adds a call to
> be32_to_cpup() 
> (within of_property_read_u32).
> Apparently, powerPC is a BE architecture, so this call should be a no
> -op.
> 
> Just wanted to point it out, in case of.

Hi Christoph,

I'm not sure I follow.

The device tree is always big endian, but of_property_read_u32() does
the
conversion to CPU endian for you already. That is one of the advantages
of
using it.

cheers

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory

[Christophe JAILLET]

Use 'of_property_read_u32()' instead of 'of_get_property()'+pointer
dereference in order to avoid access to potentially freed memory.

Use 'of_get_next_parent()' to simplify the while() loop and avoid the
need of a temp variable.

Signed-off-by: Christophe JAILLET 
---
v2: Use of_property_read_u32 instead of of_get_property+pointer dereference
*** Untested ***
---
 arch/powerpc/sysdev/mpc5xxx_clocks.c | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/sysdev/mpc5xxx_clocks.c 
b/arch/powerpc/sysdev/mpc5xxx_clocks.c
index f4f0301..92fbcf8 100644
--- a/arch/powerpc/sysdev/mpc5xxx_clocks.c
+++ b/arch/powerpc/sysdev/mpc5xxx_clocks.c
@@ -13,21 +13,17 @@
 
 unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
 {
-   struct device_node *np;
-   const unsigned int *p_bus_freq = NULL;
+   u32 bus_freq = 0;
 
of_node_get(node);
while (node) {
-   p_bus_freq = of_get_property(node, "bus-frequency", NULL);
-   if (p_bus_freq)
+   if (!of_property_read_u32(node, "bus-frequency", _freq))
break;
 
-   np = of_get_parent(node);
-   of_node_put(node);
-   node = np;
+   node = of_get_next_parent(node);
}
of_node_put(node);
 
-   return p_bus_freq ? *p_bus_freq : 0;
+   return bus_freq;
 }
 EXPORT_SYMBOL(mpc5xxx_get_bus_frequency);
-- 
2.1.4

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev