Re: [RFC/PATCH] of: Mark property::value as const

[Stephen Boyd]

Quoting Frank Rowand (2017-03-11 22:27:08)
> Hi Stephen,
> 
> On 02/23/17 15:08, Frank Rowand wrote:
> > On 02/13/17 18:50, Stephen Boyd wrote:
> >> The 'blob' we pass into populate_properties() is marked as const,
> >> but we cast that const away when we assign the result of
> >> fdt_getprop_by_offset() to pp->value. Let's mark value as const
> >> instead, so that code can't mistakenly write to the value of the
> >> property that we've so far advertised as const.
> > 
> > Instead of struct property field value being a pointer into the
> > FDT, I would rather copy the data to newly allocated memory and
> > have value be a pointer to that memory.  This is required if we
> > want to make /sys/firmware/fdt optional, which would allow us to
> > free the memory containing the initial boot FDT.
> > 
> > I also do not want overlay live subtrees to have any pointers
> > into the FDT that was used to populate the overlay, so copying
> > the data solves that problem also.
> > 
> > 
> >> Unfortunately, this exposes a problem with the fdt resolver code,
> >> where we overwrite the value member of properties of phandles to
> >> update them with their final value. Add a comment for now to
> >> indicate where we're potentially writing over const data.
> > 
> > Yes, the resolver code needs to adjust phandle values.
> > 
> > I think I can get rid of the resolver modifying the various phandle
> > values, and instead just modify the phandle value in struct
> > device_node.  At the same time, I think I can also remove all
> > instances of the phandle properties ('linux,phandle', 'ibm,phandle',
> > 'phandle') in the live tree.  These properties should not be
> > accessed directly by any code outside of the device tree framework
> > since the phandle is located in the struct device_node.  A quick
> > grep does not show any such accesses of the phandle properties,
> > but I want to look more closely.
> 
> After reading through a bit of code, I am confident that I can
> modify the resolver code to not modify the various phandle
> property values.  There are a few tentacles reaching out to
> other areas that I will have to fix also.  The biggest task
> for me will be to create some good test code.
> 
> I'll be unavailable this week, so I'll start on it in about
> a week.

Ok. No worries from my side. I'm interested to see how random properties
will be "corrected" without modifying them. I can only assume the plan
is to copy those properties.

The kbuild robot found more cases where marking this as const causes
issues. Please see the updated patch inline.

-8<-
From: Stephen Boyd 
Subject: [PATCH] of: Mark property::value as const

The 'blob' we pass into populate_properties() is marked as const,
but we cast that const away when we assign the result of
fdt_getprop_by_offset() to pp->value. Let's mark value as const
instead, so that code can't mistakenly write to the value of the
property that we've so far advertised as const.

Unfortunately, this exposes a problem with the fdt resolver code,
where we overwrite the value member of properties of phandles to
update them with their final value. Add a comment for now to
indicate where we're potentially writing over const data.

You can see such the problem here by loading an overlay dtb into
the kernel via the fw helper method (not direct loading) and
passing that tree to the resolver on an arm64 device. In this
case, the firmware data is vmapped with KERNEL_PAGE_RO and the
resolver code crashes when attempting to write to blob to update
the phandle properties.

Signed-off-by: Stephen Boyd 
---
 drivers/of/base.c |  2 +-
 drivers/of/fdt.c  | 12 ++--
 drivers/of/pdt.c  |  9 +
 drivers/of/resolver.c |  3 +++
 fs/openpromfs/inode.c |  2 +-
 include/linux/of.h|  2 +-
 6 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index fb6bb855714e..8e5f29b814c9 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1156,7 +1156,7 @@ EXPORT_SYMBOL_GPL(of_property_count_elems_of_size);
  * property data is too small or too large.
  *
  */
