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

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

[Simon Glass]

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, and also
some network code (as an example of where this might lead).


Simon Glass (2):
  Add limits.h to hold basic limits
  Use snprintf() in network code

Sonny Rao (2):
  Add safe vsnprintf and snprintf library functions
  Make printf and vprintf safe from buffer overruns

 common/console.c |   10 +-
 fs/ubifs/ubifs.h |    4 +-
 include/common.h |    8 ++-
 include/limits.h |   40 +++++++
 lib/vsprintf.c   |  316 ++++++++++++++++++++++++++++++++++++++++++------------
 net/eth.c        |   10 ++-
 net/net.c        |   15 ++-
 net/nfs.c        |    3 +-
 net/tftp.c       |    3 +-
 9 files changed, 323 insertions(+), 86 deletions(-)
 create mode 100644 include/limits.h

-- 
1.7.3.1

[U-Boot] [PATCH 1/4] Add limits.h to hold basic limits

[Simon Glass]

This brings a basic limits.h implementation into U-Boot.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 fs/ubifs/ubifs.h |    4 +---
 include/limits.h |   40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 3 deletions(-)
 create mode 100644 include/limits.h

diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 0af471a..e7b6e43 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -311,9 +311,7 @@ struct file {
 #define MAX_LFS_FILESIZE 	0x7fffffffffffffffUL
 #endif
 
-#define INT_MAX		((int)(~0U>>1))
-#define INT_MIN		(-INT_MAX - 1)
-#define LLONG_MAX	((long long)(~0ULL>>1))
+#include <limits.h>
 
 /*
  * These are the fs-independent mount-flags: up to 32 flags are supported
diff --git a/include/limits.h b/include/limits.h
new file mode 100644
index 0000000..1021291
--- /dev/null
+++ b/include/limits.h
@@ -0,0 +1,40 @@
+/*
+ * Copyright (c) 2011 The Chromium OS Authors.
+ * 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 __LIMITS_H
+#define __LIMITS_H
+
+/* Keep all our limits in one place */
+
+#define USHRT_MAX	((u16)(~0U))
+#define SHRT_MAX	((s16)(USHRT_MAX>>1))
+#define SHRT_MIN	((s16)(-SHRT_MAX - 1))
+#define INT_MAX		((int)(~0U>>1))
+#define INT_MIN		(-INT_MAX - 1)
+#define UINT_MAX	(~0U)
+#define LONG_MAX	((long)(~0UL>>1))
+#define LONG_MIN	(-LONG_MAX - 1)
+#define ULONG_MAX	(~0UL)
+#define LLONG_MAX	((long long)(~0ULL>>1))
+#define LLONG_MIN	(-LLONG_MAX - 1)
+#define ULLONG_MAX	(~0ULL)
+
+#endif
-- 
1.7.3.1

[U-Boot] [PATCH 2/4] Add safe vsnprintf and snprintf library functions

[Simon Glass]

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.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 include/common.h |    8 ++-
 lib/vsprintf.c   |  316 ++++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 256 insertions(+), 68 deletions(-)

diff --git a/include/common.h b/include/common.h
index d244bd4..fbcc55f 100644
--- a/include/common.h
+++ b/include/common.h
@@ -682,9 +682,15 @@ 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, ...)
+int	sprintf(char *buf, const char *fmt, ...)
 		__attribute__ ((format (__printf__, 2, 3)));
+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	vsprintf(char *buf, const char *fmt, va_list args);
+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);
 
 /* lib/strmhz.c */
 char *	strmhz(char *buf, unsigned long hz);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 79dead3..bac6f30 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -16,6 +16,7 @@
 #include <errno.h>
 
 #include <common.h>
+#include <limits.h>
 #if !defined (CONFIG_PANIC_HANG)
 #include <command.h>
 #endif
@@ -289,7 +290,8 @@ 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)
+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"; */
@@ -351,37 +353,63 @@ 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) {
+			if (buf < end)
+				*buf = ' ';
+			++buf;
+		}
+	}
 	/* sign */
-	if (sign)
-		*buf++ = sign;
+	if (sign) {
+		if (buf < end)
+			*buf = sign;
+		++buf;
+	}
 	/* "0x" / "0" prefix */
 	if (need_pfx) {
-		*buf++ = '0';
-		if (base == 16)
-			*buf++ = ('X' | locase);
+		if (buf < end)
+			*buf = '0';
+		++buf;
+		if (base == 16) {
+			if (buf < end)
+				*buf = ('X' | locase);
+			++buf;
+		}
 	}
 	/* zero or space padding */
 	if (!(type & LEFT)) {
 		char c = (type & ZEROPAD) ? '0' : ' ';
-		while (--size >= 0)
-			*buf++ = c;
+		while (--size >= 0) {
+			if (buf < end)
+				*buf = c;
+			++buf;
+		}
 	}
 	/* hmm even more zero padding? */
-	while (i <= --precision)
-		*buf++ = '0';
+	while (i <= --precision) {
+		if (buf < end)
+			*buf = '0';
+		++buf;
+	}
 	/* actual digits of result */
-	while (--i >= 0)
-		*buf++ = tmp[i];
+	while (--i >= 0) {
+		if (buf < end)
+			*buf = tmp[i];
+		++buf;
+
+	}
 	/* trailing space padding */
