Patchwork [U-Boot,3/4] Make printf and vprintf safe from buffer overruns

login
register
mail settings
Submitter Simon Glass
Date Sept. 23, 2011, 5:38 p.m.
Message ID <1316799532-20761-4-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/116159/
State New, archived
Headers show

Comments

Simon Glass - Sept. 23, 2011, 5:38 p.m.
From: Sonny Rao <sonnyrao@chromium.org>

From: Sonny Rao <sonnyrao@chromium.org>

utilize the added vscnprintf functions to avoid buffer overruns
The implementation is fairly dumb in that it doesn't detect
that the buffer is too small, but at least will not cause crashes.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 common/console.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)
Kumar Gala - Sept. 23, 2011, 6:36 p.m.
On Sep 23, 2011, at 12:38 PM, Simon Glass wrote:

> From: Sonny Rao <sonnyrao@chromium.org>
> 
> From: Sonny Rao <sonnyrao@chromium.org>
> 
> utilize the added vscnprintf functions to avoid buffer overruns
> The implementation is fairly dumb in that it doesn't detect
> that the buffer is too small, but at least will not cause crashes.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> common/console.c |   10 +++++-----
> 1 files changed, 5 insertions(+), 5 deletions(-)

If these are from Sonny, they really should have a Signed-off-by from him.

- k
Simon Glass - Sept. 23, 2011, 6:48 p.m.
Hi Kumar,

On Fri, Sep 23, 2011 at 11:36 AM, Kumar Gala <galak@kernel.crashing.org> wrote:
>
> On Sep 23, 2011, at 12:38 PM, Simon Glass wrote:
>
>> From: Sonny Rao <sonnyrao@chromium.org>
>>
>> From: Sonny Rao <sonnyrao@chromium.org>
>>
>> utilize the added vscnprintf functions to avoid buffer overruns
>> The implementation is fairly dumb in that it doesn't detect
>> that the buffer is too small, but at least will not cause crashes.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>> common/console.c |   10 +++++-----
>> 1 files changed, 5 insertions(+), 5 deletions(-)
>
> If these are from Sonny, they really should have a Signed-off-by from him.
>
OK will fix that with next version thanks. The commit in my tree says
Sonny but perhaps git format-patch --signoff is not doing what I
expect.

Regards,
Simon

> - k
>
Mike Frysinger - Sept. 23, 2011, 8:31 p.m.
On Friday, September 23, 2011 13:38:51 Simon Glass wrote:
> --- a/common/console.c
> +++ b/common/console.c
> @@ -212,7 +212,7 @@ int serial_printf(const char *fmt, ...)
>  	/* For this to work, printbuffer must be larger than
>  	 * anything we ever want to print.
>  	 */
> -	i = vsprintf(printbuffer, fmt, args);
> +	i = vscnprintf(printbuffer, CONFIG_SYS_PBSIZE, fmt, args);

i think sizeof(printbuffer) would be better.  same goes for all the other 
changes here.
-mike
Simon Glass - Sept. 23, 2011, 8:41 p.m.
Hi Mike,

On Fri, Sep 23, 2011 at 1:31 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Friday, September 23, 2011 13:38:51 Simon Glass wrote:
>> --- a/common/console.c
>> +++ b/common/console.c
>> @@ -212,7 +212,7 @@ int serial_printf(const char *fmt, ...)
>>       /* For this to work, printbuffer must be larger than
>>        * anything we ever want to print.
>>        */
>> -     i = vsprintf(printbuffer, fmt, args);
>> +     i = vscnprintf(printbuffer, CONFIG_SYS_PBSIZE, fmt, args);
>
> i think sizeof(printbuffer) would be better.  same goes for all the other
> changes here.
> -mike
>

Yes, indeed. Could we go as far as removing CONFIG_SYS_PBSIZE, and
just use a standard value?

