[PATCH] init - fix building bug and potential buffer overflow

[PATCH] init - fix building bug and potential buffer overflow

[Cyrill Gorcunov]

This patch does fix build bug on m68k wich does not have strncat in straight way.

What is more important - my previous patch

commit e662e1cfd434aa234b72fbc781f1d70211cb785b
Author: Cyrill Gorcunov <gorcunov@gmail.com>
Date:   Mon May 12 14:02:22 2008 -0700

    init: don't lose initcall return values

has introduced potential buffer overflow by wrong calculation
of string accumulator size.

Many thanks Andreas Schwab and Geert Uytterhoeven for helping
to catch and fix the bug.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---

Index: linux-2.6.git/init/main.c
===================================================================
--- linux-2.6.git.orig/init/main.c	2008-05-14 17:55:10.000000000 +0400
+++ linux-2.6.git/init/main.c	2008-05-14 19:11:18.000000000 +0400
@@ -702,7 +702,7 @@ static void __init do_initcalls(void)
 
 	for (call = __initcall_start; call < __initcall_end; call++) {
 		ktime_t t0, t1, delta;
-		char msgbuf[40];
+		char msgbuf[64];
 		int result;
 
 		if (initcall_debug) {
@@ -729,11 +729,11 @@ static void __init do_initcalls(void)
 			sprintf(msgbuf, "error code %d ", result);
 
 		if (preempt_count() != count) {
-			strncat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
+			strcat(msgbuf, "preemption imbalance ");
 			preempt_count() = count;
 		}
 		if (irqs_disabled()) {
-			strncat(msgbuf, "disabled interrupts ", sizeof(msgbuf));
+			strcat(msgbuf, "disabled interrupts ");
 			local_irq_enable();
 		}
 		if (msgbuf[0]) {

Re: [PATCH] init - fix building bug and potential buffer overflow

[Andrew Morton]

On Wed, 14 May 2008 19:44:02 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> This patch does fix build bug on m68k wich does not have strncat in straight way.
> 
> What is more important - my previous patch
> 
> commit e662e1cfd434aa234b72fbc781f1d70211cb785b
> Author: Cyrill Gorcunov <gorcunov@gmail.com>
> Date:   Mon May 12 14:02:22 2008 -0700
> 
>     init: don't lose initcall return values
> 
> has introduced potential buffer overflow by wrong calculation
> of string accumulator size.
> 
> Many thanks Andreas Schwab and Geert Uytterhoeven for helping
> to catch and fix the bug.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
> 
> Index: linux-2.6.git/init/main.c
> ===================================================================
> --- linux-2.6.git.orig/init/main.c	2008-05-14 17:55:10.000000000 +0400
> +++ linux-2.6.git/init/main.c	2008-05-14 19:11:18.000000000 +0400
> @@ -702,7 +702,7 @@ static void __init do_initcalls(void)
>  
>  	for (call = __initcall_start; call < __initcall_end; call++) {
>  		ktime_t t0, t1, delta;
> -		char msgbuf[40];
> +		char msgbuf[64];
>  		int result;
>  
>  		if (initcall_debug) {
> @@ -729,11 +729,11 @@ static void __init do_initcalls(void)
>  			sprintf(msgbuf, "error code %d ", result);
>  
>  		if (preempt_count() != count) {
> -			strncat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
> +			strcat(msgbuf, "preemption imbalance ");
>  			preempt_count() = count;
>  		}
>  		if (irqs_disabled()) {
> -			strncat(msgbuf, "disabled interrupts ", sizeof(msgbuf));
> +			strcat(msgbuf, "disabled interrupts ");
>  			local_irq_enable();
>  		}
>  		if (msgbuf[0]) {

umm, why can't m68k call strncat() from init/main.c??

Re: [PATCH] init - fix building bug and potential buffer overflow

[Cyrill Gorcunov]

[Andrew Morton - Thu, May 15, 2008 at 10:58:03AM -0700]
| On Wed, 14 May 2008 19:44:02 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > This patch does fix build bug on m68k wich does not have strncat in straight way.
| > 
| > What is more important - my previous patch
| > 
| > commit e662e1cfd434aa234b72fbc781f1d70211cb785b
| > Author: Cyrill Gorcunov <gorcunov@gmail.com>
| > Date:   Mon May 12 14:02:22 2008 -0700
| > 
| >     init: don't lose initcall return values
| > 
| > has introduced potential buffer overflow by wrong calculation
| > of string accumulator size.
| > 
| > Many thanks Andreas Schwab and Geert Uytterhoeven for helping
| > to catch and fix the bug.
| > 
| > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
| > ---
| > 
| > Index: linux-2.6.git/init/main.c
| > ===================================================================
| > --- linux-2.6.git.orig/init/main.c	2008-05-14 17:55:10.000000000 +0400
| > +++ linux-2.6.git/init/main.c	2008-05-14 19:11:18.000000000 +0400
| > @@ -702,7 +702,7 @@ static void __init do_initcalls(void)
| >  
| >  	for (call = __initcall_start; call < __initcall_end; call++) {
| >  		ktime_t t0, t1, delta;
| > -		char msgbuf[40];
| > +		char msgbuf[64];
| >  		int result;
| >  
| >  		if (initcall_debug) {
| > @@ -729,11 +729,11 @@ static void __init do_initcalls(void)
| >  			sprintf(msgbuf, "error code %d ", result);
| >  
| >  		if (preempt_count() != count) {
| > -			strncat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
| > +			strcat(msgbuf, "preemption imbalance ");
| >  			preempt_count() = count;
| >  		}
| >  		if (irqs_disabled()) {
| > -			strncat(msgbuf, "disabled interrupts ", sizeof(msgbuf));
| > +			strcat(msgbuf, "disabled interrupts ");
| >  			local_irq_enable();
| >  		}
| >  		if (msgbuf[0]) {
| 
| umm, why can't m68k call strncat() from init/main.c??
| 

there some problem with headers iirc, we have to declare it first or
use some gcc option (as Adrian suggested). Actually I would prefer to use
strlcat there but it seems it would fail to build too. Originally I've messed
strlcat with strncat :(

		- Cyrill -

Re: [PATCH] init - fix building bug and potential buffer overflow

[Geert Uytterhoeven]

On Thu, 15 May 2008, Cyrill Gorcunov wrote:
> [Andrew Morton - Thu, May 15, 2008 at 10:58:03AM -0700]
> | On Wed, 14 May 2008 19:44:02 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> | 
> | > This patch does fix build bug on m68k wich does not have strncat in straight way.
> | > 
> | > What is more important - my previous patch
> | > 
> | > commit e662e1cfd434aa234b72fbc781f1d70211cb785b
> | > Author: Cyrill Gorcunov <gorcunov@gmail.com>
> | > Date:   Mon May 12 14:02:22 2008 -0700
> | > 
> | >     init: don't lose initcall return values
> | > 
> | > has introduced potential buffer overflow by wrong calculation
> | > of string accumulator size.
> | > 
> | > Many thanks Andreas Schwab and Geert Uytterhoeven for helping
> | > to catch and fix the bug.
> | > 
> | > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> | > ---
> | > 
> | > Index: linux-2.6.git/init/main.c
> | > ===================================================================
> | > --- linux-2.6.git.orig/init/main.c	2008-05-14 17:55:10.000000000 +0400
> | > +++ linux-2.6.git/init/main.c	2008-05-14 19:11:18.000000000 +0400
> | > @@ -702,7 +702,7 @@ static void __init do_initcalls(void)
> | >  
> | >  	for (call = __initcall_start; call < __initcall_end; call++) {
> | >  		ktime_t t0, t1, delta;
> | > -		char msgbuf[40];
> | > +		char msgbuf[64];
> | >  		int result;
> | >  
> | >  		if (initcall_debug) {
> | > @@ -729,11 +729,11 @@ static void __init do_initcalls(void)
> | >  			sprintf(msgbuf, "error code %d ", result);
> | >  
> | >  		if (preempt_count() != count) {
> | > -			strncat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
> | > +			strcat(msgbuf, "preemption imbalance ");
> | >  			preempt_count() = count;
> | >  		}
> | >  		if (irqs_disabled()) {
> | > -			strncat(msgbuf, "disabled interrupts ", sizeof(msgbuf));
> | > +			strcat(msgbuf, "disabled interrupts ");
> | >  			local_irq_enable();
> | >  		}
> | >  		if (msgbuf[0]) {
> | 
> | umm, why can't m68k call strncat() from init/main.c??
> | 
> 
> there some problem with headers iirc, we have to declare it first or
> use some gcc option (as Adrian suggested). Actually I would prefer to use

gcc turns the strncat() into an implicit call to strlen() and some form
of expanded memcpy(). E.g.


	if (preempt_count() != count) {
		strncat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
		preempt_count() = count;
	}

becomes

        cmp.l 884(%a2),%d6      | <variable>.thread.info.preempt_count, count
        jeq .L61        |
        move.l %d7,-(%sp)       | tmp76,
        jbsr strlen     |
        addq.l #4,%sp   |,
        move.l %d7,%a0  | tmp76, tmp80
        add.l %d0,%a0   |, tmp80
        move.l #1886545253,(%a0)+       |,
        move.l #1836086377,(%a0)+       |,
        move.l #1869488233,(%a0)+       |,
        move.l #1835164012,(%a0)+       |,
        move.l #1634624357,(%a0)+       |,
        move.w #8192,(%a0)      |,* D.28541
        move.l %d6,884(%a2)     | count, <variable>.thread.info.preempt_count
.L61:

All other explicit calls to strlen() are inlined, as per
include/asm-m68k/string.h.

> strlcat there but it seems it would fail to build too. Originally I've messed
> strlcat with strncat :(

Actually it build and runs fine after s/strncat/strlcat/...

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] init - fix building bug and potential buffer overflow

[Cyrill Gorcunov]

Could you please make an update to the patch? I can make it only
tomorrow evening (ie not that fast)

On 5/15/08, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, 15 May 2008, Cyrill Gorcunov wrote:
>> [Andrew Morton - Thu, May 15, 2008 at 10:58:03AM -0700]
>> | On Wed, 14 May 2008 19:44:02 +0400 Cyrill Gorcunov <gorcunov@gmail.com>
>> wrote:
>> |
>> | > This patch does fix build bug on m68k wich does not have strncat in
>> straight way.
>> | >
>> | > What is more important - my previous patch
>> | >
>> | > commit e662e1cfd434aa234b72fbc781f1d70211cb785b
>> | > Author: Cyrill Gorcunov <gorcunov@gmail.com>
>> | > Date:   Mon May 12 14:02:22 2008 -0700
>> | >
>> | >     init: don't lose initcall return values
>> | >
>> | > has introduced potential buffer overflow by wrong calculation
>> | > of string accumulator size.
>> | >
>> | > Many thanks Andreas Schwab and Geert Uytterhoeven for helping
>> | > to catch and fix the bug.
>> | >
>> | > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
>> | > ---
>> | >
>> | > Index: linux-2.6.git/init/main.c
>> | > ===================================================================
>> | > --- linux-2.6.git.orig/init/main.c	2008-05-14 17:55:10.000000000 +0400
>> | > +++ linux-2.6.git/init/main.c	2008-05-14 19:11:18.000000000 +0400
>> | > @@ -702,7 +702,7 @@ static void __init do_initcalls(void)
>> | >
>> | >  	for (call = __initcall_start; call < __initcall_end; call++) {
>> | >  		ktime_t t0, t1, delta;
>> | > -		char msgbuf[40];
>> | > +		char msgbuf[64];
>> | >  		int result;
>> | >
>> | >  		if (initcall_debug) {
>> | > @@ -729,11 +729,11 @@ static void __init do_initcalls(void)
>> | >  			sprintf(msgbuf, "error code %d ", result);
>> | >
>> | >  		if (preempt_count() != count) {
>> | > -			strncat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
>> | > +			strcat(msgbuf, "preemption imbalance ");
>> | >  			preempt_count() = count;
>> | >  		}
>> | >  		if (irqs_disabled()) {
>> | > -			strncat(msgbuf, "disabled interrupts ", sizeof(msgbuf));
>> | > +			strcat(msgbuf, "disabled interrupts ");
>> | >  			local_irq_enable();
>> | >  		}
>> | >  		if (msgbuf[0]) {
>> |
>> | umm, why can't m68k call strncat() from init/main.c??
>> |
>>
>> there some problem with headers iirc, we have to declare it first or
>> use some gcc option (as Adrian suggested). Actually I would prefer to use
>
> gcc turns the strncat() into an implicit call to strlen() and some form
> of expanded memcpy(). E.g.
>
>
> 	if (preempt_count() != count) {
> 		strncat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
> 		preempt_count() = count;
> 	}
>
> becomes
>
>         cmp.l 884(%a2),%d6      | <variable>.thread.info.preempt_count,
> count
>         jeq .L61        |
>         move.l %d7,-(%sp)       | tmp76,
>         jbsr strlen     |
>         addq.l #4,%sp   |,
>         move.l %d7,%a0  | tmp76, tmp80
>         add.l %d0,%a0   |, tmp80
>         move.l #1886545253,(%a0)+       |,
>         move.l #1836086377,(%a0)+       |,
>         move.l #1869488233,(%a0)+       |,
>         move.l #1835164012,(%a0)+       |,
>         move.l #1634624357,(%a0)+       |,
>         move.w #8192,(%a0)      |,* D.28541
>         move.l %d6,884(%a2)     | count,
> <variable>.thread.info.preempt_count
> .L61:
>
> All other explicit calls to strlen() are inlined, as per
> include/asm-m68k/string.h.
>
>> strlcat there but it seems it would fail to build too. Originally I've
>> messed
>> strlcat with strncat :(
>
> Actually it build and runs fine after s/strncat/strlcat/...
>
> Gr{oetje,eeting}s,
>
> 						Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> 							    -- Linus Torvalds
>

Re: [PATCH] init - fix building bug and potential buffer overflow

[Andrew Morton]

On Fri, 16 May 2008 00:22:14 +0400
"Cyrill Gorcunov" <gorcunov@gmail.com> wrote:

> 
> On 5/15/08, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, 15 May 2008, Cyrill Gorcunov wrote:
> >> [Andrew Morton - Thu, May 15, 2008 at 10:58:03AM -0700]
> >> | On Wed, 14 May 2008 19:44:02 +0400 Cyrill Gorcunov <gorcunov@gmail.com>
> >> wrote:
> >> |
> >> | > This patch does fix build bug on m68k wich does not have strncat in
> >> straight way.
> >> | >
> >> | > What is more important - my previous patch
> >> | >
> >> | > commit e662e1cfd434aa234b72fbc781f1d70211cb785b
> >> | > Author: Cyrill Gorcunov <gorcunov@gmail.com>
> >> | > Date:   Mon May 12 14:02:22 2008 -0700
> >> | >
> >> | >     init: don't lose initcall return values
> >> | >
> >> | > has introduced potential buffer overflow by wrong calculation
> >> | > of string accumulator size.
> >> | >
> >> | > Many thanks Andreas Schwab and Geert Uytterhoeven for helping
> >> | > to catch and fix the bug.
> >> | >
> >> | > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> >> | > ---
> >> | >
> >> | > Index: linux-2.6.git/init/main.c
> >> | > ===================================================================
> >> | > --- linux-2.6.git.orig/init/main.c	2008-05-14 17:55:10.000000000 +0400
> >> | > +++ linux-2.6.git/init/main.c	2008-05-14 19:11:18.000000000 +0400
> >> | > @@ -702,7 +702,7 @@ static void __init do_initcalls(void)
> >> | >
> >> | >  	for (call = __initcall_start; call < __initcall_end; call++) {
> >> | >  		ktime_t t0, t1, delta;
> >> | > -		char msgbuf[40];
> >> | > +		char msgbuf[64];
> >> | >  		int result;
> >> | >
> >> | >  		if (initcall_debug) {
> >> | > @@ -729,11 +729,11 @@ static void __init do_initcalls(void)
> >> | >  			sprintf(msgbuf, "error code %d ", result);
> >> | >
> >> | >  		if (preempt_count() != count) {
> >> | > -			strncat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
> >> | > +			strcat(msgbuf, "preemption imbalance ");
> >> | >  			preempt_count() = count;
> >> | >  		}
> >> | >  		if (irqs_disabled()) {
> >> | > -			strncat(msgbuf, "disabled interrupts ", sizeof(msgbuf));
> >> | > +			strcat(msgbuf, "disabled interrupts ");
> >> | >  			local_irq_enable();
> >> | >  		}
> >> | >  		if (msgbuf[0]) {
> >> |
> >> | umm, why can't m68k call strncat() from init/main.c??
> >> |
> >>
> >> there some problem with headers iirc, we have to declare it first or
> >> use some gcc option (as Adrian suggested). Actually I would prefer to use
> >
> > gcc turns the strncat() into an implicit call to strlen() and some form
> > of expanded memcpy(). E.g.
> >
> >
> > 	if (preempt_count() != count) {
> > 		strncat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
> > 		preempt_count() = count;
> > 	}
> >
> > becomes
> >
> >         cmp.l 884(%a2),%d6      | <variable>.thread.info.preempt_count,
> > count
> >         jeq .L61        |
> >         move.l %d7,-(%sp)       | tmp76,
> >         jbsr strlen     |
> >         addq.l #4,%sp   |,
> >         move.l %d7,%a0  | tmp76, tmp80
> >         add.l %d0,%a0   |, tmp80
> >         move.l #1886545253,(%a0)+       |,
> >         move.l #1836086377,(%a0)+       |,
> >         move.l #1869488233,(%a0)+       |,
> >         move.l #1835164012,(%a0)+       |,
> >         move.l #1634624357,(%a0)+       |,
> >         move.w #8192,(%a0)      |,* D.28541
> >         move.l %d6,884(%a2)     | count,
> > <variable>.thread.info.preempt_count
> > .L61:
> >
> > All other explicit calls to strlen() are inlined, as per
> > include/asm-m68k/string.h.
> >
> >> strlcat there but it seems it would fail to build too. Originally I've
> >> messed
> >> strlcat with strncat :(
> >
> > Actually it build and runs fine after s/strncat/strlcat/...
> >

(top-posting repaired)

> Could you please make an update to the patch? I can make it only
> tomorrow evening (ie not that fast)

Like this?


From: Cyrill Gorcunov <gorcunov@gmail.com>

This patch fixes a build bug on m68k - gcc decides to emit a call to the
strlen library function, which we don't implement.  Use strlcat() instead.


What is more important - my previous patch

commit e662e1cfd434aa234b72fbc781f1d70211cb785b
Author: Cyrill Gorcunov <gorcunov@gmail.com>
Date:   Mon May 12 14:02:22 2008 -0700

    init: don't lose initcall return values

Has introduced potential buffer overflow by wrong calculation of string
accumulator size.

Many thanks Andreas Schwab and Geert Uytterhoeven for helping
to catch and fix the bug.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 init/main.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff -puN init/main.c~init-fix-building-bug-and-potential-buffer-overflow init/main.c
--- a/init/main.c~init-fix-building-bug-and-potential-buffer-overflow
+++ a/init/main.c
@@ -702,7 +702,7 @@ static void __init do_initcalls(void)
 
 	for (call = __initcall_start; call < __initcall_end; call++) {
 		ktime_t t0, t1, delta;
-		char msgbuf[40];
+		char msgbuf[64];
 		int result;
 
 		if (initcall_debug) {
@@ -729,11 +729,11 @@ static void __init do_initcalls(void)
 			sprintf(msgbuf, "error code %d ", result);
 
 		if (preempt_count() != count) {
-			strncat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
+			strlcat(msgbuf, "preemption imbalance ", sizeof msgbuf);
 			preempt_count() = count;
 		}
 		if (irqs_disabled()) {
-			strncat(msgbuf, "disabled interrupts ", sizeof(msgbuf));
+			strlcat(msgbuf, "disabled interrupts ", sizeof msgbuf);
 			local_irq_enable();
 		}
 		if (msgbuf[0]) {
_

(yeah, I normally parenthesise sizeof too, but this provided 80-col
salvation)

Re: [PATCH] init - fix building bug and potential buffer overflow

[Linus Torvalds]

On Thu, 15 May 2008, Andrew Morton wrote:
> 
> (yeah, I normally parenthesise sizeof too, but this provided 80-col
> salvation)

Why?

The nice way to provide 80-col salvation is to just fix the code. Instead 
of having complex nested loops, have a really trivial outer loop like

	for (call = __initcall_start; call < __initcall_end; call++)
		do_one_initcall(*call);

and then the code to actually do one of the initcalls is much simpler to 
read, and because it's not as indented, the 80-column issues just go away.

IOW, something like this (the diff looks ugly, but the end result is 
better).

Talking about cleanups, why the *hell* does print_fn_descriptor_symbol() 
take an "unsigned long", when every single user would want to give it a 
pointer?

Would somebody please want to move that cast into the macro (or better 
yet, make it an inline function that takes a 'void *'), and remove all the 
casts from the callers?

		Linus

---
 init/main.c |   78 +++++++++++++++++++++++++++++++----------------------------
 1 files changed, 41 insertions(+), 37 deletions(-)

diff --git a/init/main.c b/init/main.c
index f406fef..0a1d65e 100644
--- a/init/main.c
+++ b/init/main.c
@@ -695,53 +695,57 @@ __setup("initcall_debug", initcall_debug_setup);
 
 extern initcall_t __initcall_start[], __initcall_end[];
 
-static void __init do_initcalls(void)
+static void __init do_one_initcall(initcall_t call)
 {
-	initcall_t *call;
 	int count = preempt_count();
+	ktime_t t0, t1, delta;
+	char msgbuf[64];
+	int result;
+
+	if (initcall_debug) {
+		print_fn_descriptor_symbol("calling  %s()\n",
+				(unsigned long) call);
+		t0 = ktime_get();
+	}
 
-	for (call = __initcall_start; call < __initcall_end; call++) {
-		ktime_t t0, t1, delta;
-		char msgbuf[40];
-		int result;
+	result = call();
 
-		if (initcall_debug) {
-			print_fn_descriptor_symbol("calling  %s()\n",
-					(unsigned long) *call);
-			t0 = ktime_get();
-		}
+	if (initcall_debug) {
+		t1 = ktime_get();
+		delta = ktime_sub(t1, t0);
 
-		result = (*call)();
+		print_fn_descriptor_symbol("initcall %s()",
+				(unsigned long) call);
+		printk(" returned %d after %Ld msecs\n", result,
+			(unsigned long long) delta.tv64 >> 20);
+	}
 
-		if (initcall_debug) {
-			t1 = ktime_get();
-			delta = ktime_sub(t1, t0);
+	msgbuf[0] = 0;
 
-			print_fn_descriptor_symbol("initcall %s()",
-					(unsigned long) *call);
-			printk(" returned %d after %Ld msecs\n", result,
-				(unsigned long long) delta.tv64 >> 20);
-		}
+	if (result && result != -ENODEV && initcall_debug)
+		sprintf(msgbuf, "error code %d ", result);
 
-		msgbuf[0] = 0;
+	if (preempt_count() != count) {
+		strlcat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
+		preempt_count() = count;
+	}
+	if (irqs_disabled()) {
+		strlcat(msgbuf, "disabled interrupts ", sizeof(msgbuf));
+		local_irq_enable();
+	}
+	if (msgbuf[0]) {
+		print_fn_descriptor_symbol(KERN_WARNING "initcall %s()",
+				(unsigned long) call);
+		printk(" returned with %s\n", msgbuf);
+	}
+}
 
-		if (result && result != -ENODEV && initcall_debug)
-			sprintf(msgbuf, "error code %d ", result);
+static void __init do_initcalls(void)
+{
+	initcall_t *call;
 
-		if (preempt_count() != count) {
-			strncat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
-			preempt_count() = count;
-		}
-		if (irqs_disabled()) {
-			strncat(msgbuf, "disabled interrupts ", sizeof(msgbuf));
-			local_irq_enable();
-		}
-		if (msgbuf[0]) {
-			print_fn_descriptor_symbol(KERN_WARNING "initcall %s()",
-					(unsigned long) *call);
-			printk(" returned with %s\n", msgbuf);
-		}
-	}
+	for (call = __initcall_start; call < __initcall_end; call++)
+		do_one_initcall(*call);
 
 	/* Make sure there is no pending stuff from the initcall sequence */
 	flush_scheduled_work();

Re: [PATCH] init - fix building bug and potential buffer overflow

[Andrew Morton]

On Thu, 15 May 2008 14:15:38 -0700 (PDT)
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Thu, 15 May 2008, Andrew Morton wrote:
> > 
> > (yeah, I normally parenthesise sizeof too, but this provided 80-col
> > salvation)
> 
> Why?

this, basically:

>  init/main.c |   78 +++++++++++++++++++++++++++++++----------------------------
>  1 files changed, 41 insertions(+), 37 deletions(-)

the dont-mix-bugfixes-and-cleanups mantra.

The patch looks good to me and compiles cleanly on various architectures.

> Talking about cleanups, why the *hell* does print_fn_descriptor_symbol() 
> take an "unsigned long", when every single user would want to give it a 
> pointer?

Yes, we make that mistake fairly regularly (looks at timer_list.data). 
We start out not being sure whether most callers will pass in a ulong
or a pointer and guess wrongly.

> Would somebody please want to move that cast into the macro (or better 
> yet, make it an inline function that takes a 'void *'), and remove all the 
> casts from the callers?

Would be nice.

[PATCH] Add a void * alternative to print_fn_descriptor_symbol()

[Abhijit Menon-Sen]

At 2008-05-15 14:45:54 -0700, akpm@linux-foundation.org wrote:
>
> > Would somebody please want to move that cast into the macro (or
> > better yet, make it an inline function that takes a 'void *'),
> > and remove all the casts from the callers?
> 
> Would be nice.

Something like this?

-- ams

All the callers of print_fn_descriptor_symbol() had to cast their void *
argument to unsigned long. This patch adds an inline print_fn_symbolic()
that takes a void * directly, and updates the callers to use it instead.

Signed-off-by: Abhijit Menon-Sen <ams@toroid.org>
---
 include/linux/kallsyms.h  |   10 ++++++++++
 drivers/base/power/main.c |    2 +-
 drivers/pci/quirks.c      |    3 +--
 drivers/pnp/quirks.c      |    3 +--
 init/main.c               |    9 +++------
 5 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index 82de2fb..ed8ab42 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -101,6 +101,16 @@ static inline void print_symbol(const char *fmt, unsigned long addr)
 		       __builtin_extract_return_addr((void *)addr));
 }
 
+static inline void print_fn_symbolic(const char *fmt, const void *addr)
+{
+#if defined(CONFIG_IA64) || defined(CONFIG_PPC64)
+	unsigned long *faddr = (unsigned long *)addr;
+	print_symbol(fmt, faddr[0]);
+#else
+	print_symbol(fmt, (unsigned long)addr);
+#endif
+}
+
 #ifndef CONFIG_64BIT
 #define print_ip_sym(ip)		\
 do {					\

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 7b76fd3..9625317 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -418,7 +418,7 @@ void __suspend_report_result(const char *function, void *fn, int ret)
 {
 	if (ret) {
 		printk(KERN_ERR "%s(): ", function);
-		print_fn_descriptor_symbol("%s() returns ", (unsigned long)fn);
+		print_fn_symbolic("%s returns ", fn);
 		printk("%d\n", ret);
 	}
 }

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index f2d9c77..12d07eb 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -1503,8 +1503,7 @@ static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f, struct pci_f
  		    (f->device == dev->device || f->device == (u16) PCI_ANY_ID)) {
 #ifdef DEBUG
 			dev_dbg(&dev->dev, "calling ");
-			print_fn_descriptor_symbol("%s()\n",
-				(unsigned long) f->hook);
+			print_fn_symbolic("%s\n", f->hook);
 #endif
 			f->hook(dev);
 		}

diff --git a/drivers/pnp/quirks.c b/drivers/pnp/quirks.c
index ffdb12a..f84d79f 100644
--- a/drivers/pnp/quirks.c
+++ b/drivers/pnp/quirks.c
@@ -331,8 +331,7 @@ void pnp_fixup_device(struct pnp_dev *dev)
 			continue;
 #ifdef DEBUG
 		dev_dbg(&dev->dev, "%s: calling ", f->id);
-		print_fn_descriptor_symbol("%s\n",
-				(unsigned long) f->quirk_function);
+		print_fn_symbolic("%s\n", f->quirk_function);
 #endif
 		f->quirk_function(dev);
 	}

diff --git a/init/main.c b/init/main.c
index f406fef..c456ca8 100644
--- a/init/main.c
+++ b/init/main.c
@@ -706,8 +706,7 @@ static void __init do_initcalls(void)
 		int result;
 
 		if (initcall_debug) {
-			print_fn_descriptor_symbol("calling  %s()\n",
-					(unsigned long) *call);
+			print_fn_symbolic("calling %s\n", *call);
 			t0 = ktime_get();
 		}
 
@@ -717,8 +716,7 @@ static void __init do_initcalls(void)
 			t1 = ktime_get();
 			delta = ktime_sub(t1, t0);
 
-			print_fn_descriptor_symbol("initcall %s()",
-					(unsigned long) *call);
+			print_fn_symbolic("initcall %s", *call);
 			printk(" returned %d after %Ld msecs\n", result,
 				(unsigned long long) delta.tv64 >> 20);
 		}
@@ -737,8 +735,7 @@ static void __init do_initcalls(void)
 			local_irq_enable();
 		}
 		if (msgbuf[0]) {
-			print_fn_descriptor_symbol(KERN_WARNING "initcall %s()",
-					(unsigned long) *call);
+			print_fn_symbolic(KERN_WARNING "initcall %s", *call);
 			printk(" returned with %s\n", msgbuf);
 		}
 	}
-- 
1.5.5.1

Re: [PATCH] init - fix building bug and potential buffer overflow

[Linus Torvalds]

On Thu, 15 May 2008, Andrew Morton wrote:
> 
> this, basically:
> 
> >  init/main.c |   78 +++++++++++++++++++++++++++++++----------------------------
> >  1 files changed, 41 insertions(+), 37 deletions(-)
> 
> the dont-mix-bugfixes-and-cleanups mantra.

Sure. I committed them as two separate things.

The point *I* had was that we don't make source code uglier for arbitrary 
reasons. The checkpatch.pl script shouldn't be powerful enough that we do 
uglier source code and break arbitrary rules just to make that script 
happy.

So either we should sy "screw the script, it's wrong", or we should just 
not fix the bug. Or we should say "ok, I can write even _better_ source 
code, and fix the bug, make the source look nicer _and_ not piss off the 
script too".

> > Would somebody please want to move that cast into the macro (or better 
> > yet, make it an inline function that takes a 'void *'), and remove all the 
> > casts from the callers?
> 
> Would be nice.

So I did that too.

		Linus

Re: [PATCH] init - fix building bug and potential buffer overflow

[Rene Herman]

On 15-05-08 23:15, Linus Torvalds wrote:

> Talking about cleanups, why the *hell* does print_fn_descriptor_symbol() 
> take an "unsigned long", when every single user would want to give it a 
> pointer?
> 
> Would somebody please want to move that cast into the macro (or better 
> yet, make it an inline function that takes a 'void *'), and remove all the 
> casts from the callers?

[ ... ]

> +		print_fn_descriptor_symbol("calling  %s()\n",
> +				(unsigned long) call);


And while that person is at it, could he or she please remove the silly 
() from the "foo+0x0/0x1234()" (or the "foo+0x0/0x1234 [bar]()" or the 
"0x12345678()").

Rene.

Re: [PATCH] init - fix building bug and potential buffer overflow

[Cyrill Gorcunov]

Yes! Exactly! Thanks, Andrew! (i didn't look into what Linus suggested
'cause i just waked up and have to go to my job)

On 5/16/08, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Fri, 16 May 2008 00:22:14 +0400
> "Cyrill Gorcunov" <gorcunov@gmail.com> wrote:
>
>>
>> On 5/15/08, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> > On Thu, 15 May 2008, Cyrill Gorcunov wrote:
>> >> [Andrew Morton - Thu, May 15, 2008 at 10:58:03AM -0700]
>> >> | On Wed, 14 May 2008 19:44:02 +0400 Cyrill Gorcunov
>> >> <gorcunov@gmail.com>
>> >> wrote:
>> >> |
>> >> | > This patch does fix build bug on m68k wich does not have strncat in
>> >> straight way.
>> >> | >
>> >> | > What is more important - my previous patch
>> >> | >
>> >> | > commit e662e1cfd434aa234b72fbc781f1d70211cb785b
>> >> | > Author: Cyrill Gorcunov <gorcunov@gmail.com>
>> >> | > Date:   Mon May 12 14:02:22 2008 -0700
>> >> | >
>> >> | >     init: don't lose initcall return values
>> >> | >
>> >> | > has introduced potential buffer overflow by wrong calculation
>> >> | > of string accumulator size.
>> >> | >
>> >> | > Many thanks Andreas Schwab and Geert Uytterhoeven for helping
>> >> | > to catch and fix the bug.
>> >> | >
>> >> | > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
>> >> | > ---
>> >> | >
>> >> | > Index: linux-2.6.git/init/main.c
>> >> | > ===================================================================
>> >> | > --- linux-2.6.git.orig/init/main.c	2008-05-14 17:55:10.000000000
>> >> +0400
>> >> | > +++ linux-2.6.git/init/main.c	2008-05-14 19:11:18.000000000 +0400
>> >> | > @@ -702,7 +702,7 @@ static void __init do_initcalls(void)
>> >> | >
>> >> | >  	for (call = __initcall_start; call < __initcall_end; call++) {
>> >> | >  		ktime_t t0, t1, delta;
>> >> | > -		char msgbuf[40];
>> >> | > +		char msgbuf[64];
>> >> | >  		int result;
>> >> | >
>> >> | >  		if (initcall_debug) {
>> >> | > @@ -729,11 +729,11 @@ static void __init do_initcalls(void)
>> >> | >  			sprintf(msgbuf, "error code %d ", result);
>> >> | >
>> >> | >  		if (preempt_count() != count) {
>> >> | > -			strncat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
>> >> | > +			strcat(msgbuf, "preemption imbalance ");
>> >> | >  			preempt_count() = count;
>> >> | >  		}
>> >> | >  		if (irqs_disabled()) {
>> >> | > -			strncat(msgbuf, "disabled interrupts ", sizeof(msgbuf));
>> >> | > +			strcat(msgbuf, "disabled interrupts ");
>> >> | >  			local_irq_enable();
>> >> | >  		}
>> >> | >  		if (msgbuf[0]) {
>> >> |
>> >> | umm, why can't m68k call strncat() from init/main.c??
>> >> |
>> >>
>> >> there some problem with headers iirc, we have to declare it first or
>> >> use some gcc option (as Adrian suggested). Actually I would prefer to
>> >> use
>> >
>> > gcc turns the strncat() into an implicit call to strlen() and some form
>> > of expanded memcpy(). E.g.
>> >
>> >
>> > 	if (preempt_count() != count) {
>> > 		strncat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
>> > 		preempt_count() = count;
>> > 	}
>> >
>> > becomes
>> >
>> >         cmp.l 884(%a2),%d6      | <variable>.thread.info.preempt_count,
>> > count
>> >         jeq .L61        |
>> >         move.l %d7,-(%sp)       | tmp76,
>> >         jbsr strlen     |
>> >         addq.l #4,%sp   |,
>> >         move.l %d7,%a0  | tmp76, tmp80
>> >         add.l %d0,%a0   |, tmp80
>> >         move.l #1886545253,(%a0)+       |,
>> >         move.l #1836086377,(%a0)+       |,
>> >         move.l #1869488233,(%a0)+       |,
>> >         move.l #1835164012,(%a0)+       |,
>> >         move.l #1634624357,(%a0)+       |,
>> >         move.w #8192,(%a0)      |,* D.28541
>> >         move.l %d6,884(%a2)     | count,
>> > <variable>.thread.info.preempt_count
>> > .L61:
>> >
>> > All other explicit calls to strlen() are inlined, as per
>> > include/asm-m68k/string.h.
>> >
>> >> strlcat there but it seems it would fail to build too. Originally I've
>> >> messed
>> >> strlcat with strncat :(
>> >
>> > Actually it build and runs fine after s/strncat/strlcat/...
>> >
>
> (top-posting repaired)
>
>> Could you please make an update to the patch? I can make it only
>> tomorrow evening (ie not that fast)
>
> Like this?
>
>
> From: Cyrill Gorcunov <gorcunov@gmail.com>
>
> This patch fixes a build bug on m68k - gcc decides to emit a call to the
> strlen library function, which we don't implement.  Use strlcat() instead.
>
>
> What is more important - my previous patch
>
> commit e662e1cfd434aa234b72fbc781f1d70211cb785b
> Author: Cyrill Gorcunov <gorcunov@gmail.com>
> Date:   Mon May 12 14:02:22 2008 -0700
>
>     init: don't lose initcall return values
>
> Has introduced potential buffer overflow by wrong calculation of string
> accumulator size.
>
> Many thanks Andreas Schwab and Geert Uytterhoeven for helping
> to catch and fix the bug.
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
>
>  init/main.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff -puN init/main.c~init-fix-building-bug-and-potential-buffer-overflow
> init/main.c
> --- a/init/main.c~init-fix-building-bug-and-potential-buffer-overflow
> +++ a/init/main.c
> @@ -702,7 +702,7 @@ static void __init do_initcalls(void)
>
>  	for (call = __initcall_start; call < __initcall_end; call++) {
>  		ktime_t t0, t1, delta;
> -		char msgbuf[40];
> +		char msgbuf[64];
>  		int result;
>
>  		if (initcall_debug) {
> @@ -729,11 +729,11 @@ static void __init do_initcalls(void)
>  			sprintf(msgbuf, "error code %d ", result);
>
>  		if (preempt_count() != count) {
> -			strncat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
> +			strlcat(msgbuf, "preemption imbalance ", sizeof msgbuf);
>  			preempt_count() = count;
>  		}
>  		if (irqs_disabled()) {
> -			strncat(msgbuf, "disabled interrupts ", sizeof(msgbuf));
> +			strlcat(msgbuf, "disabled interrupts ", sizeof msgbuf);
>  			local_irq_enable();
>  		}
>  		if (msgbuf[0]) {
> _
>
> (yeah, I normally parenthesise sizeof too, but this provided 80-col
> salvation)
>
>

Re: [PATCH] init - fix building bug and potential buffer overflow

[Cyrill Gorcunov]

On Fri, May 16, 2008 at 7:29 AM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> Yes! Exactly! Thanks, Andrew! (i didn't look into what Linus suggested
> 'cause i just waked up and have to go to my job)
>
[...]

And sorry about top-posting (gmail mobile client is really screwed up,
trying to force top-post all the time)

Re: [PATCH] init - fix building bug and potential buffer overflow

[Geert Uytterhoeven]

On Thu, 15 May 2008, Andrew Morton wrote:
> On Fri, 16 May 2008 00:22:14 +0400
> "Cyrill Gorcunov" <gorcunov@gmail.com> wrote:
> > On 5/15/08, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Thu, 15 May 2008, Cyrill Gorcunov wrote:
> > >> [Andrew Morton - Thu, May 15, 2008 at 10:58:03AM -0700]
> > >> | On Wed, 14 May 2008 19:44:02 +0400 Cyrill Gorcunov <gorcunov@gmail.com>
> > >> wrote:
> > >> strlcat there but it seems it would fail to build too. Originally I've
> > >> messed
> > >> strlcat with strncat :(
> > >
> > > Actually it build and runs fine after s/strncat/strlcat/...
> > >
> 
> (top-posting repaired)
> 
> > Could you please make an update to the patch? I can make it only
> > tomorrow evening (ie not that fast)
> 
> Like this?

Builds and boots fine!

> From: Cyrill Gorcunov <gorcunov@gmail.com>
> 
> This patch fixes a build bug on m68k - gcc decides to emit a call to the
> strlen library function, which we don't implement.  Use strlcat() instead.
> 
> 
> What is more important - my previous patch
> 
> commit e662e1cfd434aa234b72fbc781f1d70211cb785b
> Author: Cyrill Gorcunov <gorcunov@gmail.com>
> Date:   Mon May 12 14:02:22 2008 -0700
> 
>     init: don't lose initcall return values
> 
> Has introduced potential buffer overflow by wrong calculation of string
> accumulator size.
> 
> Many thanks Andreas Schwab and Geert Uytterhoeven for helping
> to catch and fix the bug.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds