[U-Boot] cmd/fdt: support single value replacement within an array

[U-Boot] cmd/fdt: support single value replacement within an array

[Laurent Pinchart]

Hi Hannes,

(I'm not subscribed to the list, please keep me CC'ed on replies)

On Tuesday 30 May 2017 13:05:00 Hannes Schmelzer wrote:
> With this commit we can modify single values within an array of a dts
> property.

But with this commit U-Boot crashes if you try to create a new property with 
the fdt set command :-/

I've tested v2017.07 with the commit reverted, and fdt set works again for me. 
The issue is that your fdt_getprop() call fails and return NULL with len set 
to -1. You can easily imagine what the memcpy() following it will do.

> This is useful if we have for example a pwm-backlight where we want to
> modifiy the pwm frequency per u-boot script.
> 
> The pwm is described in dts like this:
> 
> backlight {
> 	pwms = <0x0000002b 0x00000000 0x004c4b40>;
> };
> 
> For changing the frequency, here the 3rd parameter, we simply type:
> 
> fdt set /backlight pwms <? ? 0x1E8480>;
> 
> For doing all this we:
> - backup the property content into our 'SCRATCHPAD'
> - only modify the array-cell if the new content doesn't start with '?'
> 
> Signed-off-by: Hannes Schmelzer <hannes.schmelzer@br-automation.com>
> 
> ---
> 
>  cmd/fdt.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/cmd/fdt.c b/cmd/fdt.c
> index a21415d..e55102a 100644
> --- a/cmd/fdt.c
> +++ b/cmd/fdt.c
> @@ -257,6 +257,7 @@  static int do_fdt(cmd_tbl_t *cmdtp, int flag, int argc,
> char * const argv[])> 
>  		char *prop;		/* property */
>  		int  nodeoffset;	/* node offset from libfdt */
>  		static char data[SCRATCHPAD];	/* storage for the property */
> +		const void *ptmp;
>  		int  len;		/* new length of the property */
>  		int  ret;		/* return value */
> 
> @@ -268,13 +269,6 @@  static int do_fdt(cmd_tbl_t *cmdtp, int flag, int
> argc, char * const argv[])> 
>  		pathp  = argv[2];
>  		prop   = argv[3];
> 
> -		if (argc == 4) {
> -			len = 0;
> -		} else {
> -			ret = fdt_parse_prop(&argv[4], argc - 4, data, &len);
> -			if (ret != 0)
> -				return ret;
> -		}
> 
>  		nodeoffset = fdt_path_offset (working_fdt, pathp);
>  		if (nodeoffset < 0) {
> @@ -286,6 +280,21 @@  static int do_fdt(cmd_tbl_t *cmdtp, int flag, int
> argc, char * const argv[])
>  			return 1;
>  		}
> 
> +		if (argc == 4) {
> +			len = 0;
> +		} else {
> +			ptmp = fdt_getprop(working_fdt, nodeoffset, prop,
> &len);
> +			if (len > SCRATCHPAD) {
> +				printf("prop (%d) doesn't fit in scratchpad!
> \n",
> +				       len);
> +				return 1;
> +			}
> +			memcpy(data, ptmp, len);
> +			ret = fdt_parse_prop(&argv[4], argc - 4, data, &len);
> +			if (ret != 0)
> +				return ret;
> +		}
> +
>  		ret = fdt_setprop(working_fdt, nodeoffset, prop, data, len);
>  		if (ret < 0) {
>  			printf ("libfdt fdt_setprop(): %s\n",
> fdt_strerror(ret));
> @@ -766,7 +775,11 @@  static int fdt_parse_prop(char * const *newval, int
> count, char *data, int *len)
>  			cp = newp;
>  			tmp = simple_strtoul(cp, &newp, 0);
> 
> -			*(fdt32_t *)data = cpu_to_fdt32(tmp);
> +			if (*cp != '?')
> +				*(fdt32_t *)data = cpu_to_fdt32(tmp);
> +			else
> +				newp++;
> +
> 
>  			data  += 4;
>  			*len += 4;
-- 
Regards,

Laurent Pinchart

[U-Boot] cmd/fdt: support single value replacement within an array

[Hannes Schmelzer]

Hi Laurent,

thanks for reporting this.
I just came back from holiday. I will check / fix this as soon as 
possible.

cheers,
Hannes


Laurent Pinchart <laurent.pinchart@ideasonboard.com> schrieb am 04.08.2017 
23:23:19:

> Von: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> An: u-boot at lists.denx.de
> Kopie: Hannes Schmelzer <hannes.schmelzer@br-automation.com>, Simon 
Glass 
> <sjg@chromium.org>
> Datum: 04.08.2017 23:23
> Betreff: Re: cmd/fdt: support single value replacement within an array
> 
> Hi Hannes,
> 
> (I'm not subscribed to the list, please keep me CC'ed on replies)
> 
> On Tuesday 30 May 2017 13:05:00 Hannes Schmelzer wrote:
> > With this commit we can modify single values within an array of a dts
> > property.
> 
> But with this commit U-Boot crashes if you try to create a new property 
with 
> the fdt set command :-/
> 
> I've tested v2017.07 with the commit reverted, and fdt set works again 
for me. 
> The issue is that your fdt_getprop() call fails and return NULL with len 
set 
> to -1. You can easily imagine what the memcpy() following it will do.
> 
> > This is useful if we have for example a pwm-backlight where we want to
> > modifiy the pwm frequency per u-boot script.
> > 
> > The pwm is described in dts like this:
> > 
> > backlight {
> >    pwms = <0x0000002b 0x00000000 0x004c4b40>;
> > };
> > 
> > For changing the frequency, here the 3rd parameter, we simply type:
> > 
> > fdt set /backlight pwms <? ? 0x1E8480>;
> > 
> > For doing all this we:
> > - backup the property content into our 'SCRATCHPAD'
> > - only modify the array-cell if the new content doesn't start with '?'
> > 
> > Signed-off-by: Hannes Schmelzer <hannes.schmelzer@br-automation.com>
> > 
> > ---
> > 
> >  cmd/fdt.c | 29 +++++++++++++++++++++--------
> >  1 file changed, 21 insertions(+), 8 deletions(-)
> > 
> > diff --git a/cmd/fdt.c b/cmd/fdt.c
> > index a21415d..e55102a 100644
> > --- a/cmd/fdt.c
> > +++ b/cmd/fdt.c
> > @@ -257,6 +257,7 @@  static int do_fdt(cmd_tbl_t *cmdtp, int flag, int 
argc,
> > char * const argv[])> 
> >        char *prop;      /* property */
> >        int  nodeoffset;   /* node offset from libfdt */
> >        static char data[SCRATCHPAD];   /* storage for the property */
> > +      const void *ptmp;
> >        int  len;      /* new length of the property */
> >        int  ret;      /* return value */
> > 
> > @@ -268,13 +269,6 @@  static int do_fdt(cmd_tbl_t *cmdtp, int flag, 
int
> > argc, char * const argv[])> 
> >        pathp  = argv[2];
> >        prop   = argv[3];
> > 
> > -      if (argc == 4) {
> > -         len = 0;
> > -      } else {
> > -         ret = fdt_parse_prop(&argv[4], argc - 4, data, &len);
> > -         if (ret != 0)
> > -            return ret;
> > -      }
> > 
> >        nodeoffset = fdt_path_offset (working_fdt, pathp);
> >        if (nodeoffset < 0) {
> > @@ -286,6 +280,21 @@  static int do_fdt(cmd_tbl_t *cmdtp, int flag, 
int
> > argc, char * const argv[])
> >           return 1;
> >        }
> > 
> > +      if (argc == 4) {
> > +         len = 0;
> > +      } else {
> > +         ptmp = fdt_getprop(working_fdt, nodeoffset, prop,
> > &len);
> > +         if (len > SCRATCHPAD) {
> > +            printf("prop (%d) doesn't fit in scratchpad!
> > \n",
> > +                   len);
> > +            return 1;
> > +         }
> > +         memcpy(data, ptmp, len);
> > +         ret = fdt_parse_prop(&argv[4], argc - 4, data, &len);
> > +         if (ret != 0)
> > +            return ret;
> > +      }
> > +
> >        ret = fdt_setprop(working_fdt, nodeoffset, prop, data, len);
> >        if (ret < 0) {
> >           printf ("libfdt fdt_setprop(): %s\n",
> > fdt_strerror(ret));
> > @@ -766,7 +775,11 @@  static int fdt_parse_prop(char * const *newval, 
int
> > count, char *data, int *len)
> >           cp = newp;
> >           tmp = simple_strtoul(cp, &newp, 0);
> > 
> > -         *(fdt32_t *)data = cpu_to_fdt32(tmp);
> > +         if (*cp != '?')
> > +            *(fdt32_t *)data = cpu_to_fdt32(tmp);
> > +         else
> > +            newp++;
> > +
> > 
> >           data  += 4;
> >           *len += 4;
> -- 
> Regards,
> 
> Laurent Pinchart
> 

[U-Boot] cmd/fdt: support single value replacement within an array

[Hannes Schmelzer]

Laurent Pinchart <laurent.pinchart@ideasonboard.com> schrieb am 04.08.2017 
23:23:19:

> Hi Hannes,
Hi Laurent,

as told a few days ago i'm now coming back to this issue.

> 
> (I'm not subscribed to the list, please keep me CC'ed on replies)
subscribing to the list would be a great idea, so you're always up to date 
whats going on ;-)

> 
> On Tuesday 30 May 2017 13:05:00 Hannes Schmelzer wrote:
> > With this commit we can modify single values within an array of a dts
> > property.
> 
> But with this commit U-Boot crashes if you try to create a new property 
with 
> the fdt set command :-/
> 
> I've tested v2017.07 with the commit reverted, and fdt set works again 
for me. 
> The issue is that your fdt_getprop() call fails and return NULL with len 
set 
> to -1. You can easily imagine what the memcpy() following it will do.

Yes. Your'e right with that.

I just checked most current source (...), there were changes regarding 
this issue.
Tom introduced a check against the fail of fdt_getprop(...) call.

http://git.denx.de/?p=u-boot.git;a=commitdiff;h=99bb38e2cce9d99238458e0f6d1880c6d2e80a4d

Can you please try with your testcase with most current source again?

please let us know if the problem is fixed with this.

cheers,
Hannes

(....)

> > ---
> > 
> >  cmd/fdt.c | 29 +++++++++++++++++++++--------
> >  1 file changed, 21 insertions(+), 8 deletions(-)
> > 
> > diff --git a/cmd/fdt.c b/cmd/fdt.c
> > index a21415d..e55102a 100644
> > --- a/cmd/fdt.c
> > +++ b/cmd/fdt.c
> > @@ -257,6 +257,7 @@  static int do_fdt(cmd_tbl_t *cmdtp, int flag, int 
argc,
> > char * const argv[])> 
> >        char *prop;      /* property */
> >        int  nodeoffset;   /* node offset from libfdt */
> >        static char data[SCRATCHPAD];   /* storage for the property */
> > +      const void *ptmp;
> >        int  len;      /* new length of the property */
> >        int  ret;      /* return value */
> > 
> > @@ -268,13 +269,6 @@  static int do_fdt(cmd_tbl_t *cmdtp, int flag, 
int
> > argc, char * const argv[])> 
> >        pathp  = argv[2];
> >        prop   = argv[3];
> > 
> > -      if (argc == 4) {
> > -         len = 0;
> > -      } else {
> > -         ret = fdt_parse_prop(&argv[4], argc - 4, data, &len);
> > -         if (ret != 0)
> > -            return ret;
> > -      }
> > 
> >        nodeoffset = fdt_path_offset (working_fdt, pathp);
> >        if (nodeoffset < 0) {
> > @@ -286,6 +280,21 @@  static int do_fdt(cmd_tbl_t *cmdtp, int flag, 
int
> > argc, char * const argv[])
> >           return 1;
> >        }
> > 
> > +      if (argc == 4) {
> > +         len = 0;
> > +      } else {
> > +         ptmp = fdt_getprop(working_fdt, nodeoffset, prop,
> > &len);
> > +         if (len > SCRATCHPAD) {
> > +            printf("prop (%d) doesn't fit in scratchpad!
> > \n",
> > +                   len);
> > +            return 1;
> > +         }
> > +         memcpy(data, ptmp, len);
> > +         ret = fdt_parse_prop(&argv[4], argc - 4, data, &len);
> > +         if (ret != 0)
> > +            return ret;
> > +      }
> > +
> >        ret = fdt_setprop(working_fdt, nodeoffset, prop, data, len);
> >        if (ret < 0) {
> >           printf ("libfdt fdt_setprop(): %s\n",
> > fdt_strerror(ret));
> > @@ -766,7 +775,11 @@  static int fdt_parse_prop(char * const *newval, 
int
> > count, char *data, int *len)
> >           cp = newp;
> >           tmp = simple_strtoul(cp, &newp, 0);
> > 
> > -         *(fdt32_t *)data = cpu_to_fdt32(tmp);
> > +         if (*cp != '?')
> > +            *(fdt32_t *)data = cpu_to_fdt32(tmp);
> > +         else
> > +            newp++;
> > +
> > 
> >           data  += 4;
> >           *len += 4;

[U-Boot] cmd/fdt: support single value replacement within an array

[Laurent Pinchart]

Hi Hannes,

On Friday 18 Aug 2017 10:07:19 Hannes Schmelzer wrote:
> Laurent Pinchart schrieb am 04.08.2017 23:23:19:
> 
> Hi Laurent,
> 
> as told a few days ago i'm now coming back to this issue.

Thank you.

> > (I'm not subscribed to the list, please keep me CC'ed on replies)
> 
> subscribing to the list would be a great idea, so you're always up to date
> whats going on ;-)

I know, but I've already stopped reading mailing lists I'm subscribed to due 
to the amount of traffic, so I figured out that subscribing to new ones 
wouldn't be a good idea :-)

> > On Tuesday 30 May 2017 13:05:00 Hannes Schmelzer wrote:
> >> With this commit we can modify single values within an array of a dts
> >> property.
> > 
> > But with this commit U-Boot crashes if you try to create a new property
> > with the fdt set command :-/
> > 
> > I've tested v2017.07 with the commit reverted, and fdt set works again
> > for me. The issue is that your fdt_getprop() call fails and return NULL
> > with len set to -1. You can easily imagine what the memcpy() following it
> > will do.
> 
> Yes. Your'e right with that.
> 
> I just checked most current source (...), there were changes regarding
> this issue.
> Tom introduced a check against the fail of fdt_getprop(...) call.
> 
> http://git.denx.de/?p=u-boot.git;a=commitdiff;h=99bb38e2cce9d99238458e0f6d18
> 80c6d2e80a4d
> 
> Can you please try with your testcase with most current source again?
> 
> please let us know if the problem is fixed with this.

I can't test that right now as I don't have access to my hardware at the 
moment, but I doubt it will work.

The code now reads as

	ptmp = fdt_getprop(working_fdt, nodeoffset, prop, &len);
	if (!ptmp) {
		printf("prop (%s) not found!\n", prop);
		return 1;
	}

The new !ptmp check should prevent the crash, but it will also prevent the fdt 
set command from operating correctly, as it will return an error if the 
property isn't found.

-- 
Regards,

Laurent Pinchart

[U-Boot] Antwort: Re: cmd/fdt: support single value replacement within an array

[Hannes Schmelzer]

Laurent Pinchart <laurent.pinchart@ideasonboard.com> schrieb am 18.08.2017 
10:14:39:

> Von: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> 
> Hi Hannes,
Hi Laurent,

i will provide some patch for fixing this issue.

cheers,
Hannes

> 
> On Friday 18 Aug 2017 10:07:19 Hannes Schmelzer wrote:
> > Laurent Pinchart schrieb am 04.08.2017 23:23:19:
> > 
> > Hi Laurent,
> > 
> > as told a few days ago i'm now coming back to this issue.
> 
> Thank you.
> 
> > > (I'm not subscribed to the list, please keep me CC'ed on replies)
> > 
> > subscribing to the list would be a great idea, so you're always up to 
date
> > whats going on ;-)
> 
> I know, but I've already stopped reading mailing lists I'm subscribed to 
due 
> to the amount of traffic, so I figured out that subscribing to new ones 
> wouldn't be a good idea :-)
> 
> > > On Tuesday 30 May 2017 13:05:00 Hannes Schmelzer wrote:
> > >> With this commit we can modify single values within an array of a 
dts
> > >> property.
> > > 
> > > But with this commit U-Boot crashes if you try to create a new 
property
> > > with the fdt set command :-/
> > > 
> > > I've tested v2017.07 with the commit reverted, and fdt set works 
again
> > > for me. The issue is that your fdt_getprop() call fails and return 
NULL
> > > with len set to -1. You can easily imagine what the memcpy() 
following it
> > > will do.
> > 
> > Yes. Your'e right with that.
> > 
> > I just checked most current source (...), there were changes regarding
> > this issue.
> > Tom introduced a check against the fail of fdt_getprop(...) call.
> > 
> > 
http://git.denx.de/?p=u-boot.git;a=commitdiff;h=99bb38e2cce9d99238458e0f6d18

> > 80c6d2e80a4d
> > 
> > Can you please try with your testcase with most current source again?
> > 
> > please let us know if the problem is fixed with this.
> 
> I can't test that right now as I don't have access to my hardware at the 

> moment, but I doubt it will work.
> 
> The code now reads as
> 
>    ptmp = fdt_getprop(working_fdt, nodeoffset, prop, &len);
>    if (!ptmp) {
>       printf("prop (%s) not found!\n", prop);
>       return 1;
>    }
> 
> The new !ptmp check should prevent the crash, but it will also prevent 
the fdt 
> set command from operating correctly, as it will return an error if the 
> property isn't found.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
>