Regards,
Simon
Mike Frysinger - Sept. 23, 2011, 10:36 p.m.
On Friday, September 23, 2011 16:41:50 Simon Glass wrote:
> On Fri, Sep 23, 2011 at 1:31 PM, Mike Frysinger wrote:
> > On Friday, September 23, 2011 13:38:51 Simon Glass wrote:
> >> --- a/common/console.c
> >> +++ b/common/console.c
> >> @@ -212,7 +212,7 @@ int serial_printf(const char *fmt, ...)
> >>       /* For this to work, printbuffer must be larger than
> >>        * anything we ever want to print.
> >>        */
> >> -     i = vsprintf(printbuffer, fmt, args);
> >> +     i = vscnprintf(printbuffer, CONFIG_SYS_PBSIZE, fmt, args);
> > 
> > i think sizeof(printbuffer) would be better.  same goes for all the other
> > changes here.
> > -mike
> 
> Yes, indeed. Could we go as far as removing CONFIG_SYS_PBSIZE, and
> just use a standard value?

in the context of I/O funcs, CONFIG_SYS_PBSIZE is the current standard
-mike
Simon Glass - Sept. 23, 2011, 11:06 p.m.
Hi Mike,

On Fri, Sep 23, 2011 at 3:36 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Friday, September 23, 2011 16:41:50 Simon Glass wrote:
>> On Fri, Sep 23, 2011 at 1:31 PM, Mike Frysinger wrote:
>> > On Friday, September 23, 2011 13:38:51 Simon Glass wrote:
>> >> --- a/common/console.c
>> >> +++ b/common/console.c
>> >> @@ -212,7 +212,7 @@ int serial_printf(const char *fmt, ...)
>> >>       /* For this to work, printbuffer must be larger than
>> >>        * anything we ever want to print.
>> >>        */
>> >> -     i = vsprintf(printbuffer, fmt, args);
>> >> +     i = vscnprintf(printbuffer, CONFIG_SYS_PBSIZE, fmt, args);
>> >
>> > i think sizeof(printbuffer) would be better.  same goes for all the other
>> > changes here.
>> > -mike
>>
>> Yes, indeed. Could we go as far as removing CONFIG_SYS_PBSIZE, and
>> just use a standard value?
>
> in the context of I/O funcs, CONFIG_SYS_PBSIZE is the current standard
> -mike
>

OK - is there a reason why boards can redefine this? Many many boards
do. It seems like something that should just be given a sensible
value.

Regards,
Simon
Wolfgang Denk - Sept. 25, 2011, 8:14 p.m.
Dear Simon Glass,

In message <CAPnjgZ0-8wU4Hj3pdmfNMWnj4EPUmQ69U_UARaRt5CbqQv0Ofg@mail.gmail.com> you wrote:
> 
> Yes, indeed. Could we go as far as removing CONFIG_SYS_PBSIZE, and
> just use a standard value?

If you can find one that fits for all boards?  Keep in mind that
printf() gets used before relocation, when available stack space may
be _very_ limited.

Best regards,

Wolfgang Denk
Wolfgang Denk - Sept. 25, 2011, 8:16 p.m.
Dear Simon Glass,

In message <CAPnjgZ2D8MYS7wHMFUnzVhXK_tcmv7GayJyEormkPDNA78p7JQ@mail.gmail.com> you wrote:
> 
> > in the context of I/O funcs, CONFIG_SYS_PBSIZE is the current standard
> 
> OK - is there a reason why boards can redefine this? Many many boards
> do. It seems like something that should just be given a sensible
> value.

Indeed - that's exactly what the board specific definitions are
supposed to do.

Best regards,

Wolfgang Denk
Simon Glass - Sept. 26, 2011, 6:25 p.m.
HI Wolfgang,

On Sun, Sep 25, 2011 at 1:14 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ0-8wU4Hj3pdmfNMWnj4EPUmQ69U_UARaRt5CbqQv0Ofg@mail.gmail.com> you wrote:
>>
>> Yes, indeed. Could we go as far as removing CONFIG_SYS_PBSIZE, and
>> just use a standard value?
>
> If you can find one that fits for all boards?  Keep in mind that
> printf() gets used before relocation, when available stack space may
> be _very_ limited.

Yes that is a problem. Perhaps we could changes things so that this
CONFIG really only matters prior to relocation (see other thread with
Albert) and we could just choose a suitable small value like 256,
which seems to be the minimum for most boards.

Regards,
Simon