-	while (--size >= 0)
-		*buf++ = ' ';
+	while (--size >= 0) {
+		if (buf < end)
+			*buf = ' ';
+		++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;
 
@@ -391,17 +419,26 @@ static char *string(char *buf, char *s, int field_width, int precision, int flag
 	len = strnlen(s, precision);
 
 	if (!(flags & LEFT))
-		while (len < field_width--)
-			*buf++ = ' ';
-	for (i = 0; i < len; ++i)
-		*buf++ = *s++;
-	while (len < field_width--)
-		*buf++ = ' ';
+		while (len < field_width--) {
+			if (buf < end)
+				*buf = ' ';
+			++buf;
+		}
+	for (i = 0; i < len; ++i) {
+		if (buf < end)
+			*buf = *s++;
+		++buf;
+	}
+	while (len < field_width--) {
+		if (buf < end)
+			*buf = ' ';
+		++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 */
@@ -415,10 +452,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 */
@@ -433,10 +471,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 */
@@ -454,7 +493,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
 
@@ -476,10 +516,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) {
@@ -487,15 +529,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;
 	}
@@ -505,31 +550,35 @@ 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)
  *
- * This function follows C99 vsprintf, but has some extensions:
+ * @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
+ *
+ * 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.
+ * 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 sprintf() instead.
+ * You probably want snprintf() instead.
  */
-int vsprintf(char *buf, const char *fmt, va_list args)
+int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 {
 	unsigned NUM_TYPE num;
 	int base;
-	char *str;
+	char *str, *end;
 
 	int flags;		/* flags to number() */
 
@@ -542,10 +591,19 @@ int vsprintf(char *buf, const char *fmt, va_list args)
 				/* 't' added for ptrdiff_t */
 
 	str = buf;
+	end = str + size;
+
+	/* Make sure end is always >= buf */
+	if (end < buf) {
+		end = ((void *)-1);
+		size = end - buf;
+	}
 
 	for (; *fmt ; ++fmt) {
 		if (*fmt != '%') {
-			*str++ = *fmt;
+			if (str < end)
+				*str = *fmt;
+			++str;
 			continue;
 		}
 
@@ -606,21 +664,32 @@ int vsprintf(char *buf, const char *fmt, va_list args)
 		base = 10;
 
 		switch (*fmt) {
-		case 'c':
+		case 'c': {
+			char c;
 			if (!(flags & LEFT))
-				while (--field_width > 0)
-					*str++ = ' ';
-			*str++ = (unsigned char) va_arg(args, int);
-			while (--field_width > 0)
-				*str++ = ' ';
+				while (--field_width > 0) {
+					if (str < end)
+						*str = ' ';
+					++str;
+				}
+			c = (unsigned char) va_arg(args, int);
+			if (str < end)
+				*str = c;
+			++str;
+			while (--field_width > 0) {
+				if (str < end)
+					*str = ' ';
+				++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 */
@@ -639,7 +708,9 @@ int vsprintf(char *buf, const char *fmt, va_list args)
 			continue;
 
 		case '%':
-			*str++ = '%';
+			if (str < end)
+				*str = '%';
+			++str;
 			continue;
 
 		/* integer number formats - set up the flags and "break" */
@@ -660,10 +731,14 @@ int vsprintf(char *buf, const char *fmt, va_list args)
 			break;
 
 		default:
-			*str++ = '%';
-			if (*fmt)
-				*str++ = *fmt;
-			else
+			if (str < end)
+				*str = '%';
+			++str;
+			if (*fmt) {
+				if (str < end)
+					*str = *fmt;
+				++str;
+			} else
 				--fmt;
 			continue;
 		}
@@ -686,17 +761,124 @@ 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);
+	}
+
+	if (size > 0) {
+		if (str < end)
+			*str = '\0';
+		else
+			end[-1] = '\0';
 	}
-	*str = '\0';
+	/* the trailing null byte doesn't count towards the total */
 	return str-buf;
 }
 
 /**
- * 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 (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;
+}
+
+/**
+ * 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;
+}
+
+/**
+ * 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(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.
-- 
1.7.3.1

[U-Boot] [PATCH 2/4] Add safe vsnprintf and snprintf library functions

[Graeme Russ]

Hi Simon,

On 24/09/11 03:38, Simon Glass 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.

Have you checked for license compatibility? U-Boot is GPLv2+ and (most) of
Linux is GPLv2 - You may not be legally permitted to do this

Regards,

Graeme

[U-Boot] [PATCH 2/4] Add safe vsnprintf and snprintf library functions

[Sonny Rao]

On Fri, Sep 23, 2011 at 4:56 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
> Hi Simon,
>
> On 24/09/11 03:38, Simon Glass 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.
>
> Have you checked for license compatibility? U-Boot is GPLv2+ and (most) of
> Linux is GPLv2 - You may not be legally permitted to do this

According to the FSF site,  GPLv2 is compatible with GPLv3, see:
http://www.gnu.org/licenses/quick-guide-gplv3.html

So it's fine to distribute them together.

In reality though, this code in U-boot was already copied from the
same file in an older version of the kernel.  The license (GPLv2 only)
hasn't changed on that file, so U-boot is already distributing what is
GPLv2 only code alongside GPLv2+ code -- which as I mentioned above is
fine.

The code here is derived from a later version of that same file, so I
don't believe integrating this patch into U-boot actually changes
anything with respect to licensing of this code.

Sonny

[U-Boot] [PATCH 2/4] Add safe vsnprintf and snprintf library functions

[Graeme Russ]

Hi Sonny,

On Thu, Sep 29, 2011 at 9:26 AM, Sonny Rao <sonnyrao@chromium.org> wrote:
> On Fri, Sep 23, 2011 at 4:56 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
>> Hi Simon,
>>
>> On 24/09/11 03:38, Simon Glass 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.
>>
>> Have you checked for license compatibility? U-Boot is GPLv2+ and (most) of
>> Linux is GPLv2 - You may not be legally permitted to do this
>
> According to the FSF site,  GPLv2 is compatible with GPLv3, see:
> http://www.gnu.org/licenses/quick-guide-gplv3.html
>
> So it's fine to distribute them together.

Not so fast:

"GPLv2 is compatible with GPLv3 if the program allows you to choose "any
later version" of the GPL, which is the case for most software released
under this license"

'most', not 'all'

The key phrase is "any later version" - If this is not in the GPLv2
license text for the code your copying, you cannot license the derivative
work under GPLv3 (or GPLv2+)

> In reality though, this code in U-boot was already copied from the
> same file in an older version of the kernel.  The license (GPLv2 only)
> hasn't changed on that file, so U-boot is already distributing what is
> GPLv2 only code alongside GPLv2+ code -- which as I mentioned above is
> fine.

As long as you are not incorporating GPLv2 code into a file licensed under
GPLv2+ and distributing the result as GPLv2+ or GPLv3.

> The code here is derived from a later version of that same file, so I
> don't believe integrating this patch into U-boot actually changes
> anything with respect to licensing of this code.

I think U-Boot will have great difficulty going past GPLv2 due to the
large volume of Linus code already in U-Boot - We would need to either get
the original code relicensed GPLv2+ or rewrite it

And from the COPYING file in the Linux source:

Also note that the only valid version of the GPL as far as the kernel
is concerned is _this_ particular version of the license (ie v2, not
v2.2 or v3.x or whatever), unless explicitly otherwise stated.

and linux/lib/vsprintf.c:

/*
 *  linux/lib/vsprintf.c
 *
 *  Copyright (C) 1991, 1992  Linus Torvalds
 */

/* vsprintf.c -- Lars Wirzenius & Linus Torvalds. */
/*
 * Wirzenius wrote this portably, Torvalds fucked it up :-)
 */

/*
 * Fri Jul 13 2001 Crutcher Dunnavant <crutcher+kernel@datastacks.com>
 * - changed to provide snprintf and vsnprintf functions
 * So Feb  1 16:51:32 CET 2004 Juergen Quade <quade@hsnr.de>
 * - scnprintf and vscnprintf
 */

No 'any later version'

Now U-Boot COPYING has:

U-Boot is Free Software.  It is copyrighted by Wolfgang Denk and
many others who contributed code (see the actual source code for
details).  You can redistribute U-Boot and/or modify it under the
terms of version 2 of the GNU General Public License as published by
the Free Software Foundation.  Most of it can also be distributed,
                               -----------------------------------
at your option, under any later version of the GNU General Public
-----------------------------------------------------------------
License -- see individual files for exceptions.
-----------------------------------------------

So U-Boot is also GPLv2, but parts (like Linux) are GPLv2+

My point was there is a long term 'vision' for U-Boot to go GPLv3, and
part of that vision is to reject any future GPLv2 only licensed code
submitted. If we want the vision to be realised, I think we need to
look at how we build new GPLv2+ code in new files rather than tying new
code into GPLv2 only files which may, potentially, lock that code down.
Although as the writer and copyright holder of a modification you may be
free to move it over at a later date anyway - I don't know, and I think
that's in the 'ask a lawyer' territory.

So yes, you are right - In terms of license compliance, we are OK as
U-Boot is currently GPLv2 - In terms of U-Boot going to GPLv3, we are
digging a bigger hole :)

Regards,

Graeme

[U-Boot] [PATCH 2/4] Add safe vsnprintf and snprintf library functions

[Sonny Rao]

On Wed, Sep 28, 2011 at 5:00 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
> Hi Sonny,
>
> On Thu, Sep 29, 2011 at 9:26 AM, Sonny Rao <sonnyrao@chromium.org> wrote:
>> On Fri, Sep 23, 2011 at 4:56 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On 24/09/11 03:38, Simon Glass 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.
>>>
>>> Have you checked for license compatibility? U-Boot is GPLv2+ and (most) of
>>> Linux is GPLv2 - You may not be legally permitted to do this
>>
>> According to the FSF site, ?GPLv2 is compatible with GPLv3, see:
>> http://www.gnu.org/licenses/quick-guide-gplv3.html
>>
>> So it's fine to distribute them together.
>
> Not so fast:
>
> "GPLv2 is compatible with GPLv3 if the program allows you to choose "any
> later version" of the GPL, which is the case for most software released
> under this license"
>
> 'most', not 'all'
>
> The key phrase is "any later version" - If this is not in the GPLv2
> license text for the code your copying, you cannot license the derivative
> work under GPLv3 (or GPLv2+)

I'm a bit confused here... I was talking about distribution, not re-licensing.
I'm certainly not saying U-boot should be trying to re-license GPLv2 only code.
But the fact is that U-boot is already contains GPLv2 and GPLv2+ code
and it is being distributed together.

>> In reality though, this code in U-boot was already copied from the
>> same file in an older version of the kernel. ?The license (GPLv2 only)
>> hasn't changed on that file, so U-boot is already distributing what is
>> GPLv2 only code alongside GPLv2+ code -- which as I mentioned above is
>> fine.
>
> As long as you are not incorporating GPLv2 code into a file licensed under
> GPLv2+ and distributing the result as GPLv2+ or GPLv3.
>

My argument is that this code in U-boot was already GPLv2 only since
it came from a GPLv2 only file in the kernel.  I don't think that
we're applying GPLv2 code into a file licensed under GPLv2+.  Perhaps
we could add an explicit comment stating that the code in
lib/vsprintf.c is GPLv2 only so nobody gets the wrong idea.

>> The code here is derived from a later version of that same file, so I
>> don't believe integrating this patch into U-boot actually changes
>> anything with respect to licensing of this code.
>
> I think U-Boot will have great difficulty going past GPLv2 due to the
> large volume of Linus code already in U-Boot - We would need to either get
> the original code relicensed GPLv2+ or rewrite it
>
> And from the COPYING file in the Linux source:
>
> Also note that the only valid version of the GPL as far as the kernel
> is concerned is _this_ particular version of the license (ie v2, not
> v2.2 or v3.x or whatever), unless explicitly otherwise stated.
>
> and linux/lib/vsprintf.c:
>
> /*
> ?* ?linux/lib/vsprintf.c
> ?*
> ?* ?Copyright (C) 1991, 1992 ?Linus Torvalds
> ?*/
>
> /* vsprintf.c -- Lars Wirzenius & Linus Torvalds. */
> /*
> ?* Wirzenius wrote this portably, Torvalds fucked it up :-)
> ?*/
>
> /*
> ?* Fri Jul 13 2001 Crutcher Dunnavant <crutcher+kernel@datastacks.com>
> ?* - changed to provide snprintf and vsnprintf functions
> ?* So Feb ?1 16:51:32 CET 2004 Juergen Quade <quade@hsnr.de>
> ?* - scnprintf and vscnprintf
> ?*/
>
> No 'any later version'

Yes, as I said before, this code is definitely GPLv2 only.  I don't
think we're disagreeing here.

> Now U-Boot COPYING has:
>
> U-Boot is Free Software. ?It is copyrighted by Wolfgang Denk and
> many others who contributed code (see the actual source code for
> details). ?You can redistribute U-Boot and/or modify it under the
> terms of version 2 of the GNU General Public License as published by
> the Free Software Foundation. ?Most of it can also be distributed,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? -----------------------------------
> at your option, under any later version of the GNU General Public
> -----------------------------------------------------------------
> License -- see individual files for exceptions.
> -----------------------------------------------
>
> So U-Boot is also GPLv2, but parts (like Linux) are GPLv2+
>
> My point was there is a long term 'vision' for U-Boot to go GPLv3, and
> part of that vision is to reject any future GPLv2 only licensed code
> submitted. If we want the vision to be realised, I think we need to
> look at how we build new GPLv2+ code in new files rather than tying new
> code into GPLv2 only files which may, potentially, lock that code down.
> Although as the writer and copyright holder of a modification you may be
> free to move it over at a later date anyway - I don't know, and I think
> that's in the 'ask a lawyer' territory.

> So yes, you are right - In terms of license compliance, we are OK as
> U-Boot is currently GPLv2 - In terms of U-Boot going to GPLv3, we are
> digging a bigger hole :)

