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

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@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_VSNPRINT option undefined, 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
Changes in v6: - Change the config option to CONFIG_SYS_VSNPRINTF - Make the default be to NOT include safe printf functions
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
README | 9 ++ common/console.c | 10 +- include/common.h | 11 +-- include/vsprintf.h | 181 +++++++++++++++++++++++++++++++++++++++ lib/vsprintf.c | 237 ++++++++++++++++++++++++++++++++------------------- 5 files changed, 345 insertions(+), 103 deletions(-) create mode 100644 include/vsprintf.h

common.h is very large, so before changing the vsprintf functions, move the prototypes into their own header file.
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v3: - Move prototypes from common.h to vsprintf.h
include/common.h | 11 +---------- include/vsprintf.h | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 10 deletions(-) create mode 100644 include/vsprintf.h
diff --git a/include/common.h b/include/common.h index 5ca8820..ce80e85 100644 --- a/include/common.h +++ b/include/common.h @@ -725,16 +725,7 @@ void uuid_str_to_bin(const char *uuid, unsigned char *out); int uuid_str_valid(const char *uuid);
/* lib/vsprintf.c */ -ulong simple_strtoul(const char *cp,char **endp,unsigned int base); -int strict_strtoul(const char *cp, unsigned int base, unsigned long *res); -unsigned long long simple_strtoull(const char *cp,char **endp,unsigned int base); -long simple_strtol(const char *cp,char **endp,unsigned int base); -void panic(const char *fmt, ...) - __attribute__ ((format (__printf__, 1, 2), noreturn)); -int sprintf(char * buf, const char *fmt, ...) - __attribute__ ((format (__printf__, 2, 3))); -int vsprintf(char *buf, const char *fmt, va_list args); -char *simple_itoa(ulong i); +#include <vsprintf.h>
/* lib/strmhz.c */ char * strmhz(char *buf, unsigned long hz); diff --git a/include/vsprintf.h b/include/vsprintf.h new file mode 100644 index 0000000..0651446 --- /dev/null +++ b/include/vsprintf.h @@ -0,0 +1,39 @@ +/* + * (C) Copyright 2000-2009 + * Wolfgang Denk, DENX Software Engineering, wd@denx.de. + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#ifndef __VSPRINTF_H +#define __VSPRINTF_H + +ulong simple_strtoul(const char *cp, char **endp, unsigned int base); +int strict_strtoul(const char *cp, unsigned int base, unsigned long *res); +unsigned long long simple_strtoull(const char *cp, char **endp, + unsigned int base); +long simple_strtol(const char *cp, char **endp, unsigned int base); +void panic(const char *fmt, ...) + __attribute__ ((format (__printf__, 1, 2), noreturn)); +int sprintf(char *buf, const char *fmt, ...) + __attribute__ ((format (__printf__, 2, 3))); +int vsprintf(char *buf, const char *fmt, va_list args); +char *simple_itoa(ulong i); + +#endif

Dear Simon Glass,
In message 1320263530-22843-2-git-send-email-sjg@chromium.org you wrote:
common.h is very large, so before changing the vsprintf functions, move the prototypes into their own header file.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v3:
- Move prototypes from common.h to vsprintf.h
include/common.h | 11 +---------- include/vsprintf.h | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 10 deletions(-) create mode 100644 include/vsprintf.h
Applied to "next" branch, thanks.
Best regards,
Wolfgang Denk

From: Sonny Rao sonnyrao@chromium.org
From: Sonny Rao sonnyrao@chromium.org
These functions are useful in U-Boot because they allow a graceful failure rather than an unpredictable stack overflow when printf() buffers are exceeded.
Mostly copied from the Linux kernel. I copied vscnprintf and scnprintf so we can change printf and vprintf to use the safe implementation but still return the correct values.
(Simon Glass sjg@chromium.org modified this commit a little)
Signed-off-by: Sonny Rao sonnyrao@chromium.org --- Changes in v3: - 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
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
Changes in v6: - Change the config option to CONFIG_SYS_VSNPRINTF - Make the default be to NOT include safe printf functions
README | 9 ++ include/vsprintf.h | 19 ++++ lib/vsprintf.c | 265 +++++++++++++++++++++++++++++++++++++++++---------- 3 files changed, 241 insertions(+), 52 deletions(-)
diff --git a/README b/README index c05c40a..6fb604d 100644 --- a/README +++ b/README @@ -638,6 +638,15 @@ The following options need to be configured: 'Sane' compilers will generate smaller code if CONFIG_PRE_CON_BUF_SZ is a power of 2
+- Safe printf() functions + Define CONFIG_SYS_VSNPRINTF to compile in safe versions of + the printf() functions. These are defined in + include/vsprintf.h and include snprintf(), vsnprintf() and + so on. Code size increase is approximately 300-500 bytes. + If this option is not given then these functions will + silently discard their buffer size argument - this means + you are not getting any overflow checking in this case. + - Boot Delay: CONFIG_BOOTDELAY - in seconds Delay before automatically booting the default image; set to -1 to disable autoboot. diff --git a/include/vsprintf.h b/include/vsprintf.h index 0651446..5195598 100644 --- a/include/vsprintf.h +++ b/include/vsprintf.h @@ -36,4 +36,23 @@ int sprintf(char *buf, const char *fmt, ...) int vsprintf(char *buf, const char *fmt, va_list args); char *simple_itoa(ulong i);
+#ifdef CONFIG_SYS_VSNPRINTF +int snprintf(char *buf, size_t size, const char *fmt, ...) + __attribute__ ((format (__printf__, 3, 4))); +int scnprintf(char *buf, size_t size, const char *fmt, ...) + __attribute__ ((format (__printf__, 3, 4))); +int vsnprintf(char *buf, size_t size, const char *fmt, va_list args); +int vscnprintf(char *buf, size_t size, const char *fmt, va_list args); +#else +/* + * Use macros to silently drop the size parameter. Note that the 'cn' + * versions are the same as the 'n' versions since the functions assume + * there is always enough buffer space when !CONFIG_SYS_VSNPRINTF + */ +#define snprintf(buf, size, fmt, args...) sprintf(buf, fmt, ##args) +#define scnprintf(buf, size, fmt, args...) sprintf(buf, fmt, ##args) +#define vsnprintf(buf, size, fmt, args...) vsprintf(buf, fmt, ##args) +#define vscnprintf(buf, size, fmt, args...) vsprintf(buf, fmt, ##args) +#endif /* CONFIG_SYS_VSNPRINTF */ + #endif diff --git a/lib/vsprintf.c b/lib/vsprintf.c index e497a86..7a145ea 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -26,6 +26,9 @@ # define NUM_TYPE long long #define noinline __attribute__((noinline))
+/* some reluctance to put this into a new limits.h, so it is here */ +#define INT_MAX ((int)(~0U>>1)) + const char hex_asc[] = "0123456789abcdef"; #define hex_asc_lo(x) hex_asc[((x) & 0x0f)] #define hex_asc_hi(x) hex_asc[((x) & 0xf0) >> 4] @@ -291,7 +294,22 @@ static noinline char* put_dec(char *buf, unsigned NUM_TYPE num) #define SMALL 32 /* Must be 32 == 0x20 */ #define SPECIAL 64 /* 0x */
-static char *number(char *buf, unsigned NUM_TYPE num, int base, int size, int precision, int type) +#ifdef CONFIG_SYS_VSNPRINTF +/* + * Macro to add a new character to our output string, but only if it will + * fit. The macro moves to the next character position in the output string. + */ +#define ADDCH(str, ch) do { \ + if ((str) < end) \ + *(str) = (ch); \ + ++str; \ + } while (0) +#else +#define ADDCH(str, ch) (*(str)++ = (ch)) +#endif + +static char *number(char *buf, char *end, unsigned NUM_TYPE num, + int base, int size, int precision, int type) { /* we are called with base 8, 10 or 16, only, thus don't need "G..." */ static const char digits[16] = "0123456789ABCDEF"; /* "GHIJKLMNOPQRSTUVWXYZ"; */ @@ -353,37 +371,40 @@ static char *number(char *buf, unsigned NUM_TYPE num, int base, int size, int pr precision = i; /* leading space padding */ size -= precision; - if (!(type & (ZEROPAD+LEFT))) - while(--size >= 0) - *buf++ = ' '; + if (!(type & (ZEROPAD + LEFT))) { + while (--size >= 0) + ADDCH(buf, ' '); + } /* sign */ if (sign) - *buf++ = sign; + ADDCH(buf, sign); /* "0x" / "0" prefix */ if (need_pfx) { - *buf++ = '0'; + ADDCH(buf, '0'); if (base == 16) - *buf++ = ('X' | locase); + ADDCH(buf, 'X' | locase); } /* zero or space padding */ if (!(type & LEFT)) { char c = (type & ZEROPAD) ? '0' : ' '; + while (--size >= 0) - *buf++ = c; + ADDCH(buf, c); } /* hmm even more zero padding? */ while (i <= --precision) - *buf++ = '0'; + ADDCH(buf, '0'); /* actual digits of result */ while (--i >= 0) - *buf++ = tmp[i]; + ADDCH(buf, tmp[i]); /* trailing space padding */ while (--size >= 0) - *buf++ = ' '; + ADDCH(buf, ' '); return buf; }
-static char *string(char *buf, char *s, int field_width, int precision, int flags) +static char *string(char *buf, char *end, char *s, int field_width, + int precision, int flags) { int len, i;
@@ -394,16 +415,16 @@ static char *string(char *buf, char *s, int field_width, int precision, int flag
if (!(flags & LEFT)) while (len < field_width--) - *buf++ = ' '; + ADDCH(buf, ' '); for (i = 0; i < len; ++i) - *buf++ = *s++; + ADDCH(buf, *s++); while (len < field_width--) - *buf++ = ' '; + ADDCH(buf, ' '); return buf; }
#ifdef CONFIG_CMD_NET -static char *mac_address_string(char *buf, u8 *addr, int field_width, +static char *mac_address_string(char *buf, char *end, u8 *addr, int field_width, int precision, int flags) { char mac_addr[6 * 3]; /* (6 * 2 hex digits), 5 colons and trailing zero */ @@ -417,10 +438,11 @@ static char *mac_address_string(char *buf, u8 *addr, int field_width, } *p = '\0';
- return string(buf, mac_addr, field_width, precision, flags & ~SPECIAL); + return string(buf, end, mac_addr, field_width, precision, + flags & ~SPECIAL); }
-static char *ip6_addr_string(char *buf, u8 *addr, int field_width, +static char *ip6_addr_string(char *buf, char *end, u8 *addr, int field_width, int precision, int flags) { char ip6_addr[8 * 5]; /* (8 * 4 hex digits), 7 colons and trailing zero */ @@ -435,10 +457,11 @@ static char *ip6_addr_string(char *buf, u8 *addr, int field_width, } *p = '\0';
- return string(buf, ip6_addr, field_width, precision, flags & ~SPECIAL); + return string(buf, end, ip6_addr, field_width, precision, + flags & ~SPECIAL); }
-static char *ip4_addr_string(char *buf, u8 *addr, int field_width, +static char *ip4_addr_string(char *buf, char *end, u8 *addr, int field_width, int precision, int flags) { char ip4_addr[4 * 4]; /* (4 * 3 decimal digits), 3 dots and trailing zero */ @@ -456,7 +479,8 @@ static char *ip4_addr_string(char *buf, u8 *addr, int field_width, } *p = '\0';
- return string(buf, ip4_addr, field_width, precision, flags & ~SPECIAL); + return string(buf, end, ip4_addr, field_width, precision, + flags & ~SPECIAL); } #endif
@@ -478,10 +502,12 @@ static char *ip4_addr_string(char *buf, u8 *addr, int field_width, * function pointers are really function descriptors, which contain a * pointer to the real address. */ -static char *pointer(const char *fmt, char *buf, void *ptr, int field_width, int precision, int flags) +static char *pointer(const char *fmt, char *buf, char *end, void *ptr, + int field_width, int precision, int flags) { if (!ptr) - return string(buf, "(null)", field_width, precision, flags); + return string(buf, end, "(null)", field_width, precision, + flags);
#ifdef CONFIG_CMD_NET switch (*fmt) { @@ -489,15 +515,18 @@ static char *pointer(const char *fmt, char *buf, void *ptr, int field_width, int flags |= SPECIAL; /* Fallthrough */ case 'M': - return mac_address_string(buf, ptr, field_width, precision, flags); + return mac_address_string(buf, end, ptr, field_width, + precision, flags); case 'i': flags |= SPECIAL; /* Fallthrough */ case 'I': if (fmt[1] == '6') - return ip6_addr_string(buf, ptr, field_width, precision, flags); + return ip6_addr_string(buf, end, ptr, field_width, + precision, flags); if (fmt[1] == '4') - return ip4_addr_string(buf, ptr, field_width, precision, flags); + return ip4_addr_string(buf, end, ptr, field_width, + precision, flags); flags &= ~SPECIAL; break; } @@ -507,27 +536,31 @@ static char *pointer(const char *fmt, char *buf, void *ptr, int field_width, int field_width = 2*sizeof(void *); flags |= ZEROPAD; } - return number(buf, (unsigned long) ptr, 16, field_width, precision, flags); + return number(buf, end, (unsigned long)ptr, 16, field_width, + precision, flags); }
/** - * vsprintf - Format a string and place it in a buffer - * @buf: The buffer to place the result into - * @fmt: The format string to use - * @args: Arguments for the format string + * Format a string and place it in a buffer (base function) + * + * @param buf The buffer to place the result into + * @param size The size of the buffer, including the trailing null space + * @param fmt The format string to use + * @param args Arguments for the format string + * @return The number characters which would be generated for the given + * input, excluding the trailing '\0', as per ISO C99. Note that fewer + * characters may be written if this number of characters is >= size. * - * This function follows C99 vsprintf, but has some extensions: + * This function follows C99 vsnprintf, but has some extensions: * %pS output the name of a text symbol * %pF output the name of a function pointer * %pR output the address range in a struct resource * - * The function returns the number of characters written - * into @buf. - * * Call this function if you are already dealing with a va_list. - * You probably want sprintf() instead. + * You probably want snprintf() instead. */ -int vsprintf(char *buf, const char *fmt, va_list args) +static int vsnprintf_internal(char *buf, size_t size, const char *fmt, + va_list args) { unsigned NUM_TYPE num; int base; @@ -542,12 +575,20 @@ int vsprintf(char *buf, const char *fmt, va_list args) /* 'z' support added 23/7/1999 S.H. */ /* 'z' changed to 'Z' --davidm 1/25/99 */ /* 't' added for ptrdiff_t */ + char *end = buf + size;
+#ifdef CONFIG_SYS_VSNPRINTF + /* Make sure end is always >= buf - do we want this in U-Boot? */ + if (end < buf) { + end = ((void *)-1); + size = end - buf; + } +#endif str = buf;
for (; *fmt ; ++fmt) { if (*fmt != '%') { - *str++ = *fmt; + ADDCH(str, *fmt); continue; }
@@ -609,20 +650,22 @@ int vsprintf(char *buf, const char *fmt, va_list args)
switch (*fmt) { case 'c': - if (!(flags & LEFT)) + if (!(flags & LEFT)) { while (--field_width > 0) - *str++ = ' '; - *str++ = (unsigned char) va_arg(args, int); + ADDCH(str, ' '); + } + ADDCH(str, (unsigned char) va_arg(args, int)); while (--field_width > 0) - *str++ = ' '; + ADDCH(str, ' '); continue;
case 's': - str = string(str, va_arg(args, char *), field_width, precision, flags); + str = string(str, end, va_arg(args, char *), + field_width, precision, flags); continue;
case 'p': - str = pointer(fmt+1, str, + str = pointer(fmt+1, str, end, va_arg(args, void *), field_width, precision, flags); /* Skip all alphanumeric pointer suffixes */ @@ -641,7 +684,7 @@ int vsprintf(char *buf, const char *fmt, va_list args) continue;
case '%': - *str++ = '%'; + ADDCH(str, '%'); continue;
/* integer number formats - set up the flags and "break" */ @@ -662,9 +705,9 @@ int vsprintf(char *buf, const char *fmt, va_list args) break;
default: - *str++ = '%'; + ADDCH(str, '%'); if (*fmt) - *str++ = *fmt; + ADDCH(str, *fmt); else --fmt; continue; @@ -688,17 +731,135 @@ int vsprintf(char *buf, const char *fmt, va_list args) if (flags & SIGN) num = (signed int) num; } - str = number(str, num, base, field_width, precision, flags); + str = number(str, end, num, base, field_width, precision, + flags); } + +#ifdef CONFIG_SYS_VSNPRINTF + if (size > 0) { + ADDCH(str, '\0'); + if (str > end) + end[-1] = '\0'; + } +#else *str = '\0'; +#endif + /* the trailing null byte doesn't count towards the total */ return str-buf; }
+#ifdef CONFIG_SYS_VSNPRINTF +int vsnprintf(char *buf, size_t size, const char *fmt, + va_list args) +{ + return vsnprintf_internal(buf, size, fmt, args); +} + +/** + * Format a string and place it in a buffer (va_list version) + * + * @param buf The buffer to place the result into + * @param size The size of the buffer, including the trailing null space + * @param fmt The format string to use + * @param args Arguments for the format string + * @return the number of characters which have been written into + * the @buf not including the trailing '\0'. If @size is == 0 the function + * returns 0. + * + * If you're not already dealing with a va_list consider using scnprintf(). + * + * See the vsprintf() documentation for format string extensions over C99. + */ +int vscnprintf(char *buf, size_t size, const char *fmt, va_list args) +{ + int i; + + i = vsnprintf(buf, size, fmt, args); + + if (likely(i < size)) + return i; + if (size != 0) + return size - 1; + return 0; +} + /** - * sprintf - Format a string and place it in a buffer - * @buf: The buffer to place the result into - * @fmt: The format string to use - * @...: Arguments for the format string + * Format a string and place it in a buffer + * + * @param buf The buffer to place the result into + * @param size The size of the buffer, including the trailing null space + * @param fmt The format string to use + * @param ... Arguments for the format string + * @return the number of characters which would be + * generated for the given input, excluding the trailing null, + * as per ISO C99. If the return is greater than or equal to + * @size, the resulting string is truncated. + * + * See the vsprintf() documentation for format string extensions over C99. + */ +int snprintf(char *buf, size_t size, const char *fmt, ...) +{ + va_list args; + int i; + + va_start(args, fmt); + i = vsnprintf(buf, size, fmt, args); + va_end(args); + + return i; +} + +/** + * Format a string and place it in a buffer + * + * @param buf The buffer to place the result into + * @param size The size of the buffer, including the trailing null space + * @param fmt The format string to use + * @param ... Arguments for the format string + * + * The return value is the number of characters written into @buf not including + * the trailing '\0'. If @size is == 0 the function returns 0. + * + * See the vsprintf() documentation for format string extensions over C99. + */ + +int scnprintf(char *buf, size_t size, const char *fmt, ...) +{ + va_list args; + int i; + + va_start(args, fmt); + i = vscnprintf(buf, size, fmt, args); + va_end(args); + + return i; +} +#endif /* CONFIG_SYS_VSNPRINT */ + +/** + * Format a string and place it in a buffer (va_list version) + * + * @param buf The buffer to place the result into + * @param fmt The format string to use + * @param args Arguments for the format string + * + * The function returns the number of characters written + * into @buf. Use vsnprintf() or vscnprintf() in order to avoid + * buffer overflows. + * + * If you're not already dealing with a va_list consider using sprintf(). + */ +int vsprintf(char *buf, const char *fmt, va_list args) +{ + return vsnprintf_internal(buf, INT_MAX, fmt, args); +} + +/** + * Format a string and place it in a buffer + * + * @param buf The buffer to place the result into + * @param fmt The format string to use + * @param ... Arguments for the format string * * The function returns the number of characters written * into @buf.

Dear Simon Glass,
In message 1320263530-22843-3-git-send-email-sjg@chromium.org you wrote:
From: Sonny Rao sonnyrao@chromium.org
From: Sonny Rao sonnyrao@chromium.org
These functions are useful in U-Boot because they allow a graceful failure rather than an unpredictable stack overflow when printf() buffers are exceeded.
Mostly copied from the Linux kernel. I copied vscnprintf and scnprintf so we can change printf and vprintf to use the safe implementation but still return the correct values.
(Simon Glass sjg@chromium.org modified this commit a little)
Signed-off-by: Sonny Rao sonnyrao@chromium.org
Changes in v3:
- 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
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
Changes in v6:
- Change the config option to CONFIG_SYS_VSNPRINTF
- Make the default be to NOT include safe printf functions
README | 9 ++ include/vsprintf.h | 19 ++++ lib/vsprintf.c | 265 +++++++++++++++++++++++++++++++++++++++++---------- 3 files changed, 241 insertions(+), 52 deletions(-)
Applied to "next" branch, thanks.
Best regards,
Wolfgang Denk

Now that this is not in common.h, perhaps it is acceptable to move this documentation into the header file.
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v3: - Move function documentation into header file
include/vsprintf.h | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++ lib/vsprintf.c | 100 ------------------------------------------ 2 files changed, 123 insertions(+), 100 deletions(-)
diff --git a/include/vsprintf.h b/include/vsprintf.h index 5195598..651077c 100644 --- a/include/vsprintf.h +++ b/include/vsprintf.h @@ -25,23 +25,146 @@ #define __VSPRINTF_H
ulong simple_strtoul(const char *cp, char **endp, unsigned int base); + +/** + * strict_strtoul - convert a string to an unsigned long strictly + * @param cp The string to be converted + * @param base The number base to use + * @param res The converted result value + * @return 0 if conversion is successful and *res is set to the converted + * value, otherwise it returns -EINVAL and *res is set to 0. + * + * strict_strtoul converts a string to an unsigned long only if the + * string is really an unsigned long string, any string containing + * any invalid char at the tail will be rejected and -EINVAL is returned, + * only a newline char at the tail is acceptible because people generally + * change a module parameter in the following way: + * + * echo 1024 > /sys/module/e1000/parameters/copybreak + * + * echo will append a newline to the tail. + * + * simple_strtoul just ignores the successive invalid characters and + * return the converted value of prefix part of the string. + * + * Copied this function from Linux 2.6.38 commit ID: + * 521cb40b0c44418a4fd36dc633f575813d59a43d + * + */ int strict_strtoul(const char *cp, unsigned int base, unsigned long *res); unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int base); long simple_strtol(const char *cp, char **endp, unsigned int base); void panic(const char *fmt, ...) __attribute__ ((format (__printf__, 1, 2), noreturn)); + +/** + * Format a string and place it in a buffer + * + * @param buf The buffer to place the result into + * @param fmt The format string to use + * @param ... Arguments for the format string + * + * The function returns the number of characters written + * into @buf. + * + * See the vsprintf() documentation for format string extensions over C99. + */ int sprintf(char *buf, const char *fmt, ...) __attribute__ ((format (__printf__, 2, 3))); + +/** + * Format a string and place it in a buffer (va_list version) + * + * @param buf The buffer to place the result into + * @param size The size of the buffer, including the trailing null space + * @param fmt The format string to use + * @param args Arguments for the format string + * @return the number of characters which have been written into + * the @buf not including the trailing '\0'. If @size is == 0 the function + * returns 0. + * + * If you're not already dealing with a va_list consider using scnprintf(). + * + * See the vsprintf() documentation for format string extensions over C99. + */ int vsprintf(char *buf, const char *fmt, va_list args); char *simple_itoa(ulong i);
#ifdef CONFIG_SYS_VSNPRINTF +/** + * Format a string and place it in a buffer + * + * @param buf The buffer to place the result into + * @param size The size of the buffer, including the trailing null space + * @param fmt The format string to use + * @param ... Arguments for the format string + * @return the number of characters which would be + * generated for the given input, excluding the trailing null, + * as per ISO C99. If the return is greater than or equal to + * @size, the resulting string is truncated. + * + * See the vsprintf() documentation for format string extensions over C99. + */ int snprintf(char *buf, size_t size, const char *fmt, ...) __attribute__ ((format (__printf__, 3, 4))); + +/** + * Format a string and place it in a buffer + * + * @param buf The buffer to place the result into + * @param size The size of the buffer, including the trailing null space + * @param fmt The format string to use + * @param ... Arguments for the format string + * + * The return value is the number of characters written into @buf not including + * the trailing '\0'. If @size is == 0 the function returns 0. + * + * See the vsprintf() documentation for format string extensions over C99. + */ int scnprintf(char *buf, size_t size, const char *fmt, ...) __attribute__ ((format (__printf__, 3, 4))); + +/** + * Format a string and place it in a buffer (base function) + * + * @param buf The buffer to place the result into + * @param size The size of the buffer, including the trailing null space + * @param fmt The format string to use + * @param args Arguments for the format string + * @return The number characters which would be generated for the given + * input, excluding the trailing '\0', as per ISO C99. Note that fewer + * characters may be written if this number of characters is >= size. + * + * This function follows C99 vsnprintf, but has some extensions: + * %pS output the name of a text symbol + * %pF output the name of a function pointer + * %pR output the address range in a struct resource + * + * The function returns the number of characters which would be + * generated for the given input, excluding the trailing '\0', + * as per ISO C99. + * + * Call this function if you are already dealing with a va_list. + * You probably want snprintf() instead. + */ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args); + +/** + * Format a string and place it in a buffer (va_list version) + * + * @param buf The buffer to place the result into + * @param size The size of the buffer, including the trailing null space + * @param fmt The format string to use + * @param args Arguments for the format string + * @return the number of characters which have been written into + * the @buf not including the trailing '\0'. If @size is == 0 the function + * returns 0. + * + * If you're not already dealing with a va_list consider using scnprintf(). + * + * See the vsprintf() documentation for format string extensions over C99. + */ int vscnprintf(char *buf, size_t size, const char *fmt, va_list args); #else /* diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 7a145ea..e38a4b7 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -67,32 +67,6 @@ unsigned long simple_strtoul(const char *cp,char **endp,unsigned int base) return result; }
-/** - * strict_strtoul - convert a string to an unsigned long strictly - * @cp: The string to be converted - * @base: The number base to use - * @res: The converted result value - * - * strict_strtoul converts a string to an unsigned long only if the - * string is really an unsigned long string, any string containing - * any invalid char at the tail will be rejected and -EINVAL is returned, - * only a newline char at the tail is acceptible because people generally - * change a module parameter in the following way: - * - * echo 1024 > /sys/module/e1000/parameters/copybreak - * - * echo will append a newline to the tail. - * - * It returns 0 if conversion is successful and *res is set to the converted - * value, otherwise it returns -EINVAL and *res is set to 0. - * - * simple_strtoul just ignores the successive invalid characters and - * return the converted value of prefix part of the string. - * - * Copied this function from Linux 2.6.38 commit ID: - * 521cb40b0c44418a4fd36dc633f575813d59a43d - * - */ int strict_strtoul(const char *cp, unsigned int base, unsigned long *res) { char *tail; @@ -540,25 +514,6 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr, precision, flags); }
-/** - * Format a string and place it in a buffer (base function) - * - * @param buf The buffer to place the result into - * @param size The size of the buffer, including the trailing null space - * @param fmt The format string to use - * @param args Arguments for the format string - * @return The number characters which would be generated for the given - * input, excluding the trailing '\0', as per ISO C99. Note that fewer - * characters may be written if this number of characters is >= size. - * - * This function follows C99 vsnprintf, but has some extensions: - * %pS output the name of a text symbol - * %pF output the name of a function pointer - * %pR output the address range in a struct resource - * - * Call this function if you are already dealing with a va_list. - * You probably want snprintf() instead. - */ static int vsnprintf_internal(char *buf, size_t size, const char *fmt, va_list args) { @@ -755,21 +710,6 @@ int vsnprintf(char *buf, size_t size, const char *fmt, return vsnprintf_internal(buf, size, fmt, args); }
-/** - * Format a string and place it in a buffer (va_list version) - * - * @param buf The buffer to place the result into - * @param size The size of the buffer, including the trailing null space - * @param fmt The format string to use - * @param args Arguments for the format string - * @return the number of characters which have been written into - * the @buf not including the trailing '\0'. If @size is == 0 the function - * returns 0. - * - * If you're not already dealing with a va_list consider using scnprintf(). - * - * See the vsprintf() documentation for format string extensions over C99. - */ int vscnprintf(char *buf, size_t size, const char *fmt, va_list args) { int i; @@ -783,20 +723,6 @@ int vscnprintf(char *buf, size_t size, const char *fmt, va_list args) return 0; }
-/** - * Format a string and place it in a buffer - * - * @param buf The buffer to place the result into - * @param size The size of the buffer, including the trailing null space - * @param fmt The format string to use - * @param ... Arguments for the format string - * @return the number of characters which would be - * generated for the given input, excluding the trailing null, - * as per ISO C99. If the return is greater than or equal to - * @size, the resulting string is truncated. - * - * See the vsprintf() documentation for format string extensions over C99. - */ int snprintf(char *buf, size_t size, const char *fmt, ...) { va_list args; @@ -809,20 +735,6 @@ int snprintf(char *buf, size_t size, const char *fmt, ...) return i; }
-/** - * Format a string and place it in a buffer - * - * @param buf The buffer to place the result into - * @param size The size of the buffer, including the trailing null space - * @param fmt The format string to use - * @param ... Arguments for the format string - * - * The return value is the number of characters written into @buf not including - * the trailing '\0'. If @size is == 0 the function returns 0. - * - * See the vsprintf() documentation for format string extensions over C99. - */ - int scnprintf(char *buf, size_t size, const char *fmt, ...) { va_list args; @@ -854,18 +766,6 @@ int vsprintf(char *buf, const char *fmt, va_list args) return vsnprintf_internal(buf, INT_MAX, fmt, args); }
-/** - * Format a string and place it in a buffer - * - * @param buf The buffer to place the result into - * @param fmt The format string to use - * @param ... Arguments for the format string - * - * The function returns the number of characters written - * into @buf. - * - * See the vsprintf() documentation for format string extensions over C99. - */ int sprintf(char * buf, const char *fmt, ...) { va_list args;

Dear Simon Glass,
In message 1320263530-22843-4-git-send-email-sjg@chromium.org you wrote:
Now that this is not in common.h, perhaps it is acceptable to move this documentation into the header file.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v3:
- Move function documentation into header file
include/vsprintf.h | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++ lib/vsprintf.c | 100 ------------------------------------------ 2 files changed, 123 insertions(+), 100 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk

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: Sonny Rao sonnyrao@chromium.org --- Changes in v2: - Use sizeof(printbuffer) instead of CONFIG_SYS_PBSIZE - Drop patch which changes network code to use snprintf()
Changes in v5: - Drop limits.h as it is used in only two places in U-Boot
common/console.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/common/console.c b/common/console.c index f17875e..1177f7d 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, sizeof(printbuffer), 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, sizeof(printbuffer), fmt, args); va_end(args);
/* Send to desired file */ @@ -426,7 +426,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, sizeof(printbuffer), fmt, args); va_end(args);
/* Print the string */ @@ -447,7 +447,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, sizeof(printbuffer), fmt, args);
/* Print the string */ puts(printbuffer); @@ -514,7 +514,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, sizeof(printbuffer), fmt, args); va_end(args);
if ((screen + sizeof(screen) - 1 - cursor)

Dear Simon Glass,
In message 1320263530-22843-5-git-send-email-sjg@chromium.org you 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: Sonny Rao sonnyrao@chromium.org
Changes in v2:
- Use sizeof(printbuffer) instead of CONFIG_SYS_PBSIZE
- Drop patch which changes network code to use snprintf()
Changes in v5:
- Drop limits.h as it is used in only two places in U-Boot
common/console.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
Applied to "next" branch, thanks.
Best regards,
Wolfgang Denk

Hi,
On Wed, Nov 2, 2011 at 12:52 PM, Simon Glass sjg@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.
Does any maintainer want to pick up this series? It passes the MAKEALL test for me.
Regards, Simon
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@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_VSNPRINT option undefined, 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
Changes in v6:
- Change the config option to CONFIG_SYS_VSNPRINTF
- Make the default be to NOT include safe printf functions
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
README | 9 ++ common/console.c | 10 +- include/common.h | 11 +-- include/vsprintf.h | 181 +++++++++++++++++++++++++++++++++++++++ lib/vsprintf.c | 237 ++++++++++++++++++++++++++++++++------------------- 5 files changed, 345 insertions(+), 103 deletions(-) create mode 100644 include/vsprintf.h
-- 1.7.3.1

Hi Wolfgang,
On Mon, Nov 21, 2011 at 12:43 PM, Simon Glass sjg@chromium.org wrote:
Hi,
On Wed, Nov 2, 2011 at 12:52 PM, Simon Glass sjg@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.
Does any maintainer want to pick up this series? It passes the MAKEALL test for me.
Is this series going into this month's release?
Regards, Simon
Regards, Simon
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@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_VSNPRINT option undefined, 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
Changes in v6:
- Change the config option to CONFIG_SYS_VSNPRINTF
- Make the default be to NOT include safe printf functions
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
README | 9 ++ common/console.c | 10 +- include/common.h | 11 +-- include/vsprintf.h | 181 +++++++++++++++++++++++++++++++++++++++ lib/vsprintf.c | 237 ++++++++++++++++++++++++++++++++------------------- 5 files changed, 345 insertions(+), 103 deletions(-) create mode 100644 include/vsprintf.h
-- 1.7.3.1

Dear Simon Glass,
In message CAPnjgZ0MFpTsLghMxEOP-nvpe=H-v2p4r3eptDpv7rV8XSrNPw@mail.gmail.com you wrote:
Is this series going into this month's release?
No. This goes into "next".
Best regards,
Wolfgang Denk
participants (2)
-
Simon Glass
-
Wolfgang Denk