| Message ID | 20230227195357.98723-8-marek.vasut+renesas@mailbox.org |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [01/32] cmd: fdt: Import is_printable_string() from DTC to fix u32 misprint | expand |
On Mon, 27 Feb 2023 at 12:55, Marek Vasut <marek.vasut+renesas@mailbox.org> wrote: > > The command assumed 32bit pointers so far, with 64bit pointer the > command would overwrite a piece of stack. Fix it by extending the > array size to cater for 64bit pointer, and use snprintf() to avoid > writing past the end of the array ever again. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org> > --- > Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Tom Rini <trini@konsulko.com> > --- > cmd/fdt.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Reviewed-by: Simon Glass <sjg@chromium.org> > diff --git a/cmd/fdt.c b/cmd/fdt.c > index 279dad9fe11..bc19303159d 100644 > --- a/cmd/fdt.c > +++ b/cmd/fdt.c > @@ -466,9 +466,9 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) > return ret; > } else if (subcmd[0] == 'a') { > /* Get address */ > - char buf[11]; > + char buf[19]; > > - sprintf(buf, "0x%p", nodep); > + snprintf(buf, sizeof(buf), "0x%p", nodep); Do we need the 0x? I believe that is always the base. > env_set(var, buf); > } else if (subcmd[0] == 's') { > /* Get size */ > -- > 2.39.2 > Regards, Simon
On 3/1/23 16:02, Simon Glass wrote: > On Mon, 27 Feb 2023 at 12:55, Marek Vasut > <marek.vasut+renesas@mailbox.org> wrote: >> >> The command assumed 32bit pointers so far, with 64bit pointer the >> command would overwrite a piece of stack. Fix it by extending the >> array size to cater for 64bit pointer, and use snprintf() to avoid >> writing past the end of the array ever again. >> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org> >> --- >> Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >> Cc: Simon Glass <sjg@chromium.org> >> Cc: Tom Rini <trini@konsulko.com> >> --- >> cmd/fdt.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> > > Reviewed-by: Simon Glass <sjg@chromium.org> > > >> diff --git a/cmd/fdt.c b/cmd/fdt.c >> index 279dad9fe11..bc19303159d 100644 >> --- a/cmd/fdt.c >> +++ b/cmd/fdt.c >> @@ -466,9 +466,9 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) >> return ret; >> } else if (subcmd[0] == 'a') { >> /* Get address */ >> - char buf[11]; >> + char buf[19]; >> >> - sprintf(buf, "0x%p", nodep); >> + snprintf(buf, sizeof(buf), "0x%p", nodep); > > Do we need the 0x? I believe that is always the base. I would argue this behavior is an ABI by now. I did send a separate patch on top, which can then be reverted if that would break anything: https://patchwork.ozlabs.org/project/uboot/patch/20230302030440.322361-1-marek.vasut+renesas@mailbox.org/
diff --git a/cmd/fdt.c b/cmd/fdt.c index 279dad9fe11..bc19303159d 100644 --- a/cmd/fdt.c +++ b/cmd/fdt.c @@ -466,9 +466,9 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) return ret; } else if (subcmd[0] == 'a') { /* Get address */ - char buf[11]; + char buf[19]; - sprintf(buf, "0x%p", nodep); + snprintf(buf, sizeof(buf), "0x%p", nodep); env_set(var, buf); } else if (subcmd[0] == 's') { /* Get size */
The command assumed 32bit pointers so far, with 64bit pointer the command would overwrite a piece of stack. Fix it by extending the array size to cater for 64bit pointer, and use snprintf() to avoid writing past the end of the array ever again. Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org> --- Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> Cc: Simon Glass <sjg@chromium.org> Cc: Tom Rini <trini@konsulko.com> --- cmd/fdt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)