Ok, then that's up to the U-boot community to decide if they wish to
remove all GPLv2 only code, but I think that's a separate issue from
what you originally brought up.  I think we're in agreement that there
aren't any licensing considerations with respect to maintaining the
existing license which is GPLv2 only for lib/vsprintf.c.

Sonny

[U-Boot] [PATCH 2/4] Add safe vsnprintf and snprintf library functions

[Graeme Russ]

Hi Sonny,

On Thu, Sep 29, 2011 at 10:38 AM, Sonny Rao <sonnyrao@chromium.org> wrote:
> On Wed, Sep 28, 2011 at 5:00 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
>> Hi Sonny,
>>

[snip]

>> So yes, you are right - In terms of license compliance, we are OK as
>> U-Boot is currently GPLv2 - In terms of U-Boot going to GPLv3, we are
>> digging a bigger hole :)
>
> Ok, then that's up to the U-boot community to decide if they wish to
> remove all GPLv2 only code, but I think that's a separate issue from
> what you originally brought up.  I think we're in agreement that there
> aren't any licensing considerations with respect to maintaining the
> existing license which is GPLv2 only for lib/vsprintf.c.

Agree 100%

Regards,

Graeme

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

[Simon Glass]

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(-)

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)
-- 
1.7.3.1

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

[Kumar Gala]

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

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

[Simon Glass]

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
>

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

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110923/a97d9d44/attachment.pgp 

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

[Simon Glass]

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

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

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110923/342d5445/attachment.pgp 

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

[Simon Glass]

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

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

[Wolfgang Denk]

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

-- 
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
Es ist offensichtlich, dass das menschliche Gehirn wie  ein  Computer
funktioniert.  Da  es  keine  dummen Computer gibt, gibt es also auch
keine dummen Menschen. Nur ein paar Leute, die unter DOS laufen.
                                                       -- <unbekannt>

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

[Wolfgang Denk]

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

-- 
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
"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

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

[Simon Glass]

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 at 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
>

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

[Wolfgang Denk]

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

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

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

[Simon Glass]

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

[U-Boot] [PATCH 4/4] Use snprintf() in network code

[Simon Glass]

This tidies up network code to use snprintf() in most cases instead of
sprintf(). A few functions remain as they require header file changes.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 net/eth.c  |   10 +++++++---
 net/net.c  |   15 ++++++++++-----
 net/nfs.c  |    3 ++-
 net/tftp.c |    3 ++-
 4 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/net/eth.c b/net/eth.c
index 5911b1c..30711d4 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -49,7 +49,7 @@ int eth_setenv_enetaddr(char *name, const uchar *enetaddr)
 {
 	char buf[20];
 
-	sprintf(buf, "%pM", enetaddr);
+	snprintf(buf, sizeof(buf), "%pM", enetaddr);
 
 	return setenv(name, buf);
 }
@@ -58,7 +58,9 @@ int eth_getenv_enetaddr_by_index(const char *base_name, int index,
 				 uchar *enetaddr)
 {
 	char enetvar[32];
-	sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
+
+	snprintf(enetvar, sizeof(enetvar), index ? "%s%daddr" : "%saddr",
+		 base_name, index);
 	return eth_getenv_enetaddr(enetvar, enetaddr);
 }
 
@@ -68,7 +70,9 @@ static int eth_mac_skip(int index)
 {
 	char enetvar[15];
 	char *skip_state;
-	sprintf(enetvar, index ? "eth%dmacskip" : "ethmacskip", index);
+
+	snprintf(enetvar, sizeof(enetvar),
+		index ? "eth%dmacskip" : "ethmacskip", index);
 	return ((skip_state = getenv(enetvar)) != NULL);
 }
 
diff --git a/net/net.c b/net/net.c
index 7a60583..28e45c0 100644
--- a/net/net.c
+++ b/net/net.c
@@ -573,10 +573,12 @@ restart:
 				printf("Bytes transferred = %ld (%lx hex)\n",
 					NetBootFileXferSize,
 					NetBootFileXferSize);
-				sprintf(buf, "%lX", NetBootFileXferSize);
+				snprintf(buf, sizeof(buf), "%lX",
+					NetBootFileXferSize);
 				setenv("filesize", buf);
 
-				sprintf(buf, "%lX", (unsigned long)load_addr);
+				snprintf(buf, sizeof(buf), "%lX",
+					 (unsigned long)load_addr);
 				setenv("fileaddr", buf);
 			}
 			eth_halt();
@@ -950,7 +952,8 @@ int CDPSendTrigger(void)
 #ifdef CONFIG_CDP_DEVICE_ID
 	*s++ = htons(CDP_DEVICE_ID_TLV);
 	*s++ = htons(CONFIG_CDP_DEVICE_ID);
-	sprintf(buf, CONFIG_CDP_DEVICE_ID_PREFIX "%pm", NetOurEther);
+	snprintf(buf, sizeof(buf), CONFIG_CDP_DEVICE_ID_PREFIX "%pm",
+		 NetOurEther);
 	memcpy((uchar *)s, buf, 16);
 	s += 16 / 2;
 #endif
