[ 0/1] 3.4.33-stable review

[ 0/1] 3.4.33-stable review

[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:
	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 <gregkh@linuxfoundation.org>
    Linux 3.4.33-rc1

Alexandre SIMON <Alexandre.Simon@univ-lorraine.fr>
    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(-)

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

[Greg Kroah-Hartman]

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

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

From: Alexandre SIMON <Alexandre.Simon@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.

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 <Alexandre.Simon@univ-lorraine.fr>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

--- 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 */
--- 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++) {
+				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) {

Re: [ 0/1] 3.4.33-stable review

[Shuah Khan]

On Mon, Feb 18, 2013 at 11:24 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> 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
>
> -------------

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

Re: [ 0/1] 3.4.33-stable review

[Greg Kroah-Hartman]

On Mon, Feb 18, 2013 at 07:50:16PM -0700, Shuah Khan wrote:
> On Mon, Feb 18, 2013 at 11:24 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > 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
> >
> > -------------
> 
> 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

Re: [ 0/1] 3.4.33-stable review

[Shuah Khan]

On Tue, Feb 19, 2013 at 7:51 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Feb 18, 2013 at 07:50:16PM -0700, Shuah Khan wrote:
>> On Mon, Feb 18, 2013 at 11:24 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > 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
>> >
>> > -------------
>>
>> 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

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

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

[Satoru Takeuchi]

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

Re: [ 0/1] 3.4.33-stable review

[Satoru Takeuchi]

At Tue, 19 Feb 2013 20:31:02 -0700,
Shuah Khan wrote:
> 
> On Tue, Feb 19, 2013 at 7:51 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Feb 18, 2013 at 07:50:16PM -0700, Shuah Khan wrote:
> >> On Mon, Feb 18, 2013 at 11:24 AM, Greg Kroah-Hartman
> >> <gregkh@linuxfoundation.org> wrote:
> >> > 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
> >> >
> >> > -------------
> >>
> >> 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
> 
> 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

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

[Alexandre SIMON]

[Satoru Takeuchi] wrote the following on [20/02/2013 14:02]:
[...]
> 
> 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 !


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

  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.


[...]
>> +			/*
>> +			 * 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

  Yes, because buf_prefix has a fixed length.


>  - the max length of log_prefix.

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


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

  Yes, that may also work.


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


> Thanks,
> Satoru

  Bye, Alex.

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

[Satoru Takeuchi]

At Wed, 20 Feb 2013 14:43:09 +0100,
Alexandre SIMON wrote:
> 
> [Satoru Takeuchi] wrote the following on [20/02/2013 14:02]:
> [...]
> > 
> > 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 !
...
> > 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