3.4.33-stable review

PATCH In review Greg Kroah-Hartman 4 participants 9 messages Feb 18, 2013 – Feb 20, 2013 Stable branch

Share & tools

Greg Kroah-Hartman

author maintainer

2013-02-18 18:24 UTC #0 lore raw

cover 3.4.33-stable review

This is the start of the stable review cycle for the 3.4.33 release. There is 1 patch in this series, which will be posted as a response to this one. If anyone has any issues with it being applied, please let me know.

Responses should be made by Wed Feb 20 18:13:44 UTC 2013. Anything received after that time might be too late.

The whole patch series can be found in one patch at:

	kernel.org/pub/linux/kernel/v3.0/stable-review/patch-3.4.33-rc1.gz

and the diffstat can be found below.

thanks,

greg k-h

-------------
Pseudo-Shortlog of commits:

Greg Kroah-Hartman <[email protected]>

    Linux 3.4.33-rc1

Alexandre SIMON <[email protected]>
printk: fix buffer overflow when calling log_prefix function from call_console_drivers

-------------

Diffstat:

 Makefile               |  4 ++--
 include/linux/syslog.h |  6 ++++++
 kernel/printk.c        | 13 ++++++++++++-
 3 files changed, 20 insertions(+), 3 deletions(-)

Greg Kroah-Hartman

author maintainer

2013-02-18 18:24 UTC #1 lore raw

PATCH 1/1 printk: fix buffer overflow when calling log_prefix function from call_console_drivers

3.4-stable review patch. If anyone has any objections, please let me know.

------------------

From: Alexandre SIMON <[email protected]>

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

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 include/linux/syslog.h |    6 ++++++
 kernel/printk.c        |   13 ++++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

include/linux/syslog.h +6 -0

@@ -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 */

kernel/printk.c +12 -1

@@ -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++) {
+				buf_prefix[i] = LOG_BUF(cur_index + i);
+			}
+			buf_prefix[i] = '\0'; /* force '\0' as last string character */
+
 			/* strip log prefix */