@@ -958,7 +961,7 @@ int CDPSendTrigger(void)
 #ifdef CONFIG_CDP_PORT_ID
 	*s++ = htons(CDP_PORT_ID_TLV);
 	memset(buf, 0, sizeof(buf));
-	sprintf(buf, CONFIG_CDP_PORT_ID, eth_get_dev_index());
+	snprintf(buf, sizeof(buf), CONFIG_CDP_PORT_ID, eth_get_dev_index());
 	len = strlen(buf);
 	if (len & 1)	/* make it even */
 		len++;
@@ -1516,7 +1519,9 @@ NetReceive(volatile uchar *inpkt, int len)
 #ifdef CONFIG_KEEP_SERVERADDR
 			if (NetServerIP == NetArpWaitPacketIP) {
 				char buf[20];
-				sprintf(buf, "%pM", arp->ar_data);
+
+				snprintf(buf, sizeof(buf), "%pM",
+					 arp->ar_data);
 				setenv("serveraddr", buf);
 			}
 #endif
diff --git a/net/nfs.c b/net/nfs.c
index f76f60d..d1e894e 100644
--- a/net/nfs.c
+++ b/net/nfs.c
@@ -688,7 +688,8 @@ NfsStart (void)
 	}
 
 	if (BootFile[0] == '\0') {
-		sprintf (default_filename, "/nfsroot/%02lX%02lX%02lX%02lX.img",
+		snprintf(default_filename, sizeof(default_filename),
+			"/nfsroot/%02lX%02lX%02lX%02lX.img",
 			NetOurIP & 0xFF,
 			(NetOurIP >>  8) & 0xFF,
 			(NetOurIP >> 16) & 0xFF,
diff --git a/net/tftp.c b/net/tftp.c
index a893e02..c4f6a59 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -591,7 +591,8 @@ TftpStart(void)
 
 	TftpRemoteIP = NetServerIP;
 	if (BootFile[0] == '\0') {
-		sprintf(default_filename, "%02lX%02lX%02lX%02lX.img",
+		snprintf(default_filename, sizeof(default_filename),
+			"%02lX%02lX%02lX%02lX.img",
 			NetOurIP & 0xFF,
 			(NetOurIP >>  8) & 0xFF,
 			(NetOurIP >> 16) & 0xFF,
-- 
1.7.3.1

[U-Boot] [PATCH 4/4] Use snprintf() in network code

[Mike Frysinger]

On Friday, September 23, 2011 13:38:52 Simon Glass wrote:
> This tidies up network code to use snprintf() in most cases instead of
> sprintf(). A few functions remain as they require header file changes.

NAK to most of these.  we pick local sized buffers that are known to not 
overflow, or require circumstances that aren't really feasible.

3 examples (which are the first 3 changes in this patch) below ...

> --- a/net/eth.c
> +++ b/net/eth.c
> 
>  	char buf[20];
> -	sprintf(buf, "%pM", enetaddr);
> +	snprintf(buf, sizeof(buf), "%pM", enetaddr);

a mac address will not take more than 19 bytes.  unless the sprintf code is 
completely busted, but if that's the case, we should fix that instead since 
it'd be pretty fundamentally screwed.

>  	char enetvar[32];
> -	sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
> +	snprintf(enetvar, sizeof(enetvar), index ? "%s%daddr" : "%saddr",
> +		 base_name, index);

in order for this to overflow, we have to have 1000+ eth devices (maybe more?  
i'd have to read the code closer)

>  	char enetvar[15];
> -	sprintf(enetvar, index ? "eth%dmacskip" : "ethmacskip", index);
> +	snprintf(enetvar, sizeof(enetvar),
> +		index ? "eth%dmacskip" : "ethmacskip", index);

in order for this to overflow, we have to have 10000+ eth devices

please look at the realistic needs rather than blanket converting to snprintf
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110923/0dca814e/attachment.pgp 

[U-Boot] [PATCH 4/4] Use snprintf() in network code

[Simon Glass]

Hi Mike,

On Fri, Sep 23, 2011 at 11:15 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Friday, September 23, 2011 13:38:52 Simon Glass wrote:
>> This tidies up network code to use snprintf() in most cases instead of
>> sprintf(). A few functions remain as they require header file changes.
>
> NAK to most of these. ?we pick local sized buffers that are known to not
> overflow, or require circumstances that aren't really feasible.

Yes that's why I sent this patch on the end, to see how far this
should be taken.

>
> 3 examples (which are the first 3 changes in this patch) below ...
>
>> --- a/net/eth.c
>> +++ b/net/eth.c
>>
>> ? ? ? char buf[20];
>> - ? ? sprintf(buf, "%pM", enetaddr);
>> + ? ? snprintf(buf, sizeof(buf), "%pM", enetaddr);
>
> a mac address will not take more than 19 bytes. ?unless the sprintf code is
> completely busted, but if that's the case, we should fix that instead since
> it'd be pretty fundamentally screwed.

OK (18 bytes?)

>
>> ? ? ? char enetvar[32];
>> - ? ? sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
>> + ? ? snprintf(enetvar, sizeof(enetvar), index ? "%s%daddr" : "%saddr",
>> + ? ? ? ? ? ? ?base_name, index);
>
> in order for this to overflow, we have to have 1000+ eth devices (maybe more?
> i'd have to read the code closer)

Or base_name could be longer.

>
>> ? ? ? char enetvar[15];
>> - ? ? sprintf(enetvar, index ? "eth%dmacskip" : "ethmacskip", index);
>> + ? ? snprintf(enetvar, sizeof(enetvar),
>> + ? ? ? ? ? ? index ? "eth%dmacskip" : "ethmacskip", index);
>
> in order for this to overflow, we have to have 10000+ eth devices
>
> please look at the realistic needs rather than blanket converting to snprintf

OK, this is fair enough. There are a couple of functions like
ip_to_string() which don't get passed a length. I would prefer to see
this done, so we can use snprintf() or assert() since the code then
becomes more robust. But there are no bugs there at present. Thoughts?

Regards,
Simon

> -mike
>

[U-Boot] [PATCH 4/4] Use snprintf() in network code

[Mike Frysinger]

On Friday, September 23, 2011 14:30:09 Simon Glass wrote:
> On Fri, Sep 23, 2011 at 11:15 AM, Mike Frysinger wrote:
> > On Friday, September 23, 2011 13:38:52 Simon Glass wrote:
> >> --- a/net/eth.c
> >> +++ b/net/eth.c
> >> 
> >>       char buf[20];
> >> -     sprintf(buf, "%pM", enetaddr);
> >> +     snprintf(buf, sizeof(buf), "%pM", enetaddr);
> > 
> > a mac address will not take more than 19 bytes.  unless the sprintf code
> > is completely busted, but if that's the case, we should fix that instead
> > since it'd be pretty fundamentally screwed.
> 
> OK (18 bytes?)

right ... i meant there's 19 available (since 20 makes NUL), and there's no 
way it should hit that 19 limit.

> >>       char enetvar[32];
> >> -     sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
> >> +     snprintf(enetvar, sizeof(enetvar), index ? "%s%daddr" : "%saddr",
> >> +              base_name, index);
> > 
> > in order for this to overflow, we have to have 1000+ eth devices (maybe
> > more? i'd have to read the code closer)
> 
> Or base_name could be longer.

true, but base_name atm is supposed to be "eth" or "usbeth".  i would hope 
we'd be diligent about device naming conventions rather than letting someone 
slip in "mysuperawesomeboardeth" (although that still wouldn't overflow ;]).

> >>       char enetvar[15];
> >> -     sprintf(enetvar, index ? "eth%dmacskip" : "ethmacskip", index);
> >> +     snprintf(enetvar, sizeof(enetvar),
> >> +             index ? "eth%dmacskip" : "ethmacskip", index);
> > 
> > in order for this to overflow, we have to have 10000+ eth devices
> > 
> > please look at the realistic needs rather than blanket converting to
> > snprintf
> 
> OK, this is fair enough. There are a couple of functions like
> ip_to_string() which don't get passed a length. I would prefer to see
> this done, so we can use snprintf() or assert() since the code then
> becomes more robust. But there are no bugs there at present. Thoughts?

for funcs that do generally handle arbitrary data, i don't mind including len 
specs, but when we're using known standards, i'd prefer to stick to the 
smaller/simpler code.  this is a thin boot loader that is meant to be fast, 
and there are plenty of ways to bring down the system otherwise (and i don't 
think these cases are generally a robustness concern).

don't know if wolfgang has an opinion.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110923/7c5e56f6/attachment.pgp 

[U-Boot] [PATCH 4/4] Use snprintf() in network code

[Simon Glass]

Hi Mike,

On Fri, Sep 23, 2011 at 1:09 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Friday, September 23, 2011 14:30:09 Simon Glass wrote:
>> On Fri, Sep 23, 2011 at 11:15 AM, Mike Frysinger wrote:
>> > On Friday, September 23, 2011 13:38:52 Simon Glass wrote:
>> >> --- a/net/eth.c
>> >> +++ b/net/eth.c
>> >>
>> >> ? ? ? char buf[20];
>> >> - ? ? sprintf(buf, "%pM", enetaddr);
>> >> + ? ? snprintf(buf, sizeof(buf), "%pM", enetaddr);
>> >
>> > a mac address will not take more than 19 bytes. ?unless the sprintf code
>> > is completely busted, but if that's the case, we should fix that instead
>> > since it'd be pretty fundamentally screwed.
>>
>> OK (18 bytes?)
>
> right ... i meant there's 19 available (since 20 makes NUL), and there's no
> way it should hit that 19 limit.
>
>> >> ? ? ? char enetvar[32];
>> >> - ? ? sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
>> >> + ? ? snprintf(enetvar, sizeof(enetvar), index ? "%s%daddr" : "%saddr",
>> >> + ? ? ? ? ? ? ?base_name, index);
>> >
>> > in order for this to overflow, we have to have 1000+ eth devices (maybe
>> > more? i'd have to read the code closer)
>>
>> Or base_name could be longer.
>
> true, but base_name atm is supposed to be "eth" or "usbeth". ?i would hope
> we'd be diligent about device naming conventions rather than letting someone
> slip in "mysuperawesomeboardeth" (although that still wouldn't overflow ;]).

Well here I feel that it is pushing it a bit to have a function that
builds a string based on arguments over which it has no control. I
suppose an assert(strlen(base_name) < 23) or whatever would do it. But
isn't snprintf() better?

>
>> >> ? ? ? char enetvar[15];
>> >> - ? ? sprintf(enetvar, index ? "eth%dmacskip" : "ethmacskip", index);
>> >> + ? ? snprintf(enetvar, sizeof(enetvar),
>> >> + ? ? ? ? ? ? index ? "eth%dmacskip" : "ethmacskip", index);
>> >
>> > in order for this to overflow, we have to have 10000+ eth devices
>> >
>> > please look at the realistic needs rather than blanket converting to
>> > snprintf
>>
>> OK, this is fair enough. There are a couple of functions like
>> ip_to_string() which don't get passed a length. I would prefer to see
>> this done, so we can use snprintf() or assert() since the code then
>> becomes more robust. But there are no bugs there at present. Thoughts?
>
> for funcs that do generally handle arbitrary data, i don't mind including len
> specs, but when we're using known standards, i'd prefer to stick to the
> smaller/simpler code. ?this is a thin boot loader that is meant to be fast,
> and there are plenty of ways to bring down the system otherwise (and i don't
> think these cases are generally a robustness concern).

Well that argues towards using assert() in these cases rather than
just ignoring them. I'm keen to try to make U-Boot more testable. I do
understand it is just a 'thin' boot loader, but it has quite a few
features now, and it if breaks, the system won't boot.

I do agree there is a difference between telling devs about a bug /
overflow at build/test time and bloating the code in production.
Either/or is good with me, but I'm not entirely comfortable with just
ignoring these issues and hoping they don't bite us. It's not like we
have a test suite to check it.

>
> don't know if wolfgang has an opinion.

<insert here>

Regards,
Simon

> -mike
>

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

[Albert ARIBAUD]

Hi Simon,

Le 23/09/2011 19:38, Simon Glass a ?crit :
> 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.

Indeed overruns can lead to bad behaviors, but in any case, it can never 
be recovered, because at the root, the problem is that the caller 
provided inconsistent arguments to printf.

So in essence, you're 'fixing' printf for a design error in printf's 
caller, instead of fixing the design error.

Amicalement,
-- 
Albert.

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

[Simon Glass]

Hi Albert,

On Fri, Sep 23, 2011 at 1:40 PM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Hi Simon,
>
> Le 23/09/2011 19:38, Simon Glass a ?crit :
>>
>> 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.
>
> Indeed overruns can lead to bad behaviors, but in any case, it can never be
> recovered, because at the root, the problem is that the caller provided
> inconsistent arguments to printf.

Recovery is one thing, but I would settle for just not crashing, which
is the purpose of this patch. We could also easily WARN if that were
considered appropriate here.

>
> So in essence, you're 'fixing' printf for a design error in printf's caller,
> instead of fixing the design error.

Well, the nature of a function is that it cannot know what arguments
might be passed to it. It can only assert(), limit check, etc. A limit
check is what this patch aims to add.

Regards,
Simon

>
> Amicalement,
> --
> Albert.
>

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

[Albert ARIBAUD]

Le 23/09/2011 22:46, Simon Glass a ?crit :
> Hi Albert,
>
> On Fri, Sep 23, 2011 at 1:40 PM, Albert ARIBAUD
> <albert.u.boot@aribaud.net>  wrote:
>> Hi Simon,
>>
>> Le 23/09/2011 19:38, Simon Glass a ?crit :
>>>
>>> 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.
>>
>> Indeed overruns can lead to bad behaviors, but in any case, it can never be
>> recovered, because at the root, the problem is that the caller provided
>> inconsistent arguments to printf.
>
> Recovery is one thing, but I would settle for just not crashing, which
> is the purpose of this patch. We could also easily WARN if that were
> considered appropriate here.
>
>>
>> So in essence, you're 'fixing' printf for a design error in printf's caller,
>> instead of fixing the design error.
>
> Well, the nature of a function is that it cannot know what arguments
> might be passed to it. It can only assert(), limit check, etc. A limit
> check is what this patch aims to add.

I do agree that as a rule, a function should check its arguments, but as 
any rule, this one has understated assumptions. Here, one assumption is 
that calls to the functions cannot be made compliant, and therefore it 
falls to the function to ensure this compliance; and another one is that 
the function can actually check this conformance.

The first assumption is correct in an OS or general-purpose library 
where the number of callers is virtually unlimited and there is no way 
to make sure all calls are conforming.

In U-Boot however, the number of callers is bound and known (unless 
you're thinking of standalone U-Boot apps, but I don't see this as a use 
case so frequent that it warrants a 'fix')

The second assumption is false by nature for printf(), which simply 
cannot check its input buffer (OTOH *nprintf() does, and it is its job).

Besides, there *is* a sub-family of printf functions which are dedicated 
to the case where buffers provided may be too small for the print output 
-- meaning that the issue is known and an easy fix exists if cropping 
the output is allowable.

So basically the choice is between:

- adding code to the printf() family to try and fix an issue that it is 
fundamentally unable to properly fix, and for which a solution exists, or

- grepping and fixing calls to *sprintf() in U-Boot that do not respect 
the known contraints of printf(), by resizing the buffer or calling 
*snprintf() instead.

I am definitely not in favor of the first option concerning U-Boot.

> Regards,
> Simon

Amicalement,
-- 
Albert.

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

[Simon Glass]

Hi Albert,

On Sat, Sep 24, 2011 at 2:37 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Le 23/09/2011 22:46, Simon Glass a ?crit :
>>
>> Hi Albert,
>>
>> On Fri, Sep 23, 2011 at 1:40 PM, Albert ARIBAUD
>> <albert.u.boot@aribaud.net> ?wrote:
>>>
>>> Hi Simon,
>>>
>>> Le 23/09/2011 19:38, Simon Glass a ?crit :
>>>>
>>>> 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.
>>>
>>> Indeed overruns can lead to bad behaviors, but in any case, it can never
>>> be
>>> recovered, because at the root, the problem is that the caller provided
>>> inconsistent arguments to printf.
>>
>> Recovery is one thing, but I would settle for just not crashing, which
>> is the purpose of this patch. We could also easily WARN if that were
>> considered appropriate here.
>>
>>>
>>> So in essence, you're 'fixing' printf for a design error in printf's
>>> caller,
>>> instead of fixing the design error.
>>
>> Well, the nature of a function is that it cannot know what arguments
>> might be passed to it. It can only assert(), limit check, etc. A limit
>> check is what this patch aims to add.
>
> I do agree that as a rule, a function should check its arguments, but as any
> rule, this one has understated assumptions. Here, one assumption is that
> calls to the functions cannot be made compliant, and therefore it falls to
> the function to ensure this compliance; and another one is that the function
> can actually check this conformance.

There is also the issue of programmer error, or unexpected situations,
and graceful failure in these cases. We want to avoid hard-to-find
memory/stack corruption bugs if the cost of doing so is moderate.

>
> The first assumption is correct in an OS or general-purpose library where
> the number of callers is virtually unlimited and there is no way to make
> sure all calls are conforming.
>
> In U-Boot however, the number of callers is bound and known (unless you're
> thinking of standalone U-Boot apps, but I don't see this as a use case so
> frequent that it warrants a 'fix')

Yes, and one can inspect each call and decide if the nprint() variants
are preferable in each case.
>
> The second assumption is false by nature for printf(), which simply cannot
> check its input buffer (OTOH *nprintf() does, and it is its job).
>
> Besides, there *is* a sub-family of printf functions which are dedicated to
> the case where buffers provided may be too small for the print output --
> meaning that the issue is known and an easy fix exists if cropping the
> output is allowable.

Yes, or even if cropping is not allowed, we might expect a nice error,
rather than a crash sometime later when the stack corruption or data
space overwriting causes problems.
>
> So basically the choice is between:
>
> - adding code to the printf() family to try and fix an issue that it is
> fundamentally unable to properly fix, and for which a solution exists, or
>
> - grepping and fixing calls to *sprintf() in U-Boot that do not respect the
> known contraints of printf(), by resizing the buffer or calling *snprintf()
> instead.
>
> I am definitely not in favor of the first option concerning U-Boot.

Sounds fine to me. So I think we need the nprintf() variants in there,
but the message is not to use them willy nilly. Going back to my patch
series, 3/4 is ok, but 4/4 mostly crosses the line. Do I have that
right?

By the way, printf() ends up calling the same code, but without limit
checking in place. The alternative is to duplicate all the format
string processing code (a limit-checking version and an unchecked
version) which would be worse.

> Amicalement,
> --
> Albert.
>

Regards,
Simon

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

[Albert ARIBAUD]

Le 24/09/2011 16:00, Simon Glass a ?crit :

>> So basically the choice is between:
>>
>> - adding code to the printf() family to try and fix an issue that it is
>> fundamentally unable to properly fix, and for which a solution exists, or
>>
>> - grepping and fixing calls to *sprintf() in U-Boot that do not respect the
>> known contraints of printf(), by resizing the buffer or calling *snprintf()
>> instead.
>>
>> I am definitely not in favor of the first option concerning U-Boot.
>
> Sounds fine to me. So I think we need the nprintf() variants in there,
> but the message is not to use them willy nilly. Going back to my patch
> series, 3/4 is ok, but 4/4 mostly crosses the line. Do I have that
> right?

It is the exact opposite for me : 3/4 makes all printf functions work 
like some kind of *nprintf(), while 4/4 is about the network code 
switching to *nprintf() for safety, so 3/4 would be nak and 4/4 ack as 
far as I am concerned.

Basically, printf family functions which do not have the 'n' are *know* 
by all -- experienced enough :) -- programmers to be *unsafe* (but to 
require less from the caller) and it should remain so: no programmer 
should ever encounter an implementation of printf that pretends to be 
even somewhat safe, because it might bite him/her elsewhere, in another 
project based on another C library where printf is just the beartrap it 
usually is.

IOW, programmers already have assumptions about *printf(), including how 
to deal with length limitations and what happens if you don't, and it is 
best that these assumption remain true whatever project they work with.

> By the way, printf() ends up calling the same code, but without limit
> checking in place. The alternative is to duplicate all the format
> string processing code (a limit-checking version and an unchecked
> version) which would be worse.

I don't intend to dictate the way things can be implemented, so the 
degree of code reuse is an open question as far as I am concerned. I am 
only voicing my opinion that *printf() APIs and their contracts should 
remain identical across all implementations of *printf(), and thus that 
providing *nprintf() where they don't exist is commandable, but 
hardening printf() is not, since you basically cannot do it without 
somewhat departing from the de facto standard.

> Regards,
> Simon

Amicalement,
-- 
Albert.

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

[Simon Glass]

Hi Albert,

On Sun, Sep 25, 2011 at 1:40 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Le 24/09/2011 16:00, Simon Glass a ?crit :
>
>>> So basically the choice is between:
>>>
>>> - adding code to the printf() family to try and fix an issue that it is
>>> fundamentally unable to properly fix, and for which a solution exists, or
>>>
>>> - grepping and fixing calls to *sprintf() in U-Boot that do not respect
>>> the
>>> known contraints of printf(), by resizing the buffer or calling
>>> *snprintf()
>>> instead.
>>>
>>> I am definitely not in favor of the first option concerning U-Boot.
>>
>> Sounds fine to me. So I think we need the nprintf() variants in there,
>> but the message is not to use them willy nilly. Going back to my patch
>> series, 3/4 is ok, but 4/4 mostly crosses the line. Do I have that
>> right?
>
> It is the exact opposite for me : 3/4 makes all printf functions work like
> some kind of *nprintf(), while 4/4 is about the network code switching to
> *nprintf() for safety, so 3/4 would be nak and 4/4 ack as far as I am
> concerned.
>
> Basically, printf family functions which do not have the 'n' are *know* by
> all -- experienced enough :) -- programmers to be *unsafe* (but to require
> less from the caller) and it should remain so: no programmer should ever
> encounter an implementation of printf that pretends to be even somewhat
> safe, because it might bite him/her elsewhere, in another project based on
> another C library where printf is just the beartrap it usually is.
>
> IOW, programmers already have assumptions about *printf(), including how to
> deal with length limitations and what happens if you don't, and it is best
> that these assumption remain true whatever project they work with.

I don't quite understand this. You are saying that we shouldn't modify
the existing printf(), serial_printf() etc. so live within the buffer
they use? If I call printf() and my resulting string is too long for
the library then I would much prefer to get truncated output than have
to hunt for a wierd crash.

It sounds like you are asking for a new printf(), serial_printf()
which provides the facility of limiting the output to n characters,
and leave these ones alone. But these functions are not passed a
buffer - they use their own and they set the site of it. So I think
they should be responsible for enforcing that size.

If you are asking for a new set of functions - nprintf(),
serial_nprintf(), etc. then I wonder how you can pass the 'n' but not
the actual buffer. In my mind you should not do one without the other.

So please can you clarify?

>
>> By the way, printf() ends up calling the same code, but without limit
>> checking in place. The alternative is to duplicate all the format
>> string processing code (a limit-checking version and an unchecked
>> version) which would be worse.
>
> I don't intend to dictate the way things can be implemented, so the degree
> of code reuse is an open question as far as I am concerned. I am only
> voicing my opinion that *printf() APIs and their contracts should remain
> identical across all implementations of *printf(), and thus that providing
> *nprintf() where they don't exist is commandable, but hardening printf() is
> not, since you basically cannot do it without somewhat departing from the de
> facto standard.

OK fair enough. People are used to printf() just working, no matter
what the size. I haven't looked at the implementation but I suspect
when the buffer fills it outputs it and starts the buffer again .In
any case you don't have to worry about it with the GNU C library.

We probably don't need to go that far in U-Boot, but some simple
checking would avoid a nasty surprise for the user. It is obvious from
the result that something is truncated, and we can WARN if that helps.

Regards,
Simon

>
>> Regards,
>> Simon
>
> Amicalement,
> --
> Albert.
>

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

[Albert ARIBAUD]

Hi Simon,

Le 25/09/2011 16:50, Simon Glass a ?crit :

>> Basically, printf family functions which do not have the 'n' are *know* by
>> all -- experienced enough :) -- programmers to be *unsafe* (but to require
>> less from the caller) and it should remain so: no programmer should ever
>> encounter an implementation of printf that pretends to be even somewhat
>> safe, because it might bite him/her elsewhere, in another project based on
>> another C library where printf is just the beartrap it usually is.
>>
>> IOW, programmers already have assumptions about *printf(), including how to
>> deal with length limitations and what happens if you don't, and it is best
>> that these assumption remain true whatever project they work with.
>
> I don't quite understand this. You are saying that we shouldn't modify
> the existing printf(), serial_printf() etc. so live within the buffer
> they use? If I call printf() and my resulting string is too long for
> the library then I would much prefer to get truncated output than have
> to hunt for a wierd crash.

I understand the preference for truncated output, but the output length 
is primarily in the hands of the caller, through the format stringand 
possibly also through *nprintf() -- i.e. the caller can only get an 
output overrun if (s)he does not take the required steps to avoid it.

> It sounds like you are asking for a new printf(), serial_printf()
> which provides the facility of limiting the output to n characters,
> and leave these ones alone. But these functions are not passed a
> buffer - they use their own and they set the site of it. So I think
> they should be responsible for enforcing that size.

The existing contract for printf family functions (think wider than 
U-Boot) does not enforce this responsibility on them.

> If you are asking for a new set of functions - nprintf(),
> serial_nprintf(), etc. then I wonder how you can pass the 'n' but not
> the actual buffer. In my mind you should not do one without the other.
>
> So please can you clarify?

I should clarify indeed. My opinion, expressed as a single general rule, 
is "keep the known semantics of *printf() function family unchanged in 
U-Boot wrt all other printif implementations around". Thus I think that:

- the printf() function should *not* attempt anything to mitigate length 
issues beyond what the standard mandates. If a user calls printf(), 
(s)he is expected to be aware of the risks of overruns,  and to take -- 
if (s)he so decides -- steps to avoid it; besides, the function does not 
have a way. Conversively, implementers of the printf() function need not 
consider any specific recovery action with regard to size issues. For 
instance, why do we have an internal buffer for printf to begin with? 
The implementation does not need them (and besides, I guess the buffer 
does not respect the single C99 environmental constraint for printf, 
that any field should be able to be at least 4095 bytes -- no kidding).

- users who actually wisht to limit outpout ca use either

-- a crafted format string with maximum-sized format specifiers, or

-- a call tp a *nprintf() function into a local buffer of known size 
followed by a call to fputs().

>>> By the way, printf() ends up calling the same code, but without limit
>>> checking in place. The alternative is to duplicate all the format
>>> string processing code (a limit-checking version and an unchecked
>>> version) which would be worse.
>>
>> I don't intend to dictate the way things can be implemented, so the degree
>> of code reuse is an open question as far as I am concerned. I am only
>> voicing my opinion that *printf() APIs and their contracts should remain
>> identical across all implementations of *printf(), and thus that providing
>> *nprintf() where they don't exist is commandable, but hardening printf() is
>> not, since you basically cannot do it without somewhat departing from the de
>> facto standard.
>
> OK fair enough. People are used to printf() just working, no matter
> what the size. I haven't looked at the implementation but I suspect
> when the buffer fills it outputs it and starts the buffer again .In
> any case you don't have to worry about it with the GNU C library.

You may also find implementations where the buffer is flushed after each 
literal part in the format string and after each format specifier 
output. Or some which emit serial characters as soon as they are 
produced and use a buffer only for those formats where the ASCII 
representation cannot be easily constructed left-to-right.

> We probably don't need to go that far in U-Boot, but some simple
> checking would avoid a nasty surprise for the user. It is obvious from
> the result that something is truncated, and we can WARN if that helps.

A user who does not expect a nasty surprise from calling printf() with a 
fair chance of overflowing the output buffer deserves the surprise. :)

> Regards,
> Simon

Amicalement,
-- 
Albert.

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

[Simon Glass]

Hi Albert,

On Mon, Sep 26, 2011 at 4:20 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Hi Simon,
>
> Le 25/09/2011 16:50, Simon Glass a ?crit :
>
>>> Basically, printf family functions which do not have the 'n' are *know*
>>> by
>>> all -- experienced enough :) -- programmers to be *unsafe* (but to
>>> require
>>> less from the caller) and it should remain so: no programmer should ever
>>> encounter an implementation of printf that pretends to be even somewhat
>>> safe, because it might bite him/her elsewhere, in another project based
>>> on
>>> another C library where printf is just the beartrap it usually is.
>>>
>>> IOW, programmers already have assumptions about *printf(), including how
>>> to
>>> deal with length limitations and what happens if you don't, and it is
>>> best
>>> that these assumption remain true whatever project they work with.
>>
>> I don't quite understand this. You are saying that we shouldn't modify
>> the existing printf(), serial_printf() etc. so live within the buffer
>> they use? If I call printf() and my resulting string is too long for
>> the library then I would much prefer to get truncated output than have
>> to hunt for a wierd crash.
>
> I understand the preference for truncated output, but the output length is
> primarily in the hands of the caller, through the format stringand possibly
> also through *nprintf() -- i.e. the caller can only get an output overrun if
> (s)he does not take the required steps to avoid it.

So we are talking about two things here. I am fine with this one -
basically people can call sprintf() or snprintf(). If the former then
they should be aware of the potential for overflow, if the latter they
should prepare for possible truncation.

>
>> It sounds like you are asking for a new printf(), serial_printf()
>> which provides the facility of limiting the output to n characters,
>> and leave these ones alone. But these functions are not passed a
>> buffer - they use their own and they set the site of it. So I think
>> they should be responsible for enforcing that size.
>
> The existing contract for printf family functions (think wider than U-Boot)
> does not enforce this responsibility on them.

Let me re-word: printf() and serial_printf() should be responsible for
working within the buffer that they use. In the 'existing printf()
contract' it is no concern of the user what the size of that buffer
may be. In U-Boot we have an implementation that works wholly within
that buffer, and that buffer is small, so we are exposing our
limitation, either by a buffer overflow crash or truncation. I
advocate the latter.

>
>> If you are asking for a new set of functions - nprintf(),
>> serial_nprintf(), etc. then I wonder how you can pass the 'n' but not
>> the actual buffer. In my mind you should not do one without the other.
>>
>> So please can you clarify?
>
> I should clarify indeed. My opinion, expressed as a single general rule, is
> "keep the known semantics of *printf() function family unchanged in U-Boot
> wrt all other printif implementations around". Thus I think that:
>
> - the printf() function should *not* attempt anything to mitigate length
> issues beyond what the standard mandates. If a user calls printf(), (s)he is
> expected to be aware of the risks of overruns, ?and to take -- if (s)he so
> decides -- steps to avoid it; besides, the function does not have a way.
> Conversively, implementers of the printf() function need not consider any
> specific recovery action with regard to size issues. For instance, why do we
> have an internal buffer for printf to begin with? The implementation does
> not need them (and besides, I guess the buffer does not respect the single
> C99 environmental constraint for printf, that any field should be able to be
> at least 4095 bytes -- no kidding).
>
> - users who actually wisht to limit outpout ca use either
>
> -- a crafted format string with maximum-sized format specifiers, or
>
> -- a call tp a *nprintf() function into a local buffer of known size
> followed by a call to fputs().

I think this last one is clumsy. It could lead to buffers all over
U-Boot to work around the printf() limitation. If there is a desire
for this (and I'm not saying there is), let's enhance printf() to
output bit by bit as it goes (as a separate discussion from this one
though). One justification for leaving printf() as it is (but with a
size limit instead of a buffer overflow) is that people seldom printf
more than a few lines at once.

>
>>>> By the way, printf() ends up calling the same code, but without limit
>>>> checking in place. The alternative is to duplicate all the format
>>>> string processing code (a limit-checking version and an unchecked
>>>> version) which would be worse.
>>>
>>> I don't intend to dictate the way things can be implemented, so the
>>> degree
>>> of code reuse is an open question as far as I am concerned. I am only
>>> voicing my opinion that *printf() APIs and their contracts should remain
>>> identical across all implementations of *printf(), and thus that
>>> providing
>>> *nprintf() where they don't exist is commandable, but hardening printf()
>>> is
>>> not, since you basically cannot do it without somewhat departing from the
>>> de
>>> facto standard.
>>
>> OK fair enough. People are used to printf() just working, no matter
>> what the size. I haven't looked at the implementation but I suspect
>> when the buffer fills it outputs it and starts the buffer again .In
>> any case you don't have to worry about it with the GNU C library.
>
> You may also find implementations where the buffer is flushed after each
> literal part in the format string and after each format specifier output. Or
> some which emit serial characters as soon as they are produced and use a
> buffer only for those formats where the ASCII representation cannot be
> easily constructed left-to-right.
>
>> We probably don't need to go that far in U-Boot, but some simple
>> checking would avoid a nasty surprise for the user. It is obvious from
>> the result that something is truncated, and we can WARN if that helps.
>
> A user who does not expect a nasty surprise from calling printf() with a
> fair chance of overflowing the output buffer deserves the surprise. :)

For sprintf() I agree - this is well understood and people are aware
of it. For printf() I am not so sure.

Regards,
Simon

>
>> Regards,
>> Simon
>
> Amicalement,
> --
> Albert.
>

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

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ0y_TWJ4ANEXekGxNpeoJHhkth7hod_3Quu2HVcS=1wHA@mail.gmail.com> you wrote:
> 
> For sprintf() I agree - this is well understood and people are aware
> of it. For printf() I am not so sure.

We are a resource limited boot loader.  We got for a small footprint,
and accept some resulting restrictions, if they are not really severe.
THe printf() restriction has not exactly frequently popped up before,
so I consider it a non-issue.

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
Once upon a time, "PC" meant "personal computer".  Now  it  seems  to
only  mean  "Prisoner  of Bill". Alas! All my PCs run Unix, and those
include Intel, Sparc, and other processors.
          -- Tom "Bring back the non-PC meaning of `PC'" Christiansen

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

[Scott Wood]

On 09/26/2011 06:20 AM, Albert ARIBAUD wrote:
> Hi Simon,
> 
> Le 25/09/2011 16:50, Simon Glass a ?crit :
> 
>>> Basically, printf family functions which do not have the 'n' are *know* by
>>> all -- experienced enough :) -- programmers to be *unsafe* (but to require
>>> less from the caller)

printf()/fprintf() are usually safe enough for the things they're used for.

> and it should remain so: no programmer should ever
>>> encounter an implementation of printf that pretends to be even somewhat
>>> safe, because it might bite him/her elsewhere, in another project based on
>>> another C library where printf is just the beartrap it usually is.

And we should mount large spikes on every steering wheel to make people
drive more carefully. :-P

> I should clarify indeed. My opinion, expressed as a single general rule, 
> is "keep the known semantics of *printf() function family unchanged in 
> U-Boot wrt all other printif implementations around". Thus I think that:

FWIW, Linux's printk() truncates output if it exceeds the internal
buffer.  glibc's is an ugly mess, but appears to not need a fixed printf
buffer -- it streams output into its file buffering mechanism.  That
there may be some crappy implementations out there is no excuse for
making these functions difficult to use everywhere.

> Conversively, implementers of the printf() function need not 
> consider any specific recovery action with regard to size issues. For 
> instance, why do we have an internal buffer for printf to begin with? 
> The implementation does not need them 

Apparently this implementation does.  I suppose you could rewrite the
code to not need one, but is that really worthwhile here, compared to
this easy and virtually cost-free (once you have snprintf) form of
damage control?  It's just passing in a meaningful number to vsnprintf
instead of INT_MAX.  You could save some bytes by removing vsprintf(),
as well.

> (and besides, I guess the buffer 
> does not respect the single C99 environmental constraint for printf, 
> that any field should be able to be at least 4095 bytes -- no kidding).

That this minimum is more than most boards allow for the entire output,
and that it is board-specific (so it could work for one board but fail
for another), are all the more reason to want some safety here.  I'd
much rather see a truncated message (and proceed to fix the obvious bug)
than have to debug corruption and possibly recover a bricked board.

> - users who actually wisht to limit outpout ca use either

You say "actually wish to limit output" as if "let it corrupt memory if
it's too large" is the normal thing to want.

-scott

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

[Albert ARIBAUD]

On 27/09/2011 00:28, Scott Wood wrote:

>> - users who actually wisht to limit outpout ca use either
>
> You say "actually wish to limit output" as if "let it corrupt memory if
> it's too large" is the normal thing to want.

What I meant was "users who actually want to limit output explicitly by 
truncating it rather than implicitly by always outputting small enough 
text or crafting limitations in the format string specifiers, ..."

> -scott

Amicalement,
-- 
Albert.

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

[Simon Glass]

Hi,

On Mon, Sep 26, 2011 at 11:52 PM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> On 27/09/2011 00:28, Scott Wood wrote:
>
>>> - users who actually wisht to limit outpout ca use either
>>
>> You say "actually wish to limit output" as if "let it corrupt memory if
>> it's too large" is the normal thing to want.
>
> What I meant was "users who actually want to limit output explicitly by
> truncating it rather than implicitly by always outputting small enough text
> or crafting limitations in the format string specifiers, ..."
>
>> -scott
>
> Amicalement,
> --
> Albert.
>

Just to follow up this thread, I am going to drop the network patch
and resubmit the rest of it. This will make snprintf() available in
U-Boot.

Regards,
Simon

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

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ3hYmXEHr8MT0deZ054kuOxkNDt4Sa+nVPSzPAdAQFf0w@mail.gmail.com> you wrote:
> 
> Just to follow up this thread, I am going to drop the network patch
> and resubmit the rest of it. This will make snprintf() available in
> U-Boot.

Without code that uses it?  We don't accept dead code...

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
Due to lack of disk space, this fortune database has been discontinued.

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

[Simon Glass]

Hi Wolfgang,

On Mon, Oct 10, 2011 at 1:36 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ3hYmXEHr8MT0deZ054kuOxkNDt4Sa+nVPSzPAdAQFf0w@mail.gmail.com> you wrote:
>>
>> Just to follow up this thread, I am going to drop the network patch
>> and resubmit the rest of it. This will make snprintf() available in
>> U-Boot.
>
> Without code that uses it? ?We don't accept dead code...

It is used by printf() and the like - please see console.c.

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 at denx.de
> Due to lack of disk space, this fortune database has been discontinued.
>

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

[Wolfgang Denk]

Dear Simon Glass,

In message <1316799532-20761-1-git-send-email-sjg@chromium.org> you 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, and also
> some network code (as an example of where this might lead).

What's the impact of this patch set on the memory footprint of typical
configurations?

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
                  Nail here --X-- for new monitor.

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

[Simon Glass]

Hi Wolfgang,

On Sun, Sep 25, 2011 at 1:04 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <1316799532-20761-1-git-send-email-sjg@chromium.org> you 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, and also
>> some network code (as an example of where this might lead).
>
> What's the impact of this patch set on the memory footprint of typical
> configurations?

Good question. The short answer with my ARMv7 compiler
(gcc-4.4.3_cos_gg_53174) is 328 bytes, about 10% increase to code size
vsprintf.o.

The newly added functions (snprintf, vscnprintf, scnprintf) are a
total of 116 bytes.

The changes to number(), string() and vsprintf() to make them respect
an end pointer increase size by 80, 20 and 80 bytes respectively.

Total text size for existing vsprintf.o functions goes from 0xc10
(3088) to 0xd58 (3416), or 328 bytes. Of this 116 bytes is the new
functions and the rest is dealing with the end pointer. There is no
data.

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 at denx.de
> ? ? ? ? ? ? ? ? ?Nail here --X-- for new monitor.
>