init - fix building bug and potential buffer overflow

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 <[email protected]>
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 <[email protected]>

---

Index: linux-2.6.git/init/main.c
===================================================================

init/main.c +1 -1

@@ -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) {

[Andrew Morton - Thu, May 15, 2008 at 10:58:03AM -0700]

| On Wed, 14 May 2008 19:44:02 +0400 Cyrill Gorcunov <[email protected]> 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 <[email protected]>
| > 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 <[email protected]>
| > ---
| > 
| > 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 :(

Quoted 55 lines · Cyrill Gorcunov

[Andrew Morton - Thu, May 15, 2008 at 10:58:03AM -0700]
| On Wed, 14 May 2008 19:44:02 +0400 Cyrill Gorcunov <[email protected]> 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 <[email protected]>
| > 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 <[email protected]>
| > ---
| >
| > 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??
|

Quoted 2 lines

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.

Quoted 3 lines

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 -- [email protected]

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

Quoted 91 lines · Geert Uytterhoeven

Quoted 55 lines · Cyrill Gorcunov

[Andrew Morton - Thu, May 15, 2008 at 10:58:03AM -0700]
| On Wed, 14 May 2008 19:44:02 +0400 Cyrill Gorcunov <[email protected]> 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 <[email protected]>
| > 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 <[email protected]>
| > ---
| >
| > 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??
|

Quoted 2 lines

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.

Quoted 6 lines

Quoted 3 lines

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

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

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

Quoted 3 lines · Andrew Morton

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

Why?

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

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?

Quoted 4 lines

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.

this, basically:

Quoted 2 lines

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

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

Quoted 3 lines

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.

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?

Quoted 96 lines · On Fri, 16 May 2008 00:22:14 +0400 "Cyrill Gorcunov" <[email protected]> wrote:

Quoted 94 lines · Geert Uytterhoeven

Quoted 57 lines · Cyrill Gorcunov

[Andrew Morton - Thu, May 15, 2008 at 10:58:03AM -0700]
| On Wed, 14 May 2008 19:44:02 +0400 Cyrill Gorcunov <[email protected]>
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 <[email protected]>
| > 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 <[email protected]>
| > ---
| >
| > 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??
|

Quoted 3 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

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)

Quoted 2 lines

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 <[email protected]>

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 <[email protected]>
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 <[email protected]>

Cc Geert Uytterhoeven <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 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

init/main.c renamed from init/main.c~init-fix-building-bug-and-potential-buffer-overflow +3 -3

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

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

Quoted 11 lines · On Fri, 16 May 2008 00:22:14 +0400 "Cyrill Gorcunov" <[email protected]> wrote:

Quoted 10 lines · Geert Uytterhoeven

Quoted 6 lines · Cyrill Gorcunov

[Andrew Morton - Thu, May 15, 2008 at 10:58:03AM -0700]
| On Wed, 14 May 2008 19:44:02 +0400 Cyrill Gorcunov <[email protected]> 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)

Quoted 2 lines

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 <[email protected]>

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 <[email protected]>
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 <[email protected]>

Cc Geert Uytterhoeven <[email protected]>

Signed-off-by Andrew Morton <[email protected]>