[U-Boot] [PATCH] cmd_mtdparts.c: prevent printbuffer overflows

[U-Boot] [PATCH] cmd_mtdparts.c: prevent printbuffer overflows

[Anatolij Gustschin]

The length of configured MTDPARTS_DEFAULT string
could be greather than console printbuffer size.
Check the lenght of the string before printing
to prevent U-Boot crashes.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
---
 common/cmd_mtdparts.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/common/cmd_mtdparts.c b/common/cmd_mtdparts.c
index 230e96e..d4cb194 100644
--- a/common/cmd_mtdparts.c
+++ b/common/cmd_mtdparts.c
@@ -1254,6 +1254,14 @@ static void list_partitions(void)
 	printf("\ndefaults:\n");
 	printf("mtdids  : %s\n",
 		mtdids_default ? mtdids_default : "none");
+
+	/* Check to prevent printbuffer overflows */
+	if (mtdparts_default && strlen(mtdparts_default) > CONFIG_SYS_PBSIZE) {
+		puts("Warning: mtdparts too long,"
+		     " please increase CONFIG_SYS_PBSIZE\n");
+		return;
+	}
+
 	printf("mtdparts: %s\n",
 		mtdparts_default ? mtdparts_default : "none");
 }
-- 
1.6.3.3

[U-Boot] [PATCH] cmd_mtdparts.c: prevent printbuffer overflows

[Detlev Zundel]

Hi Anatolij,

> The length of configured MTDPARTS_DEFAULT string
> could be greather than console printbuffer size.
> Check the lenght of the string before printing
> to prevent U-Boot crashes.
>
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> ---
>  common/cmd_mtdparts.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/common/cmd_mtdparts.c b/common/cmd_mtdparts.c
> index 230e96e..d4cb194 100644
> --- a/common/cmd_mtdparts.c
> +++ b/common/cmd_mtdparts.c
> @@ -1254,6 +1254,14 @@ static void list_partitions(void)
>  	printf("\ndefaults:\n");
>  	printf("mtdids  : %s\n",
>  		mtdids_default ? mtdids_default : "none");
> +
> +	/* Check to prevent printbuffer overflows */
> +	if (mtdparts_default && strlen(mtdparts_default) > CONFIG_SYS_PBSIZE) {
> +		puts("Warning: mtdparts too long,"
> +		     " please increase CONFIG_SYS_PBSIZE\n");
> +		return;
> +	}
> +
>  	printf("mtdparts: %s\n",
>  		mtdparts_default ? mtdparts_default : "none");
>  }

If I understand this correctly, then the real problem is the console
code crashing without a warning, correct?  If so, then please put such a
warning in the correct place instead of fixing the caller sites.

Thanks
  Detlev

-- 
Man sei weder unzufrieden mit sich selbst - denn das waere Kleinmut - noch
selbstzufrieden - denn das waere Dummheit.
                                    --- Baltasar Gracian
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH] cmd_mtdparts.c: prevent printbuffer overflows

[Wolfgang Denk]

Dear Detlev Zundel,

In message <m2ljekyyd5.fsf@ohwell.denx.de> you wrote:
> 
> >  	printf("mtdparts: %s\n",
> >  		mtdparts_default ? mtdparts_default : "none");
> >  }
> 
> If I understand this correctly, then the real problem is the console
> code crashing without a warning, correct?  If so, then please put such a

True.

> warning in the correct place instead of fixing the caller sites.

That's not exactly trivial, if you have a look at the code in
"common/console.c" - the printf code has no way to know the size of
the provided buffer.

OTOH it might make sense to use the (small, preconfigured) buffer only
before relocation, and then, when devices become available, switch to
a reasonably sized malloc()ed buffer - say 4 kB?   But then - this
just shifts the limits...

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Do you know about being with somebody? Wanting to be? If  I  had  the
whole  universe,  I'd  give it to you, Janice. When I see you, I feel
like I'm hungry all over. Do you know how that feels?
	-- Charlie Evans, "Charlie X", stardate 1535.8

[U-Boot] [PATCH] cmd_mtdparts.c: prevent printbuffer overflows

[Wolfgang Denk]

Dear Anatolij Gustschin,

In message <1266924140-14457-1-git-send-email-agust@denx.de> you wrote:
> The length of configured MTDPARTS_DEFAULT string
> could be greather than console printbuffer size.
> Check the lenght of the string before printing
> to prevent U-Boot crashes.
> 
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> ---
>  common/cmd_mtdparts.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/common/cmd_mtdparts.c b/common/cmd_mtdparts.c
> index 230e96e..d4cb194 100644
> --- a/common/cmd_mtdparts.c
> +++ b/common/cmd_mtdparts.c
> @@ -1254,6 +1254,14 @@ static void list_partitions(void)
>  	printf("\ndefaults:\n");
>  	printf("mtdids  : %s\n",
>  		mtdids_default ? mtdids_default : "none");
> +
> +	/* Check to prevent printbuffer overflows */
> +	if (mtdparts_default && strlen(mtdparts_default) > CONFIG_SYS_PBSIZE) {
> +		puts("Warning: mtdparts too long,"
> +		     " please increase CONFIG_SYS_PBSIZE\n");
> +		return;
> +	}
> +

Instead of adding essentially dead code that does not really help the
end user, it would be better to avoid the potential problems. As log
as the console code has not been improved, it may make sense to avoid
printf() when you don't really need it.

I recommend to change this

>  	printf("mtdparts: %s\n",
>  		mtdparts_default ? mtdparts_default : "none");

into something like

	puts("mtdparts: ");
	puts(mtdparts_default ? mtdparts_default : "none");

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Unix is simple, but it takes a genius to understand the simplicity."
					             - Dennis Ritchie

[U-Boot] [PATCH] cmd_mtdparts.c: prevent printbuffer overflows

[Anatolij Gustschin]

Hello Wolfgang,

Wolfgang Denk <wd@denx.de> wrote:

> > +	/* Check to prevent printbuffer overflows */
> > +	if (mtdparts_default && strlen(mtdparts_default) > CONFIG_SYS_PBSIZE) {
> > +		puts("Warning: mtdparts too long,"
> > +		     " please increase CONFIG_SYS_PBSIZE\n");
> > +		return;
> > +	}
> > +
> 
> Instead of adding essentially dead code that does not really help the
> end user, it would be better to avoid the potential problems. As log
> as the console code has not been improved, it may make sense to avoid
> printf() when you don't really need it.

This is indeed much better, thanks!

> I recommend to change this
> 
> >  	printf("mtdparts: %s\n",
> >  		mtdparts_default ? mtdparts_default : "none");
> 
> into something like
> 
> 	puts("mtdparts: ");
> 	puts(mtdparts_default ? mtdparts_default : "none");

I'll fix it as suggested, thanks!

Best regards,
Anatolij
--
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de