-			cur_index += log_prefix(&LOG_BUF(cur_index), &msg_level, NULL);
+			cur_index += log_prefix((const char *)&buf_prefix, &msg_level, NULL);
 			start_print = cur_index;
 		}
 		while (cur_index != end) {

Satoru Takeuchi

2013-02-20 13:02 UTC #2 lore raw

Re: [ 1/1] printk: fix buffer overflow when calling log_prefix function from call_console_drivers

Hi Alexandre,

At Mon, 18 Feb 2013 10:24:56 -0800,

Quoted 16 lines · Greg Kroah-Hartman

3.4-stable review patch. If anyone has any objections, please let me know.
------------------
From: Alexandre SIMON <[email protected]>
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.

Quoted 4 lines

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?

kernel/printk.c static void call_console_drivers(unsigne · line 642

Quoted 1 line

kernel/printk.c +0 -1

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

Quoted 13 lines

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.

...

kernel/printk.c static void call_console_drivers(unsigne · line 647

Quoted 13 lines

kernel/printk.c +7 -0

@@ -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.

kernel/printk.c static void call_console_drivers(unsigne · line 648

Quoted 1 line

kernel/printk.c +1 -0

+				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

Alexandre SIMON

2013-02-20 13:43 UTC #3 lore raw

Re: [ 1/1] printk: fix buffer overflow when calling log_prefix function from call_console_drivers

[Satoru Takeuchi] wrote the following on [20/02/2013 14:02]:
[...]

Quoted 4 lines

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?

No problem, it's nice to talk about this patch !

Quoted 14 lines

kernel/printk.c static void call_console_drivers(unsigne · line 642

Quoted 1 line

kernel/printk.c +0 -1

-			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

Quoted 8 lines

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.

Yes, your analyze is totally right. Your figure is clear and shows exactly when the problem occurs in the "borderline case".
The initial code does not check that the "cur_index" can be at the end of "log_buf" whereas "log_prefix" function may access to one, two or more indexes after.

[...]

Quoted 11 lines

kernel/printk.c static void call_console_drivers(unsigne · line 647
Quoted 7 lines
kernel/printk.c +7 -0
+			/*
+			 * 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

Quoted 1 line

- the region to write out, and

Yes, because buf_prefix has a fixed length.

Quoted 1 line

- the max length of log_prefix.

In another term : to do not exceed the end boundary of "log_buf".

Quoted 1 line

In addition, "min(end - cur_index, SYSLOG_PRI_MAX_LENGTH)" has the same meaning here.

Yes, that may also work.

Quoted 4 lines

kernel/printk.c static void call_console_drivers(unsigne · line 648
Quoted 1 line
kernel/printk.c +1 -0
+				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.

Yes, I also take care of the index that must be masked to wrap around the log_buf : we restart at index=0 when cur_index > length(log_buf).
At this point, we are sure that the buffer passed to log_prefix is consistent and all the access (through p[1], p[2], simple_strtoul(...)) may not cause a buffer overflow.

Quoted 2 lines

Thanks,
Satoru

Bye, Alex.

Satoru Takeuchi

2013-02-20 15:56 UTC #4 lore raw

Re: [ 1/1] printk: fix buffer overflow when calling log_prefix function from call_console_drivers

At Wed, 20 Feb 2013 14:43:09 +0100,

Quoted 9 lines · Alexandre SIMON

[Satoru Takeuchi] wrote the following on [20/02/2013 14:02]:
[...]
Quoted 4 lines
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?
No problem, it's nice to talk about this patch !

...

Quoted 11 lines

Quoted 8 lines
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.
Yes, your analyze is totally right. Your figure is clear and shows exactly when the problem occurs in the "borderline case".
The initial code does not check that the "cur_index" can be at the end of "log_buf" whereas "log_prefix" function may access to one, two or more indexes after.

Alex, thank you for quick response and the detailed explanation. Greg, then I can say this patch looks correct and 3.0.66-rc1 version is too.

Thanks,
Satoru

Shuah Khan

2013-02-19 02:50 UTC #5 lore raw

In reply to Greg Kroah-Hartman

Quoted 9 lines · Greg Kroah-Hartman

This is the start of the stable review cycle for the 3.4.33 release. There is 1 patch in this series, which will be posted as a response to this one. If anyone has any issues with it being applied, please let me know.
Responses should be made by Wed Feb 20 18:13:44 UTC 2013. Anything received after that time might be too late.
The whole patch series can be found in one patch at:

Quoted 8 lines

        kernel.org/pub/linux/kernel/v3.0/stable-review/patch-3.4.33-rc1.gz
and the diffstat can be found below.
thanks,
greg k-h
-------------

Patches applied cleanly to 3.0.65, and 3.4.32 Compiled and booted on the following systems:
HP EliteBook 6930p Intel(R) Core(TM)2 Duo CPU T9400 @ 2.53GHz HP ProBook 6475b AMD A10-4600M APU with Radeon(tm) HD Graphics

Reviewed patches

Skipped Cross-compile tests

-- Shuah

Greg Kroah-Hartman

author maintainer

2013-02-20 02:51 UTC #6 lore raw

Quoted 20 lines · Shuah Khan

Quoted 9 lines · Greg Kroah-Hartman
This is the start of the stable review cycle for the 3.4.33 release. There is 1 patch in this series, which will be posted as a response to this one. If anyone has any issues with it being applied, please let me know.
Responses should be made by Wed Feb 20 18:13:44 UTC 2013. Anything received after that time might be too late.
The whole patch series can be found in one patch at:
Quoted 8 lines
        kernel.org/pub/linux/kernel/v3.0/stable-review/patch-3.4.33-rc1.gz
and the diffstat can be found below.
thanks,
greg k-h
-------------

Quoted 8 lines

Patches applied cleanly to 3.0.65, and 3.4.32 Compiled and booted on the following systems:
HP EliteBook 6930p Intel(R) Core(TM)2 Duo CPU T9400 @ 2.53GHz HP ProBook 6475b AMD A10-4600M APU with Radeon(tm) HD Graphics
Reviewed patches
Skipped Cross-compile tests

Thanks for testing, your kernel log messages looked correct, right?

greg k-h

Shuah Khan

2013-02-20 03:31 UTC #7 lore raw

Quoted 26 lines · Greg Kroah-Hartman

Quoted 25 lines · Shuah Khan
Quoted 9 lines · Greg Kroah-Hartman
This is the start of the stable review cycle for the 3.4.33 release. There is 1 patch in this series, which will be posted as a response to this one. If anyone has any issues with it being applied, please let me know.
Responses should be made by Wed Feb 20 18:13:44 UTC 2013. Anything received after that time might be too late.
The whole patch series can be found in one patch at:
Quoted 8 lines
        kernel.org/pub/linux/kernel/v3.0/stable-review/patch-3.4.33-rc1.gz
and the diffstat can be found below.
thanks,
greg k-h
-------------
Patches applied cleanly to 3.0.65, and 3.4.32 Compiled and booted on the following systems:
HP EliteBook 6930p Intel(R) Core(TM)2 Duo CPU T9400 @ 2.53GHz HP ProBook 6475b AMD A10-4600M APU with Radeon(tm) HD Graphics

Quoted 7 lines

Quoted 3 lines
Reviewed patches
Skipped Cross-compile tests
Thanks for testing, your kernel log messages looked correct, right?
greg k-h

Yes, should have mentioned that I checked dmesg and syslog both and compared dmesg
with the old dmesg I saved from the previous rc-1 testing for each of these releases. Didn't
see anything that jumped out at me. I usually save dmesg from rc cycles to look for regressions
if any from one rc cycle to the next.

-- Shuah

Satoru Takeuchi

2013-02-20 13:09 UTC #8 lore raw

At Tue, 19 Feb 2013 20:31:02 -0700,

Quoted 37 lines · Shuah Khan

Quoted 26 lines · Greg Kroah-Hartman
Quoted 25 lines · Shuah Khan
Quoted 9 lines · Greg Kroah-Hartman
This is the start of the stable review cycle for the 3.4.33 release. There is 1 patch in this series, which will be posted as a response to this one. If anyone has any issues with it being applied, please let me know.
Responses should be made by Wed Feb 20 18:13:44 UTC 2013. Anything received after that time might be too late.
The whole patch series can be found in one patch at:
Quoted 8 lines
        kernel.org/pub/linux/kernel/v3.0/stable-review/patch-3.4.33-rc1.gz
and the diffstat can be found below.
thanks,
greg k-h
-------------
Patches applied cleanly to 3.0.65, and 3.4.32 Compiled and booted on the following systems:
HP EliteBook 6930p Intel(R) Core(TM)2 Duo CPU T9400 @ 2.53GHz HP ProBook 6475b AMD A10-4600M APU with Radeon(tm) HD Graphics
Quoted 7 lines
Quoted 3 lines
Reviewed patches
Skipped Cross-compile tests
Thanks for testing, your kernel log messages looked correct, right?
greg k-h

Quoted 7 lines

Yes, should have mentioned that I checked dmesg and syslog both and compared dmesg
with the old dmesg I saved from the previous rc-1 testing for each of these releases. Didn't
see anything that jumped out at me. I usually save dmesg from rc cycles to look for regressions
if any from one rc cycle to the next.

+1.

This kernel can be built and boot without any problem. Building a kernel with this kernel also works fine.
* Its dmesg also looks correct. *

- Build Machine: debian wheezy x86_64
CPU: Intel(R) Core(TM) i5-2400 CPU @ 3.10GHz x 4
memory: 8GB

- Test machine: debian wheezy x86_64(KVM guest on the Build Machine)
vCPU: x2
memory: 2GB

Thanks,
Satoru