[U-Boot] [PATCH v5 0/4] Buffer overruns in printf

[Simon Glass]

Hi,

On Tue, Oct 25, 2011 at 4:42 PM, Simon Glass <sjg at chromium.org> wrote:
> The printf family of functions in U-Boot cannot deal with a situation where
> the caller provides a buffer which turns out to be too small for the format
> string. This can result in buffer overflows, stack overflows and other bad
> behavior.
>
> This patch series tidies this up in the common vsprintf.c code.
>
> You can find a discussion of the Linux / U-Boot licensing issues here:
>
> http://patchwork.ozlabs.org/patch/116161/
>
> Code Size Impact
> ----------------
>
> (From Simon Glass <sjg at chromium.org>)
> With my ARMv7 compiler (gcc-4.4.3_cos_gg_53174) the code size increase is
> 312 bytes, about 10% increase to code size vsprintf.o.
>
> With the CONFIG_SYS_NO_VSNPRINT option defined, the code size impact
> is 4 bytes.
>
>
> Changes in v2:
> - Use sizeof(printbuffer) instead of CONFIG_SYS_PBSIZE
> - Drop patch which changes network code to use snprintf()
>
> Changes in v3:
> - Move prototypes from common.h to vsprintf.h
> - Add CONFIG_SYS_VSNPRINT option to enable vsnprintf() functions
> - Update README with CONFIG_SYS_VSNPRINT docs
> - Use ADDCH macro to support checking/not checking end pointer
> - Move function documentation into header file
>
> Changes in v4:
> - Add these changes in unless CONFIG_NO_SYS_VSNPRINT is defined
> - Reduce code size overhead if disabled to only 4 bytes on ARM
> - Remove the ugly #ifdef patch from series since it only saves 4 bytes
>
> Changes in v5:
> - Define INT_MAX locally within vsprintf.c
> - Drop limits.h as it is used in only two places in U-Boot
>
> Simon Glass (2):
>  Move vsprintf functions into their own header
>  vsprintf: Move function documentation into header file
>
> Sonny Rao (2):
>  Add safe vsnprintf and snprintf library functions
>  Make printf and vprintf safe from buffer overruns
>

Any more comments on this?

I have suggested making these functions available by default, and
having CONFIG_NO_SYS_VSNPRINT to remove them. The rationale is that it
if CONFIG_NO_SYS_VSNPRINT is not defined, then snprintf() will not
check the buffer length (all the code to do it is removed), and this
is unexpected. In this case snprintf() is #defined to sprintf().

There is therefore a code size increase by default with this series.
We can instead use CONFIG_SYS_VSNPRINT if people think it is safe.

Regards,
Simon


>  README             |    7 ++
>  common/console.c   |   10 +-
>  include/common.h   |   10 +--
>  include/vsprintf.h |  180 +++++++++++++++++++++++++++++++++++++++
>  lib/vsprintf.c     |  237 ++++++++++++++++++++++++++++++++-------------------
>  5 files changed, 342 insertions(+), 102 deletions(-)
>  create mode 100644 include/vsprintf.h
>
> --
> 1.7.3.1
>
>