On Mon, Jan 28, 2013 at 01:51:31PM +0100, Alexandre SIMON wrote: > [Serge E. Hallyn] wrote the following on [26/01/2013 23:01]: > > Quoting Alexandre SIMON ([email protected]): > >> commit 4234697a64df3b1524aa047448930ff940c1bb92 upstream. > >> > >> This patch corrects a buffer overflow in kernels from 3.0 to 3.4 when > >> calling > >> log_prefix() function from call_console_drivers(). > >> > >> This bug existed in previous releases but has been revealed with commit > >> 162a7e7500f9664636e649ba59defe541b7c2c60 (2.6.39 => 3.0) that made changes > >> about how to allocate memory for early printk buffer (use of > >> memblock_alloc). > >> It disappears with commit 7ff9554bb578ba02166071d2d487b7fc7d860d62 (3.4 => > >> 3.5) > >> that does a refactoring of printk buffer management. > >> > >> In log_prefix(), the access to "p[0]", "p[1]", "p[2]" or > >> "simple_strtoul(&p[1], &endp, 10)" may cause a buffer overflow as this > >> function is called from call_console_drivers by passing > >> "&LOG_BUF(cur_index)" > >> where the index must be masked to do not exceed the buffer's boundary. > >> > >> The trick is to prepare in call_console_drivers() a buffer with the > >> necessary > >> data (PRI field of syslog message) to be safely evaluated in log_prefix(). > >> > >> This patch can be applied to stable kernel branches 3.0.y, 3.2.y and 3.4.y. > >> > >> Without this patch, one can freeze a server running this loop from shell : > >> $ export DUMMY=`cat /dev/urandom | tr -dc > >> '12345AZERTYUIOPQSDFGHJKLMWXCVBNazertyuiopqsdfghjklmwxcvbn' | head -c255` > >> $ while true do ; echo $DUMMY > /dev/kmsg ; done > >> > >> The "server freeze" depends on where memblock_alloc does allocate printk > >> buffer : > >> if the buffer overflow is inside another kernel allocation the problem may > >> not > >> be revealed, else the server may hangs up. > >> > >> Signed-off-by: Alexandre SIMON <[email protected]> > >> --- > >> include/linux/syslog.h | 6 ++++++ > >> kernel/printk.c | 12 +++++++++++- > >> 2 files changed, 17 insertions(+), 1 deletion(-) > >> > >> diff --git a/include/linux/syslog.h b/include/linux/syslog.h > >> index 3891139..ce4c665 100644 > >> --- a/include/linux/syslog.h > >> +++ b/include/linux/syslog.h > >> @@ -47,6 +47,12 @@ > >> #define SYSLOG_FROM_CALL 0 > >> #define SYSLOG_FROM_FILE 1 > >> > >> +/* > >> + * Syslog priority (PRI) maximum length in char : '<[0-9]{1,3}>' > >> + * See RFC5424 for details > >> +*/ > >> +#define SYSLOG_PRI_MAX_LENGTH 5 > >> + > >> int do_syslog(int type, char __user *buf, int count, bool from_file); > >> > >> #endif /* _LINUX_SYSLOG_H */ > >> diff --git a/kernel/printk.c b/kernel/printk.c > >> index 3fc4708..130b436 100644 > >> --- a/kernel/printk.c > >> +++ b/kernel/printk.c > >> @@ -633,8 +633,18 @@ static void call_console_drivers(unsigned start, > >> unsigned end) > >> start_print = start; > >> while (cur_index != end) { > >> if (msg_level < 0 && ((end - cur_index) > 2)) { > >> + /* > >> + * prepare buf_prefix, as a contiguous array, > >> + * to be processed by log_prefix function > >> + */ > >> + char buf_prefix[SYSLOG_PRI_MAX_LENGTH+1]; > >> + unsigned i; > >> + for (i = 0; i < ((end - cur_index)) && (i < > >> SYSLOG_PRI_MAX_LENGTH); i++) { > >> + buf_prefix[i] = LOG_BUF(cur_index + i); > >> + } > > > > You're not guaranteeing that bug_prefix ends with a '\0'. (I'd assume > > LOG_BUF(end) is '\0', and you don't copy it) Is that guaranteed some > > other way? If not, does that not introduce new potential bugginess when > > doing the simple_strtoul()? > > You are totally right. > > It can be fixed with just a line after the loop block : > for (i = 0; i < ((end - cur_index)) && (i < SYSLOG_PRI_MAX_LENGTH); i++) { > ... > } > buf_prefix[i] = '\0'; /* force \0 as last string character */ > > => If you agree, I can re-post the entire patch with this add. > > Just for me : you told about something that "LOG_BUF(end) is '\0'". I did > not see it in the code. Maybe the initialization is not obvious.
Please resend, I can't take this as-is. greg k-h -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