>
> 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@denx.de
> "And it should be the law: If you use  the  word  `paradigm'  without
> knowing  what  the  dictionary  says  it  means,  you  go to jail. No
> exceptions."                     - David Jones @ Megatest Corporation
>
Wolfgang Denk - Sept. 26, 2011, 6:47 p.m.
Dear Simon Glass,

In message <CAPnjgZ1Q89Ew2Be2QcjoWF3Nyb+sJ-SxfFunZturMwdLxBc_Wg@mail.gmail.com> you wrote:
> 
> > If you can find one that fits for all boards? =A0Keep in mind that
> > printf() gets used before relocation, when available stack space may
> > be _very_ limited.
> 
> Yes that is a problem. Perhaps we could changes things so that this
> CONFIG really only matters prior to relocation (see other thread with
> Albert) and we could just choose a suitable small value like 256,
> which seems to be the minimum for most boards.

You are probably addressing a non-issue.  How many related bug reports
can you find in the last decade or so?

Best regards,

Wolfgang Denk
Simon Glass - Sept. 26, 2011, 7:02 p.m.
Hi Wolfgang,

On Mon, Sep 26, 2011 at 11:47 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ1Q89Ew2Be2QcjoWF3Nyb+sJ-SxfFunZturMwdLxBc_Wg@mail.gmail.com> you wrote:
>>
>> > If you can find one that fits for all boards? =A0Keep in mind that
>> > printf() gets used before relocation, when available stack space may
>> > be _very_ limited.
>>
>> Yes that is a problem. Perhaps we could changes things so that this
>> CONFIG really only matters prior to relocation (see other thread with
>> Albert) and we could just choose a suitable small value like 256,
>> which seems to be the minimum for most boards.
>
> You are probably addressing a non-issue.  How many related bug reports
> can you find in the last decade or so?
>

Yes probably, it isn't important and is off-topic from my patch anyway.

Regards,
Simon

> 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@denx.de
> All your people must learn before you can reach for the stars.
>        -- Kirk, "The Gamesters of Triskelion", stardate 3259.2
>

Patch

diff --git a/common/console.c b/common/console.c
index 8c650e0..6057e9a 100644
--- a/common/console.c
+++ b/common/console.c
@@ -212,7 +212,7 @@  int serial_printf(const char *fmt, ...)
 	/* For this to work, printbuffer must be larger than
 	 * anything we ever want to print.
 	 */
-	i = vsprintf(printbuffer, fmt, args);
+	i = vscnprintf(printbuffer, CONFIG_SYS_PBSIZE, fmt, args);
 	va_end(args);
 
 	serial_puts(printbuffer);
@@ -281,7 +281,7 @@  int fprintf(int file, const char *fmt, ...)
 	/* For this to work, printbuffer must be larger than
 	 * anything we ever want to print.
 	 */
-	i = vsprintf(printbuffer, fmt, args);
+	i = vscnprintf(printbuffer, CONFIG_SYS_PBSIZE, fmt, args);
 	va_end(args);
 
 	/* Send to desired file */
@@ -376,7 +376,7 @@  int printf(const char *fmt, ...)
 	/* For this to work, printbuffer must be larger than
 	 * anything we ever want to print.
 	 */
-	i = vsprintf(printbuffer, fmt, args);
+	i = vscnprintf(printbuffer, CONFIG_SYS_PBSIZE, fmt, args);
 	va_end(args);
 
 	/* Print the string */
@@ -392,7 +392,7 @@  int vprintf(const char *fmt, va_list args)
 	/* For this to work, printbuffer must be larger than
 	 * anything we ever want to print.
 	 */
-	i = vsprintf(printbuffer, fmt, args);
+	i = vscnprintf(printbuffer, CONFIG_SYS_PBSIZE, fmt, args);
 
 	/* Print the string */
 	puts(printbuffer);
@@ -459,7 +459,7 @@  inline void dbg(const char *fmt, ...)
 	/* For this to work, printbuffer must be larger than
 	 * anything we ever want to print.
 	 */
-	i = vsprintf(printbuffer, fmt, args);
+	i = vsnprintf(printbuffer, CONFIG_SYS_PBSIZE, fmt, args);
 	va_end(args);
 
 	if ((screen + sizeof(screen) - 1 - cursor)