isdn: icn: Fix stack overflow bug

isdn: icn: Fix stack overflow bug

[Steven Rostedt]

Running randconfig with ktest.pl I hit this bug:

[   16.101158] ICN-ISDN-driver Rev 1.65.6.8 mem=0x000d0000
[   16.106376] icn: (line0) ICN-2B, port 0x320 added
[   16.111064] Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: c1642880
[   16.111066] 
[   16.121214] Pid: 1, comm: swapper Not tainted 2.6.37-rc2-test-00124-g6656b3f #8
[   16.128499] Call Trace:
[   16.130942]  [<c0f51662>] ? printk+0x1d/0x23
[   16.135200]  [<c0f5153f>] panic+0x5c/0x162
[   16.139286]  [<c0d62a9a>] ? icn_addcard+0x6d/0xbe
[   16.143975]  [<c0445783>] print_tainted+0x0/0x8c
[   16.148582]  [<c1642880>] ? icn_init+0xd8/0xdf
[   16.153012]  [<c1642880>] icn_init+0xd8/0xdf
[   16.157271]  [<c04012e5>] do_one_initcall+0x8c/0x143
[   16.162222]  [<c16427a8>] ? icn_init+0x0/0xdf
[   16.166566]  [<c15f1a05>] kernel_init+0x13f/0x1da
[   16.171256]  [<c15f18c6>] ? kernel_init+0x0/0x1da
[   16.175945]  [<c0403bfe>] kernel_thread_helper+0x6/0x10
[   16.181181] panic occurred, switching back to text console

Looking into it I found that the stack was corrupted by the assignment
of the Rev #. The variable rev is given 10 bytes, and in this output the
characters that were copied was: " 1.65.6.8 $". Which was 11 characters
plus the null ending character for a total of 12 bytes, thus corrupting
the stack.

This patch ups the variable size to 20 bytes as well as changes the
strcpy to strncpy. I also added a check to make sure '$' is found.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/drivers/isdn/icn/icn.c b/drivers/isdn/icn/icn.c
index 2e847a9..f2b5bab 100644
--- a/drivers/isdn/icn/icn.c
+++ b/drivers/isdn/icn/icn.c
@@ -1627,7 +1627,7 @@ __setup("icn=", icn_setup);
 static int __init icn_init(void)
 {
 	char *p;
-	char rev[10];
+	char rev[20];
 
 	memset(&dev, 0, sizeof(icn_dev));
 	dev.memaddr = (membase & 0x0ffc000);
@@ -1637,9 +1637,10 @@ static int __init icn_init(void)
 	spin_lock_init(&dev.devlock);
 
 	if ((p = strchr(revision, ':'))) {
-		strcpy(rev, p + 1);
+		strncpy(rev, p + 1, 20);
 		p = strchr(rev, '$');
-		*p = 0;
+		if (p)
+			*p = 0;
 	} else
 		strcpy(rev, " ??? ");
 	printk(KERN_NOTICE "ICN-ISDN-driver Rev%smem=0x%08lx\n", rev,

Re: [PATCH] isdn: icn: Fix stack overflow bug

[Steven Rostedt]

Note, the subject should really be:

[PATCH] isdn: icn: Fix stack corruption bug.

-- Steve

Re: [PATCH] isdn: icn: Fix stack overflow bug

[David Miller]

From: Steven Rostedt <rostedt@goodmis.org>
Date: Tue, 23 Nov 2010 23:46:27 -0500

> 
> Note, the subject should really be:
> 
> [PATCH] isdn: icn: Fix stack corruption bug.

Applied.

Please CC: isdn patches to netdev@vger.kernel.org in the future as per
"ISDN SUBSYSTEM" in MAINTAINERS.  Patches sent only to the ISDN list
and Karsten are likely to not get looked at for weeks if not longer.

Also, printing out the driver version is not only a waste of text and
data, it's unnecessary (MODULE_VERSION).  So it should be killed off
at some point.

Re: [PATCH] isdn: icn: Fix stack overflow bug

[Steven Rostedt]

On Wed, 2010-11-24 at 11:20 -0800, David Miller wrote:
> From: Steven Rostedt <rostedt@goodmis.org>
> Date: Tue, 23 Nov 2010 23:46:27 -0500
> 
> > 
> > Note, the subject should really be:
> > 
> > [PATCH] isdn: icn: Fix stack corruption bug.
> 
> Applied.

Thanks,

> 
> Please CC: isdn patches to netdev@vger.kernel.org in the future as per
> "ISDN SUBSYSTEM" in MAINTAINERS.  Patches sent only to the ISDN list
> and Karsten are likely to not get looked at for weeks if not longer.

OK, will do.

That's what I get for sending patches when I'm tired. I only saw the
first mailing list. I didn't even notice the netdev one. And I also
forgot to add the [PATCH] to the original subject.

I wrote the patch while still awake, but ran it through a few more tests
just to make sure it worked. Forgot about it, and then when I was done
with my normal job, I decided to send it out (making change log and
everything).

-- Steve