-static void *of_find_property_value_of_size(const struct device_node *np,
+static const void *of_find_property_value_of_size(const struct device_node *np,
const char *propname, u32 min, u32 max, size_t *len)
 {
struct property *prop = of_find_property(np, propname, NULL);
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index c9b5cac03b36..0635ef5dabf3 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -222,7 +222,7 @@ static void populate_properties(const void *blob,
 
pp->name   = (char *)pname;
pp->length = sz;
-   pp->value  = (__be32 *)val;
+   pp->value  = val;
*pprev = pp;
pprev  = >next;
}
@@ -232,6 +232,7 @@ static void populate_properties(const void *blob,
 */

Re: [RFC/PATCH] of: Mark property::value as const

[Stephen Boyd]

Quoting Frank Rowand (2017-03-11 22:27:08)
> Hi Stephen,
> 
> On 02/23/17 15:08, Frank Rowand wrote:
> > On 02/13/17 18:50, Stephen Boyd wrote:
> >> The 'blob' we pass into populate_properties() is marked as const,
> >> but we cast that const away when we assign the result of
> >> fdt_getprop_by_offset() to pp->value. Let's mark value as const
> >> instead, so that code can't mistakenly write to the value of the
> >> property that we've so far advertised as const.
> > 
> > Instead of struct property field value being a pointer into the
> > FDT, I would rather copy the data to newly allocated memory and
> > have value be a pointer to that memory.  This is required if we
> > want to make /sys/firmware/fdt optional, which would allow us to
> > free the memory containing the initial boot FDT.
> > 
> > I also do not want overlay live subtrees to have any pointers
> > into the FDT that was used to populate the overlay, so copying
> > the data solves that problem also.
> > 
> > 
> >> Unfortunately, this exposes a problem with the fdt resolver code,
> >> where we overwrite the value member of properties of phandles to
> >> update them with their final value. Add a comment for now to
> >> indicate where we're potentially writing over const data.
> > 
> > Yes, the resolver code needs to adjust phandle values.
> > 
> > I think I can get rid of the resolver modifying the various phandle
> > values, and instead just modify the phandle value in struct
> > device_node.  At the same time, I think I can also remove all
> > instances of the phandle properties ('linux,phandle', 'ibm,phandle',
> > 'phandle') in the live tree.  These properties should not be
> > accessed directly by any code outside of the device tree framework
> > since the phandle is located in the struct device_node.  A quick
> > grep does not show any such accesses of the phandle properties,
> > but I want to look more closely.
> 
> After reading through a bit of code, I am confident that I can
> modify the resolver code to not modify the various phandle
> property values.  There are a few tentacles reaching out to
> other areas that I will have to fix also.  The biggest task
> for me will be to create some good test code.
> 
> I'll be unavailable this week, so I'll start on it in about
> a week.

Ok. No worries from my side. I'm interested to see how random properties
will be "corrected" without modifying them. I can only assume the plan
is to copy those properties.

The kbuild robot found more cases where marking this as const causes
issues. Please see the updated patch inline.

-8<-
From: Stephen Boyd 
Subject: [PATCH] of: Mark property::value as const

The 'blob' we pass into populate_properties() is marked as const,
but we cast that const away when we assign the result of
fdt_getprop_by_offset() to pp->value. Let's mark value as const
instead, so that code can't mistakenly write to the value of the
property that we've so far advertised as const.

Unfortunately, this exposes a problem with the fdt resolver code,
where we overwrite the value member of properties of phandles to
update them with their final value. Add a comment for now to
indicate where we're potentially writing over const data.

You can see such the problem here by loading an overlay dtb into
the kernel via the fw helper method (not direct loading) and
passing that tree to the resolver on an arm64 device. In this
case, the firmware data is vmapped with KERNEL_PAGE_RO and the
resolver code crashes when attempting to write to blob to update
the phandle properties.

Signed-off-by: Stephen Boyd 
---
 drivers/of/base.c |  2 +-
 drivers/of/fdt.c  | 12 ++--
 drivers/of/pdt.c  |  9 +
 drivers/of/resolver.c |  3 +++
 fs/openpromfs/inode.c |  2 +-
 include/linux/of.h|  2 +-
 6 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index fb6bb855714e..8e5f29b814c9 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1156,7 +1156,7 @@ EXPORT_SYMBOL_GPL(of_property_count_elems_of_size);
  * property data is too small or too large.
  *
  */
-static void *of_find_property_value_of_size(const struct device_node *np,
+static const void *of_find_property_value_of_size(const struct device_node *np,
const char *propname, u32 min, u32 max, size_t *len)
 {
struct property *prop = of_find_property(np, propname, NULL);
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index c9b5cac03b36..0635ef5dabf3 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -222,7 +222,7 @@ static void populate_properties(const void *blob,
 
pp->name   = (char *)pname;
pp->length = sz;
-   pp->value  = (__be32 *)val;
+   pp->value  = val;
*pprev = pp;
pprev  = >next;
}
@@ -232,6 +232,7 @@ static void populate_properties(const void *blob,
 */
if (!has_name) {
const char *p = 

Re: [RFC/PATCH] of: Mark property::value as const

[Frank Rowand]

Hi Stephen,

On 02/23/17 15:08, Frank Rowand wrote:
> On 02/13/17 18:50, Stephen Boyd wrote:
>> The 'blob' we pass into populate_properties() is marked as const,
>> but we cast that const away when we assign the result of
>> fdt_getprop_by_offset() to pp->value. Let's mark value as const
>> instead, so that code can't mistakenly write to the value of the
>> property that we've so far advertised as const.
> 
> Instead of struct property field value being a pointer into the
> FDT, I would rather copy the data to newly allocated memory and
> have value be a pointer to that memory.  This is required if we
> want to make /sys/firmware/fdt optional, which would allow us to
> free the memory containing the initial boot FDT.
> 
> I also do not want overlay live subtrees to have any pointers
> into the FDT that was used to populate the overlay, so copying
> the data solves that problem also.
> 
> 
>> Unfortunately, this exposes a problem with the fdt resolver code,
>> where we overwrite the value member of properties of phandles to
>> update them with their final value. Add a comment for now to
>> indicate where we're potentially writing over const data.
> 
> Yes, the resolver code needs to adjust phandle values.
> 
> I think I can get rid of the resolver modifying the various phandle
> values, and instead just modify the phandle value in struct
> device_node.  At the same time, I think I can also remove all
> instances of the phandle properties ('linux,phandle', 'ibm,phandle',
> 'phandle') in the live tree.  These properties should not be
> accessed directly by any code outside of the device tree framework
> since the phandle is located in the struct device_node.  A quick
> grep does not show any such accesses of the phandle properties,
> but I want to look more closely.

After reading through a bit of code, I am confident that I can
modify the resolver code to not modify the various phandle
property values.  There are a few tentacles reaching out to
other areas that I will have to fix also.  The biggest task
for me will be to create some good test code.

I'll be unavailable this week, so I'll start on it in about
a week.

-Frank

Re: [RFC/PATCH] of: Mark property::value as const

[Frank Rowand]

Hi Stephen,

On 02/23/17 15:08, Frank Rowand wrote:
> On 02/13/17 18:50, Stephen Boyd wrote:
>> The 'blob' we pass into populate_properties() is marked as const,
>> but we cast that const away when we assign the result of
>> fdt_getprop_by_offset() to pp->value. Let's mark value as const
>> instead, so that code can't mistakenly write to the value of the
>> property that we've so far advertised as const.
> 
> Instead of struct property field value being a pointer into the
> FDT, I would rather copy the data to newly allocated memory and
> have value be a pointer to that memory.  This is required if we
> want to make /sys/firmware/fdt optional, which would allow us to
> free the memory containing the initial boot FDT.
> 
> I also do not want overlay live subtrees to have any pointers
> into the FDT that was used to populate the overlay, so copying
> the data solves that problem also.
> 
> 
>> Unfortunately, this exposes a problem with the fdt resolver code,
>> where we overwrite the value member of properties of phandles to
>> update them with their final value. Add a comment for now to
>> indicate where we're potentially writing over const data.
> 
> Yes, the resolver code needs to adjust phandle values.
> 
> I think I can get rid of the resolver modifying the various phandle
> values, and instead just modify the phandle value in struct
> device_node.  At the same time, I think I can also remove all
> instances of the phandle properties ('linux,phandle', 'ibm,phandle',
> 'phandle') in the live tree.  These properties should not be
> accessed directly by any code outside of the device tree framework
> since the phandle is located in the struct device_node.  A quick
> grep does not show any such accesses of the phandle properties,
> but I want to look more closely.

After reading through a bit of code, I am confident that I can
modify the resolver code to not modify the various phandle
property values.  There are a few tentacles reaching out to
other areas that I will have to fix also.  The biggest task
for me will be to create some good test code.

I'll be unavailable this week, so I'll start on it in about
a week.

-Frank

Re: [RFC/PATCH] of: Mark property::value as const

[Rob Herring]

On Thu, Feb 23, 2017 at 5:08 PM, Frank Rowand  wrote:
> On 02/13/17 18:50, Stephen Boyd wrote:
>> The 'blob' we pass into populate_properties() is marked as const,
>> but we cast that const away when we assign the result of
>> fdt_getprop_by_offset() to pp->value. Let's mark value as const
>> instead, so that code can't mistakenly write to the value of the
>> property that we've so far advertised as const.
>
> Instead of struct property field value being a pointer into the
> FDT, I would rather copy the data to newly allocated memory and
> have value be a pointer to that memory.  This is required if we
> want to make /sys/firmware/fdt optional, which would allow us to
> free the memory containing the initial boot FDT.

Making a copy would simplify several things.

> I also do not want overlay live subtrees to have any pointers
> into the FDT that was used to populate the overlay, so copying
> the data solves that problem also.
>
>
>> Unfortunately, this exposes a problem with the fdt resolver code,
>> where we overwrite the value member of properties of phandles to
>> update them with their final value. Add a comment for now to
>> indicate where we're potentially writing over const data.
>
> Yes, the resolver code needs to adjust phandle values.
>
> I think I can get rid of the resolver modifying the various phandle
> values, and instead just modify the phandle value in struct
> device_node.  At the same time, I think I can also remove all
> instances of the phandle properties ('linux,phandle', 'ibm,phandle',
> 'phandle') in the live tree.  These properties should not be
> accessed directly by any code outside of the device tree framework
> since the phandle is located in the struct device_node.  A quick
> grep does not show any such accesses of the phandle properties,
> but I want to look more closely.

Good idea.

BTW, I recently noticed that dtc by default generates both
linux,phandle and phandle properties. I would think the new one has
been around long enough that we can turn off the old one by default
now.

Rob

Re: [RFC/PATCH] of: Mark property::value as const

[Rob Herring]

On Thu, Feb 23, 2017 at 5:08 PM, Frank Rowand  wrote:
> On 02/13/17 18:50, Stephen Boyd wrote:
>> The 'blob' we pass into populate_properties() is marked as const,
>> but we cast that const away when we assign the result of
>> fdt_getprop_by_offset() to pp->value. Let's mark value as const
>> instead, so that code can't mistakenly write to the value of the
>> property that we've so far advertised as const.
>
> Instead of struct property field value being a pointer into the
> FDT, I would rather copy the data to newly allocated memory and
> have value be a pointer to that memory.  This is required if we
> want to make /sys/firmware/fdt optional, which would allow us to
> free the memory containing the initial boot FDT.

Making a copy would simplify several things.

> I also do not want overlay live subtrees to have any pointers
> into the FDT that was used to populate the overlay, so copying
> the data solves that problem also.
>
>
>> Unfortunately, this exposes a problem with the fdt resolver code,
>> where we overwrite the value member of properties of phandles to
>> update them with their final value. Add a comment for now to
>> indicate where we're potentially writing over const data.
>
> Yes, the resolver code needs to adjust phandle values.
>
> I think I can get rid of the resolver modifying the various phandle
> values, and instead just modify the phandle value in struct
> device_node.  At the same time, I think I can also remove all
> instances of the phandle properties ('linux,phandle', 'ibm,phandle',
> 'phandle') in the live tree.  These properties should not be
> accessed directly by any code outside of the device tree framework
> since the phandle is located in the struct device_node.  A quick
> grep does not show any such accesses of the phandle properties,
> but I want to look more closely.

Good idea.

BTW, I recently noticed that dtc by default generates both
linux,phandle and phandle properties. I would think the new one has
been around long enough that we can turn off the old one by default
now.

Rob

Re: [RFC/PATCH] of: Mark property::value as const

[Frank Rowand]

On 02/23/17 15:08, Frank Rowand wrote:
> On 02/13/17 18:50, Stephen Boyd wrote:
>> The 'blob' we pass into populate_properties() is marked as const,
>> but we cast that const away when we assign the result of
>> fdt_getprop_by_offset() to pp->value. Let's mark value as const
>> instead, so that code can't mistakenly write to the value of the
>> property that we've so far advertised as const.
> 
> Instead of struct property field value being a pointer into the
> FDT, I would rather copy the data to newly allocated memory and
> have value be a pointer to that memory.  This is required if we
> want to make /sys/firmware/fdt optional, which would allow us to
> free the memory containing the initial boot FDT.
> 
> I also do not want overlay live subtrees to have any pointers
> into the FDT that was used to populate the overlay, so copying
> the data solves that problem also.
> 
> 
>> Unfortunately, this exposes a problem with the fdt resolver code,
>> where we overwrite the value member of properties of phandles to
>> update them with their final value. Add a comment for now to
>> indicate where we're potentially writing over const data.
> 
> Yes, the resolver code needs to adjust phandle values.
> 
> I think I can get rid of the resolver modifying the various phandle
> values, and instead just modify the phandle value in struct
> device_node.  At the same time, I think I can also remove all
> instances of the phandle properties ('linux,phandle', 'ibm,phandle',
> 'phandle') in the live tree.  These properties should not be
> accessed directly by any code outside of the device tree framework
> since the phandle is located in the struct device_node.  A quick
> grep does not show any such accesses of the phandle properties,
> but I want to look more closely.

If I remove the various phandle properties from the live tree,
the one place I can not inspect for impact is the live tree that
is exposed at /proc/device-tree/  I do not know whether that is
a problem or not.

-Frank

Re: [RFC/PATCH] of: Mark property::value as const

[Frank Rowand]

On 02/23/17 15:08, Frank Rowand wrote:
> On 02/13/17 18:50, Stephen Boyd wrote:
>> The 'blob' we pass into populate_properties() is marked as const,
>> but we cast that const away when we assign the result of
>> fdt_getprop_by_offset() to pp->value. Let's mark value as const
>> instead, so that code can't mistakenly write to the value of the
>> property that we've so far advertised as const.
> 
> Instead of struct property field value being a pointer into the
> FDT, I would rather copy the data to newly allocated memory and
> have value be a pointer to that memory.  This is required if we
> want to make /sys/firmware/fdt optional, which would allow us to
> free the memory containing the initial boot FDT.
> 
> I also do not want overlay live subtrees to have any pointers
> into the FDT that was used to populate the overlay, so copying
> the data solves that problem also.
> 
> 
>> Unfortunately, this exposes a problem with the fdt resolver code,
>> where we overwrite the value member of properties of phandles to
>> update them with their final value. Add a comment for now to
>> indicate where we're potentially writing over const data.
> 
> Yes, the resolver code needs to adjust phandle values.
> 
> I think I can get rid of the resolver modifying the various phandle
> values, and instead just modify the phandle value in struct
> device_node.  At the same time, I think I can also remove all
> instances of the phandle properties ('linux,phandle', 'ibm,phandle',
> 'phandle') in the live tree.  These properties should not be
> accessed directly by any code outside of the device tree framework
> since the phandle is located in the struct device_node.  A quick
> grep does not show any such accesses of the phandle properties,
> but I want to look more closely.

If I remove the various phandle properties from the live tree,
the one place I can not inspect for impact is the live tree that
is exposed at /proc/device-tree/  I do not know whether that is
a problem or not.

-Frank

Re: [RFC/PATCH] of: Mark property::value as const

[Frank Rowand]

On 02/23/17 14:09, Rob Herring wrote:
> On Thu, Feb 23, 2017 at 2:58 PM, Frank Rowand  wrote:
>> On 02/23/17 11:54, Frank Rowand wrote:
>>> On 02/13/17 18:50, Stephen Boyd wrote:
 The 'blob' we pass into populate_properties() is marked as const,
 but we cast that const away when we assign the result of
 fdt_getprop_by_offset() to pp->value. Let's mark value as const
 instead, so that code can't mistakenly write to the value of the
 property that we've so far advertised as const.

 Unfortunately, this exposes a problem with the fdt resolver code,
 where we overwrite the value member of properties of phandles to
 update them with their final value. Add a comment for now to
 indicate where we're potentially writing over const data.
>>>
>>> The resolver should not be over writing anything in the FDT.  I'll
>>> look at what is going on there.
>>>
>>> The FDT we expose to user space should be the FDT we booted with,
>>> not something later modified.
>>
>> It seems that /sys/firmware/fdt is not documented.  I'll look into
>> fixing that.
> 
> That's because the "official" interface is /proc/device-tree/ which is
> now a symlink. IIRC, it is documented to use /proc/device-tree, not
> the sysfs path.
> 
> Rob
> .
> 


Documentation/ABI/testing/sysfs-firmware-ofw describes
/sys/firmware/devicetree/* but not /sys/firmware/fdt


It also describes the /proc/device-tree symlink to
/sys/firmware/devicetree/base as the official
stable ABI, as you mentioned.

So it is just fdt that I have not found the documentation
for.

-Frank

Re: [RFC/PATCH] of: Mark property::value as const

[Frank Rowand]

On 02/23/17 14:09, Rob Herring wrote:
> On Thu, Feb 23, 2017 at 2:58 PM, Frank Rowand  wrote:
>> On 02/23/17 11:54, Frank Rowand wrote:
>>> On 02/13/17 18:50, Stephen Boyd wrote:
 The 'blob' we pass into populate_properties() is marked as const,
 but we cast that const away when we assign the result of
 fdt_getprop_by_offset() to pp->value. Let's mark value as const
 instead, so that code can't mistakenly write to the value of the
 property that we've so far advertised as const.

 Unfortunately, this exposes a problem with the fdt resolver code,
 where we overwrite the value member of properties of phandles to
 update them with their final value. Add a comment for now to
 indicate where we're potentially writing over const data.
>>>
>>> The resolver should not be over writing anything in the FDT.  I'll
>>> look at what is going on there.
>>>
>>> The FDT we expose to user space should be the FDT we booted with,
>>> not something later modified.
>>
>> It seems that /sys/firmware/fdt is not documented.  I'll look into
>> fixing that.
> 
> That's because the "official" interface is /proc/device-tree/ which is
> now a symlink. IIRC, it is documented to use /proc/device-tree, not
> the sysfs path.
> 
> Rob
> .
> 


Documentation/ABI/testing/sysfs-firmware-ofw describes
/sys/firmware/devicetree/* but not /sys/firmware/fdt


It also describes the /proc/device-tree symlink to
/sys/firmware/devicetree/base as the official
stable ABI, as you mentioned.

So it is just fdt that I have not found the documentation
for.

-Frank

Re: [RFC/PATCH] of: Mark property::value as const

[Frank Rowand]

On 02/13/17 18:50, Stephen Boyd wrote:
> The 'blob' we pass into populate_properties() is marked as const,
> but we cast that const away when we assign the result of
> fdt_getprop_by_offset() to pp->value. Let's mark value as const
> instead, so that code can't mistakenly write to the value of the
> property that we've so far advertised as const.

Instead of struct property field value being a pointer into the
FDT, I would rather copy the data to newly allocated memory and
have value be a pointer to that memory.  This is required if we
want to make /sys/firmware/fdt optional, which would allow us to
free the memory containing the initial boot FDT.

I also do not want overlay live subtrees to have any pointers
into the FDT that was used to populate the overlay, so copying
the data solves that problem also.


> Unfortunately, this exposes a problem with the fdt resolver code,
> where we overwrite the value member of properties of phandles to
> update them with their final value. Add a comment for now to
> indicate where we're potentially writing over const data.

Yes, the resolver code needs to adjust phandle values.

I think I can get rid of the resolver modifying the various phandle
values, and instead just modify the phandle value in struct
device_node.  At the same time, I think I can also remove all
instances of the phandle properties ('linux,phandle', 'ibm,phandle',
'phandle') in the live tree.  These properties should not be
accessed directly by any code outside of the device tree framework
since the phandle is located in the struct device_node.  A quick
grep does not show any such accesses of the phandle properties,
but I want to look more closely.


> 
> You can see the problem here by loading an overlay dtb into
> the kernel via the request firmware helper method (not direct
> loading) and then passing that tree to the resolver on an arm64
> device. In this case, the firmware data is vmapped with KERNEL_PAGE_RO
> and the code crashes when attempting to write to the blob to update
> the phandle properties.
> 
> Signed-off-by: Stephen Boyd 
> ---
> 
> I was thinking perhaps it would work to store another __be32 variant
> of the phandle in each device node, but then we still have a problem

That would complicate the overlay code.  Once adjustments are done to
the phandle property value, the overlay code does not need to special
case phandle - phandle is just another property.  I am hoping I can
instead modify the resolver code as I described above.


> with properties that have phandles inside them at some offset that we
> need to update. I guess the only real solution is to deep copy the
> property in that case and then save around some info to free the
> duplicated property later on?
> 
>  drivers/of/base.c |  2 +-
>  drivers/of/fdt.c  | 12 ++--
>  drivers/of/resolver.c |  3 +++
>  include/linux/of.h|  2 +-
>  4 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index fb6bb855714e..8e5f29b814c9 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1156,7 +1156,7 @@ EXPORT_SYMBOL_GPL(of_property_count_elems_of_size);
>   * property data is too small or too large.
>   *
>   */
> -static void *of_find_property_value_of_size(const struct device_node *np,
> +static const void *of_find_property_value_of_size(const struct device_node 
> *np,
>   const char *propname, u32 min, u32 max, size_t *len)
>  {
>   struct property *prop = of_find_property(np, propname, NULL);
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index c9b5cac03b36..0635ef5dabf3 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -222,7 +222,7 @@ static void populate_properties(const void *blob,
>  
>   pp->name   = (char *)pname;
>   pp->length = sz;
> - pp->value  = (__be32 *)val;
> + pp->value  = val;
>   *pprev = pp;
>   pprev  = >next;
>   }
> @@ -232,6 +232,7 @@ static void populate_properties(const void *blob,
>*/
>   if (!has_name) {
>   const char *p = nodename, *ps = p, *pa = NULL;
> + char *b;
>   int len;
>  
>   while (*p) {
> @@ -250,13 +251,12 @@ static void populate_properties(const void *blob,
>   if (!dryrun) {
>   pp->name   = "name";
>   pp->length = len;
> - pp->value  = pp + 1;
> + pp->value  = b = (char *)(pp + 1);

I would prefer not to hide an assignment in the middle of a statement.


>   *pprev = pp;
>   pprev  = >next;
> - memcpy(pp->value, ps, len - 1);
> - ((char *)pp->value)[len - 1] = 0;
> - pr_debug("fixed up name for %s -> %s\n",
> -  nodename, (char *)pp->value);
> + 

Re: [RFC/PATCH] of: Mark property::value as const

[Frank Rowand]

On 02/13/17 18:50, Stephen Boyd wrote:
> The 'blob' we pass into populate_properties() is marked as const,
> but we cast that const away when we assign the result of
> fdt_getprop_by_offset() to pp->value. Let's mark value as const
> instead, so that code can't mistakenly write to the value of the
> property that we've so far advertised as const.

Instead of struct property field value being a pointer into the
FDT, I would rather copy the data to newly allocated memory and
have value be a pointer to that memory.  This is required if we
want to make /sys/firmware/fdt optional, which would allow us to
free the memory containing the initial boot FDT.

I also do not want overlay live subtrees to have any pointers
into the FDT that was used to populate the overlay, so copying
the data solves that problem also.


> Unfortunately, this exposes a problem with the fdt resolver code,
> where we overwrite the value member of properties of phandles to
> update them with their final value. Add a comment for now to
> indicate where we're potentially writing over const data.

Yes, the resolver code needs to adjust phandle values.

I think I can get rid of the resolver modifying the various phandle
values, and instead just modify the phandle value in struct
device_node.  At the same time, I think I can also remove all
instances of the phandle properties ('linux,phandle', 'ibm,phandle',
'phandle') in the live tree.  These properties should not be
accessed directly by any code outside of the device tree framework
since the phandle is located in the struct device_node.  A quick
grep does not show any such accesses of the phandle properties,
but I want to look more closely.


> 
> You can see the problem here by loading an overlay dtb into
> the kernel via the request firmware helper method (not direct
> loading) and then passing that tree to the resolver on an arm64
> device. In this case, the firmware data is vmapped with KERNEL_PAGE_RO
> and the code crashes when attempting to write to the blob to update
> the phandle properties.
> 
> Signed-off-by: Stephen Boyd 
> ---
> 
> I was thinking perhaps it would work to store another __be32 variant
> of the phandle in each device node, but then we still have a problem

That would complicate the overlay code.  Once adjustments are done to
the phandle property value, the overlay code does not need to special
case phandle - phandle is just another property.  I am hoping I can
instead modify the resolver code as I described above.


> with properties that have phandles inside them at some offset that we
> need to update. I guess the only real solution is to deep copy the
> property in that case and then save around some info to free the
> duplicated property later on?
> 
>  drivers/of/base.c |  2 +-
>  drivers/of/fdt.c  | 12 ++--
>  drivers/of/resolver.c |  3 +++
>  include/linux/of.h|  2 +-
>  4 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index fb6bb855714e..8e5f29b814c9 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1156,7 +1156,7 @@ EXPORT_SYMBOL_GPL(of_property_count_elems_of_size);
>   * property data is too small or too large.
>   *
>   */
> -static void *of_find_property_value_of_size(const struct device_node *np,
> +static const void *of_find_property_value_of_size(const struct device_node 
> *np,
>   const char *propname, u32 min, u32 max, size_t *len)
>  {
>   struct property *prop = of_find_property(np, propname, NULL);
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index c9b5cac03b36..0635ef5dabf3 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -222,7 +222,7 @@ static void populate_properties(const void *blob,
>  
>   pp->name   = (char *)pname;
>   pp->length = sz;
> - pp->value  = (__be32 *)val;
> + pp->value  = val;
>   *pprev = pp;
>   pprev  = >next;
>   }
> @@ -232,6 +232,7 @@ static void populate_properties(const void *blob,
>*/
>   if (!has_name) {
>   const char *p = nodename, *ps = p, *pa = NULL;
> + char *b;
>   int len;
>  
>   while (*p) {
> @@ -250,13 +251,12 @@ static void populate_properties(const void *blob,
>   if (!dryrun) {
>   pp->name   = "name";
>   pp->length = len;
> - pp->value  = pp + 1;
> + pp->value  = b = (char *)(pp + 1);

I would prefer not to hide an assignment in the middle of a statement.


>   *pprev = pp;
>   pprev  = >next;
> - memcpy(pp->value, ps, len - 1);
> - ((char *)pp->value)[len - 1] = 0;
> - pr_debug("fixed up name for %s -> %s\n",
> -  nodename, (char *)pp->value);
> + memcpy(b, ps, len - 1);
> +   

Re: [RFC/PATCH] of: Mark property::value as const

[Rob Herring]

On Thu, Feb 23, 2017 at 2:58 PM, Frank Rowand  wrote:
> On 02/23/17 11:54, Frank Rowand wrote:
>> On 02/13/17 18:50, Stephen Boyd wrote:
>>> The 'blob' we pass into populate_properties() is marked as const,
>>> but we cast that const away when we assign the result of
>>> fdt_getprop_by_offset() to pp->value. Let's mark value as const
>>> instead, so that code can't mistakenly write to the value of the
>>> property that we've so far advertised as const.
>>>
>>> Unfortunately, this exposes a problem with the fdt resolver code,
>>> where we overwrite the value member of properties of phandles to
>>> update them with their final value. Add a comment for now to
>>> indicate where we're potentially writing over const data.
>>
>> The resolver should not be over writing anything in the FDT.  I'll
>> look at what is going on there.
>>
>> The FDT we expose to user space should be the FDT we booted with,
>> not something later modified.
>
> It seems that /sys/firmware/fdt is not documented.  I'll look into
> fixing that.

That's because the "official" interface is /proc/device-tree/ which is
now a symlink. IIRC, it is documented to use /proc/device-tree, not
the sysfs path.

Rob

Re: [RFC/PATCH] of: Mark property::value as const

[Rob Herring]

On Thu, Feb 23, 2017 at 2:58 PM, Frank Rowand  wrote:
> On 02/23/17 11:54, Frank Rowand wrote:
>> On 02/13/17 18:50, Stephen Boyd wrote:
>>> The 'blob' we pass into populate_properties() is marked as const,
>>> but we cast that const away when we assign the result of
>>> fdt_getprop_by_offset() to pp->value. Let's mark value as const
>>> instead, so that code can't mistakenly write to the value of the
>>> property that we've so far advertised as const.
>>>
>>> Unfortunately, this exposes a problem with the fdt resolver code,
>>> where we overwrite the value member of properties of phandles to
>>> update them with their final value. Add a comment for now to
>>> indicate where we're potentially writing over const data.
>>
>> The resolver should not be over writing anything in the FDT.  I'll
>> look at what is going on there.
>>
>> The FDT we expose to user space should be the FDT we booted with,
>> not something later modified.
>
> It seems that /sys/firmware/fdt is not documented.  I'll look into
> fixing that.

That's because the "official" interface is /proc/device-tree/ which is
now a symlink. IIRC, it is documented to use /proc/device-tree, not
the sysfs path.

Rob

Re: [RFC/PATCH] of: Mark property::value as const

[Frank Rowand]

On 02/23/17 11:54, Frank Rowand wrote:
> On 02/13/17 18:50, Stephen Boyd wrote:
>> The 'blob' we pass into populate_properties() is marked as const,
>> but we cast that const away when we assign the result of
>> fdt_getprop_by_offset() to pp->value. Let's mark value as const
>> instead, so that code can't mistakenly write to the value of the
>> property that we've so far advertised as const.
>>
>> Unfortunately, this exposes a problem with the fdt resolver code,
>> where we overwrite the value member of properties of phandles to
>> update them with their final value. Add a comment for now to
>> indicate where we're potentially writing over const data.
> 
> The resolver should not be over writing anything in the FDT.  I'll
> look at what is going on there.

OK, now that I have looked at the resolver, I see that you meant that
the resolver code is modifying the _overlay_ FDT, not the FDT that
was used at initial boot.  So not what I thought you meant.

I'll reply separately to the original patch email with a more complete
response.

-Frank

> 
> The FDT we expose to user space should be the FDT we booted with,
> not something later modified.
> 
> -Frank
> 
>>
>> You can see the problem here by loading an overlay dtb into
>> the kernel via the request firmware helper method (not direct
>> loading) and then passing that tree to the resolver on an arm64
>> device. In this case, the firmware data is vmapped with KERNEL_PAGE_RO
>> and the code crashes when attempting to write to the blob to update
>> the phandle properties.
>>
>> Signed-off-by: Stephen Boyd 
>> ---
>>
>> I was thinking perhaps it would work to store another __be32 variant
>> of the phandle in each device node, but then we still have a problem
>> with properties that have phandles inside them at some offset that we
>> need to update. I guess the only real solution is to deep copy the
>> property in that case and then save around some info to free the
>> duplicated property later on?
>>
>>  drivers/of/base.c |  2 +-
>>  drivers/of/fdt.c  | 12 ++--
>>  drivers/of/resolver.c |  3 +++
>>  include/linux/of.h|  2 +-
>>  4 files changed, 11 insertions(+), 8 deletions(-)
> 
> < snip >
> 

Re: [RFC/PATCH] of: Mark property::value as const

[Frank Rowand]

On 02/23/17 11:54, Frank Rowand wrote:
> On 02/13/17 18:50, Stephen Boyd wrote:
>> The 'blob' we pass into populate_properties() is marked as const,
>> but we cast that const away when we assign the result of
>> fdt_getprop_by_offset() to pp->value. Let's mark value as const
>> instead, so that code can't mistakenly write to the value of the
>> property that we've so far advertised as const.
>>
>> Unfortunately, this exposes a problem with the fdt resolver code,
>> where we overwrite the value member of properties of phandles to
>> update them with their final value. Add a comment for now to
>> indicate where we're potentially writing over const data.
> 
> The resolver should not be over writing anything in the FDT.  I'll
> look at what is going on there.

OK, now that I have looked at the resolver, I see that you meant that
the resolver code is modifying the _overlay_ FDT, not the FDT that
was used at initial boot.  So not what I thought you meant.

I'll reply separately to the original patch email with a more complete
response.

-Frank

> 
> The FDT we expose to user space should be the FDT we booted with,
> not something later modified.
> 
> -Frank
> 
>>
>> You can see the problem here by loading an overlay dtb into
>> the kernel via the request firmware helper method (not direct
>> loading) and then passing that tree to the resolver on an arm64
>> device. In this case, the firmware data is vmapped with KERNEL_PAGE_RO
>> and the code crashes when attempting to write to the blob to update
>> the phandle properties.
>>
>> Signed-off-by: Stephen Boyd 
>> ---
>>
>> I was thinking perhaps it would work to store another __be32 variant
>> of the phandle in each device node, but then we still have a problem
>> with properties that have phandles inside them at some offset that we
>> need to update. I guess the only real solution is to deep copy the
>> property in that case and then save around some info to free the
>> duplicated property later on?
>>
>>  drivers/of/base.c |  2 +-
>>  drivers/of/fdt.c  | 12 ++--
>>  drivers/of/resolver.c |  3 +++
>>  include/linux/of.h|  2 +-
>>  4 files changed, 11 insertions(+), 8 deletions(-)
> 
> < snip >
> 

Re: [RFC/PATCH] of: Mark property::value as const

[Frank Rowand]

On 02/23/17 11:54, Frank Rowand wrote:
> On 02/13/17 18:50, Stephen Boyd wrote:
>> The 'blob' we pass into populate_properties() is marked as const,
>> but we cast that const away when we assign the result of
>> fdt_getprop_by_offset() to pp->value. Let's mark value as const
>> instead, so that code can't mistakenly write to the value of the
>> property that we've so far advertised as const.
>>
>> Unfortunately, this exposes a problem with the fdt resolver code,
>> where we overwrite the value member of properties of phandles to
>> update them with their final value. Add a comment for now to
>> indicate where we're potentially writing over const data.
> 
> The resolver should not be over writing anything in the FDT.  I'll
> look at what is going on there.
> 
> The FDT we expose to user space should be the FDT we booted with,
> not something later modified.

It seems that /sys/firmware/fdt is not documented.  I'll look into
fixing that.

-Frank

> 
> -Frank
> 
>>
>> You can see the problem here by loading an overlay dtb into
>> the kernel via the request firmware helper method (not direct
>> loading) and then passing that tree to the resolver on an arm64
>> device. In this case, the firmware data is vmapped with KERNEL_PAGE_RO
>> and the code crashes when attempting to write to the blob to update
>> the phandle properties.
>>
>> Signed-off-by: Stephen Boyd 
>> ---
>>
>> I was thinking perhaps it would work to store another __be32 variant
>> of the phandle in each device node, but then we still have a problem
>> with properties that have phandles inside them at some offset that we
>> need to update. I guess the only real solution is to deep copy the
>> property in that case and then save around some info to free the
>> duplicated property later on?
>>
>>  drivers/of/base.c |  2 +-
>>  drivers/of/fdt.c  | 12 ++--
>>  drivers/of/resolver.c |  3 +++
>>  include/linux/of.h|  2 +-
>>  4 files changed, 11 insertions(+), 8 deletions(-)
> 
> < snip >
> 

Re: [RFC/PATCH] of: Mark property::value as const

[Frank Rowand]

On 02/23/17 11:54, Frank Rowand wrote:
> On 02/13/17 18:50, Stephen Boyd wrote:
>> The 'blob' we pass into populate_properties() is marked as const,
>> but we cast that const away when we assign the result of
>> fdt_getprop_by_offset() to pp->value. Let's mark value as const
>> instead, so that code can't mistakenly write to the value of the
>> property that we've so far advertised as const.
>>
>> Unfortunately, this exposes a problem with the fdt resolver code,
>> where we overwrite the value member of properties of phandles to
>> update them with their final value. Add a comment for now to
>> indicate where we're potentially writing over const data.
> 
> The resolver should not be over writing anything in the FDT.  I'll
> look at what is going on there.
> 
> The FDT we expose to user space should be the FDT we booted with,
> not something later modified.

It seems that /sys/firmware/fdt is not documented.  I'll look into
fixing that.

-Frank

> 
> -Frank
> 
>>
>> You can see the problem here by loading an overlay dtb into
>> the kernel via the request firmware helper method (not direct
>> loading) and then passing that tree to the resolver on an arm64
>> device. In this case, the firmware data is vmapped with KERNEL_PAGE_RO
>> and the code crashes when attempting to write to the blob to update
>> the phandle properties.
>>
>> Signed-off-by: Stephen Boyd 
>> ---
>>
>> I was thinking perhaps it would work to store another __be32 variant
>> of the phandle in each device node, but then we still have a problem
>> with properties that have phandles inside them at some offset that we
>> need to update. I guess the only real solution is to deep copy the
>> property in that case and then save around some info to free the
>> duplicated property later on?
>>
>>  drivers/of/base.c |  2 +-
>>  drivers/of/fdt.c  | 12 ++--
>>  drivers/of/resolver.c |  3 +++
>>  include/linux/of.h|  2 +-
>>  4 files changed, 11 insertions(+), 8 deletions(-)
> 
> < snip >
> 

Re: [RFC/PATCH] of: Mark property::value as const

[Frank Rowand]

On 02/13/17 18:50, Stephen Boyd wrote:
> The 'blob' we pass into populate_properties() is marked as const,
> but we cast that const away when we assign the result of
> fdt_getprop_by_offset() to pp->value. Let's mark value as const
> instead, so that code can't mistakenly write to the value of the
> property that we've so far advertised as const.
> 
> Unfortunately, this exposes a problem with the fdt resolver code,
> where we overwrite the value member of properties of phandles to
> update them with their final value. Add a comment for now to
> indicate where we're potentially writing over const data.

The resolver should not be over writing anything in the FDT.  I'll
look at what is going on there.

The FDT we expose to user space should be the FDT we booted with,
not something later modified.

-Frank

> 
> You can see the problem here by loading an overlay dtb into
> the kernel via the request firmware helper method (not direct
> loading) and then passing that tree to the resolver on an arm64
> device. In this case, the firmware data is vmapped with KERNEL_PAGE_RO
> and the code crashes when attempting to write to the blob to update
> the phandle properties.
> 
> Signed-off-by: Stephen Boyd 
> ---
> 
> I was thinking perhaps it would work to store another __be32 variant
> of the phandle in each device node, but then we still have a problem
> with properties that have phandles inside them at some offset that we
> need to update. I guess the only real solution is to deep copy the
> property in that case and then save around some info to free the
> duplicated property later on?
> 
>  drivers/of/base.c |  2 +-
>  drivers/of/fdt.c  | 12 ++--
>  drivers/of/resolver.c |  3 +++
>  include/linux/of.h|  2 +-
>  4 files changed, 11 insertions(+), 8 deletions(-)

< snip >

Re: [RFC/PATCH] of: Mark property::value as const

[Frank Rowand]

On 02/13/17 18:50, Stephen Boyd wrote:
> The 'blob' we pass into populate_properties() is marked as const,
> but we cast that const away when we assign the result of
> fdt_getprop_by_offset() to pp->value. Let's mark value as const
> instead, so that code can't mistakenly write to the value of the
> property that we've so far advertised as const.
> 
> Unfortunately, this exposes a problem with the fdt resolver code,
> where we overwrite the value member of properties of phandles to
> update them with their final value. Add a comment for now to
> indicate where we're potentially writing over const data.

The resolver should not be over writing anything in the FDT.  I'll
look at what is going on there.

The FDT we expose to user space should be the FDT we booted with,
not something later modified.

-Frank

> 
> You can see the problem here by loading an overlay dtb into
> the kernel via the request firmware helper method (not direct
> loading) and then passing that tree to the resolver on an arm64
> device. In this case, the firmware data is vmapped with KERNEL_PAGE_RO
> and the code crashes when attempting to write to the blob to update
> the phandle properties.
> 
> Signed-off-by: Stephen Boyd 
> ---
> 
> I was thinking perhaps it would work to store another __be32 variant
> of the phandle in each device node, but then we still have a problem
> with properties that have phandles inside them at some offset that we
> need to update. I guess the only real solution is to deep copy the
> property in that case and then save around some info to free the
> duplicated property later on?
> 
>  drivers/of/base.c |  2 +-
>  drivers/of/fdt.c  | 12 ++--
>  drivers/of/resolver.c |  3 +++
>  include/linux/of.h|  2 +-
>  4 files changed, 11 insertions(+), 8 deletions(-)

< snip >

Re: [RFC/PATCH] of: Mark property::value as const

[Rob Herring]

On Mon, Feb 13, 2017 at 8:50 PM, Stephen Boyd  wrote:
> The 'blob' we pass into populate_properties() is marked as const,
> but we cast that const away when we assign the result of
> fdt_getprop_by_offset() to pp->value. Let's mark value as const
> instead, so that code can't mistakenly write to the value of the
> property that we've so far advertised as const.
>
> Unfortunately, this exposes a problem with the fdt resolver code,
> where we overwrite the value member of properties of phandles to
> update them with their final value. Add a comment for now to
> indicate where we're potentially writing over const data.
>
> You can see the problem here by loading an overlay dtb into
> the kernel via the request firmware helper method (not direct
> loading) and then passing that tree to the resolver on an arm64
> device. In this case, the firmware data is vmapped with KERNEL_PAGE_RO
> and the code crashes when attempting to write to the blob to update
> the phandle properties.

This problem exists with or without this patch, right?

> Signed-off-by: Stephen Boyd 
> ---
>
> I was thinking perhaps it would work to store another __be32 variant
> of the phandle in each device node, but then we still have a problem
> with properties that have phandles inside them at some offset that we
> need to update. I guess the only real solution is to deep copy the
> property in that case and then save around some info to free the
> duplicated property later on?

Couldn't we just copy the overlay and free the firmware blob in this
case? That seems easier than trying to selectively copy things. We
keep the base FDT to pass to userspace and we could need that for
overlays, but not ones we load from userspace.

Rob

Re: [RFC/PATCH] of: Mark property::value as const

[Rob Herring]

On Mon, Feb 13, 2017 at 8:50 PM, Stephen Boyd  wrote:
> The 'blob' we pass into populate_properties() is marked as const,
> but we cast that const away when we assign the result of
> fdt_getprop_by_offset() to pp->value. Let's mark value as const
> instead, so that code can't mistakenly write to the value of the
> property that we've so far advertised as const.
>
> Unfortunately, this exposes a problem with the fdt resolver code,
> where we overwrite the value member of properties of phandles to
> update them with their final value. Add a comment for now to
> indicate where we're potentially writing over const data.
>
> You can see the problem here by loading an overlay dtb into
> the kernel via the request firmware helper method (not direct
> loading) and then passing that tree to the resolver on an arm64
> device. In this case, the firmware data is vmapped with KERNEL_PAGE_RO
> and the code crashes when attempting to write to the blob to update
> the phandle properties.

This problem exists with or without this patch, right?

> Signed-off-by: Stephen Boyd 
> ---
>
> I was thinking perhaps it would work to store another __be32 variant
> of the phandle in each device node, but then we still have a problem
> with properties that have phandles inside them at some offset that we
> need to update. I guess the only real solution is to deep copy the
> property in that case and then save around some info to free the
> duplicated property later on?

Couldn't we just copy the overlay and free the firmware blob in this
case? That seems easier than trying to selectively copy things. We
keep the base FDT to pass to userspace and we could need that for
overlays, but not ones we load from userspace.

Rob

[RFC/PATCH] of: Mark property::value as const

[Stephen Boyd]

The 'blob' we pass into populate_properties() is marked as const,
but we cast that const away when we assign the result of
fdt_getprop_by_offset() to pp->value. Let's mark value as const
instead, so that code can't mistakenly write to the value of the
property that we've so far advertised as const.

Unfortunately, this exposes a problem with the fdt resolver code,
where we overwrite the value member of properties of phandles to
update them with their final value. Add a comment for now to
indicate where we're potentially writing over const data.

You can see the problem here by loading an overlay dtb into
the kernel via the request firmware helper method (not direct
loading) and then passing that tree to the resolver on an arm64
device. In this case, the firmware data is vmapped with KERNEL_PAGE_RO
and the code crashes when attempting to write to the blob to update
the phandle properties.

Signed-off-by: Stephen Boyd 
---

I was thinking perhaps it would work to store another __be32 variant
of the phandle in each device node, but then we still have a problem
with properties that have phandles inside them at some offset that we
need to update. I guess the only real solution is to deep copy the
property in that case and then save around some info to free the
duplicated property later on?

 drivers/of/base.c |  2 +-
 drivers/of/fdt.c  | 12 ++--
 drivers/of/resolver.c |  3 +++
 include/linux/of.h|  2 +-
 4 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index fb6bb855714e..8e5f29b814c9 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1156,7 +1156,7 @@ EXPORT_SYMBOL_GPL(of_property_count_elems_of_size);
  * property data is too small or too large.
  *
  */
-static void *of_find_property_value_of_size(const struct device_node *np,
+static const void *of_find_property_value_of_size(const struct device_node *np,
const char *propname, u32 min, u32 max, size_t *len)
 {
struct property *prop = of_find_property(np, propname, NULL);
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index c9b5cac03b36..0635ef5dabf3 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -222,7 +222,7 @@ static void populate_properties(const void *blob,
 
pp->name   = (char *)pname;
pp->length = sz;
-   pp->value  = (__be32 *)val;
+   pp->value  = val;
*pprev = pp;
pprev  = >next;
}
@@ -232,6 +232,7 @@ static void populate_properties(const void *blob,
 */
if (!has_name) {
const char *p = nodename, *ps = p, *pa = NULL;
+   char *b;
int len;
 
while (*p) {
@@ -250,13 +251,12 @@ static void populate_properties(const void *blob,
if (!dryrun) {
pp->name   = "name";
pp->length = len;
-   pp->value  = pp + 1;
+   pp->value  = b = (char *)(pp + 1);
*pprev = pp;
pprev  = >next;
-   memcpy(pp->value, ps, len - 1);
-   ((char *)pp->value)[len - 1] = 0;
-   pr_debug("fixed up name for %s -> %s\n",
-nodename, (char *)pp->value);
+   memcpy(b, ps, len - 1);
+   b[len - 1] = 0;
+   pr_debug("fixed up name for %s -> %s\n", nodename, b);
}
}
 
diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 8bf12e904fd2..6d88f8100318 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -93,6 +93,7 @@ static void adjust_overlay_phandles(struct device_node 
*overlay,
if (phandle == OF_PHANDLE_ILLEGAL)
continue;
 
+   /* This is bad because we cast away const */
*(uint32_t *)prop->value = cpu_to_be32(overlay->phandle);
}
 
@@ -154,6 +155,7 @@ static int update_usages_of_a_phandle_reference(struct 
device_node *overlay,
goto err_fail;
}
 
+   /* This is bad because we cast away const */
*(__be32 *)(prop->value + offset) = cpu_to_be32(phandle);
}
 
@@ -222,6 +224,7 @@ static int adjust_local_phandle_references(struct 
device_node *local_fixups,
 
phandle = be32_to_cpu(*(__be32 *)(prop->value + off));
phandle += phandle_delta;
+   /* This is bad because we cast away const */
*(__be32 *)(prop->value + off) = cpu_to_be32(phandle);
}
}
diff --git a/include/linux/of.h b/include/linux/of.h
index f22d4a83ca07..b0253886ef46 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -35,7 +35,7 @@ typedef u32 ihandle;
 struct property {
char  

[RFC/PATCH] of: Mark property::value as const

[Stephen Boyd]

The 'blob' we pass into populate_properties() is marked as const,
but we cast that const away when we assign the result of
fdt_getprop_by_offset() to pp->value. Let's mark value as const
instead, so that code can't mistakenly write to the value of the
property that we've so far advertised as const.

Unfortunately, this exposes a problem with the fdt resolver code,
where we overwrite the value member of properties of phandles to
update them with their final value. Add a comment for now to
indicate where we're potentially writing over const data.

You can see the problem here by loading an overlay dtb into
the kernel via the request firmware helper method (not direct
loading) and then passing that tree to the resolver on an arm64
device. In this case, the firmware data is vmapped with KERNEL_PAGE_RO
and the code crashes when attempting to write to the blob to update
the phandle properties.

Signed-off-by: Stephen Boyd 
---

I was thinking perhaps it would work to store another __be32 variant
of the phandle in each device node, but then we still have a problem
with properties that have phandles inside them at some offset that we
need to update. I guess the only real solution is to deep copy the
property in that case and then save around some info to free the
duplicated property later on?

 drivers/of/base.c |  2 +-
 drivers/of/fdt.c  | 12 ++--
 drivers/of/resolver.c |  3 +++
 include/linux/of.h|  2 +-
 4 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index fb6bb855714e..8e5f29b814c9 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1156,7 +1156,7 @@ EXPORT_SYMBOL_GPL(of_property_count_elems_of_size);
  * property data is too small or too large.
  *
  */
-static void *of_find_property_value_of_size(const struct device_node *np,
+static const void *of_find_property_value_of_size(const struct device_node *np,
const char *propname, u32 min, u32 max, size_t *len)
 {
struct property *prop = of_find_property(np, propname, NULL);
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index c9b5cac03b36..0635ef5dabf3 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -222,7 +222,7 @@ static void populate_properties(const void *blob,
 
pp->name   = (char *)pname;
pp->length = sz;
-   pp->value  = (__be32 *)val;
+   pp->value  = val;
*pprev = pp;
pprev  = >next;
}
@@ -232,6 +232,7 @@ static void populate_properties(const void *blob,
 */
if (!has_name) {
const char *p = nodename, *ps = p, *pa = NULL;
+   char *b;
int len;
 
while (*p) {
@@ -250,13 +251,12 @@ static void populate_properties(const void *blob,
if (!dryrun) {
pp->name   = "name";
pp->length = len;
-   pp->value  = pp + 1;
+   pp->value  = b = (char *)(pp + 1);
*pprev = pp;
pprev  = >next;
-   memcpy(pp->value, ps, len - 1);
-   ((char *)pp->value)[len - 1] = 0;
-   pr_debug("fixed up name for %s -> %s\n",
-nodename, (char *)pp->value);
+   memcpy(b, ps, len - 1);
+   b[len - 1] = 0;
+   pr_debug("fixed up name for %s -> %s\n", nodename, b);
}
}
 
diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 8bf12e904fd2..6d88f8100318 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -93,6 +93,7 @@ static void adjust_overlay_phandles(struct device_node 
*overlay,
if (phandle == OF_PHANDLE_ILLEGAL)
continue;
 
+   /* This is bad because we cast away const */
*(uint32_t *)prop->value = cpu_to_be32(overlay->phandle);
}
 
@@ -154,6 +155,7 @@ static int update_usages_of_a_phandle_reference(struct 
device_node *overlay,
goto err_fail;
}
 
+   /* This is bad because we cast away const */
*(__be32 *)(prop->value + offset) = cpu_to_be32(phandle);
}
 
@@ -222,6 +224,7 @@ static int adjust_local_phandle_references(struct 
device_node *local_fixups,
 
phandle = be32_to_cpu(*(__be32 *)(prop->value + off));
phandle += phandle_delta;
+   /* This is bad because we cast away const */
*(__be32 *)(prop->value + off) = cpu_to_be32(phandle);
}
}
diff --git a/include/linux/of.h b/include/linux/of.h
index f22d4a83ca07..b0253886ef46 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -35,7 +35,7 @@ typedef u32 ihandle;
 struct property {
char*name;
int