Hi Alexandre,
At Mon, 18 Feb 2013 10:24:56 -0800,
Greg Kroah-Hartman wrote:
>
> 3.4-stable review patch. If anyone has any objections, please let me know.
>
> ------------------
>
> From: Alexandre SIMON <alexandre.si...@univ-lorraine.fr>
>
> 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.
I reviewed this patch and it seems to be good for me. Since I'm not good at
printk code, I want to confirm whether what I think is correct or not.
Is the following my understanding correct?
> - cur_index += log_prefix(&LOG_BUF(cur_index),
> &msg_level, NULL);
Here is one example of the problematic case.
+---- start of log_buf
|
| +--- end of log_buf
| |
v v
<-------- log_buf ----------><------- * outside of log_buf. Don't access here
!!! * --- ~~~
^
|
cur_index
In this case, only LOG_BUF(cur_index) is safe to access and
- "LOG_BUF(cur_index) + 1" as p[1],
- "LOG_BUF(cur_index) + 2" as p[2], and
- "LOG_BUF(cur_index) + 1 or more" as simple_strtoul(&p[1], &endp, 10)
in log_prefix() are not to do so. Hence touching them would cause the system
hang as you
said as follows.
>
> 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.
...
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -638,8 +638,19 @@ static void call_console_drivers(unsigne
> 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++) {
The condition, "i < ((end - cur_index)) && (i < SYSLOG_PRI_MAX_LENGTH)", is to
prevent
access over
- the region to write out, and
- the max length of log_prefix.
In addition, "min(end - cur_index, SYSLOG_PRI_MAX_LENGTH)" has the same meaning
here.
> + buf_prefix[i] = LOG_BUF(cur_index + i);
You ensure that all characters (not only first character) in the candidate of
log_prefix
are inside of log_buf here by copying each character of them one by one.
Thanks,
Satoru
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
