diff mbox

isdnloop: several buffer overflows

Message ID 20140408092309.GA26450@mwanda
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Dan Carpenter April 8, 2014, 9:23 a.m. UTC 
There are three buffer overflows addressed in this patch.

1) In isdnloop_fake_err() we add an 'E' to a 60 character string and
then copy it into a 60 character buffer.  I have made the destination
buffer 64 characters and I'm changed the sprintf() to a snprintf().

2) In isdnloop_parse_cmd(), p points to a 6 characters into a 60
character buffer so we have 54 characters.  The ->eazlist[] is 11
characters long.  I have modified the code to return if the source
buffer is too long.

3) In isdnloop_command() the cbuf[] array was 60 characters long but the
max length of the string then can be up to 79 characters.  I made the
cbuf array 80 characters long and changed the sprintf() to snprintf().
I also removed the temporary "dial" buffer and changed it to use "p"
directly.

Unfortunately, we pass the "cbuf" string from isdnloop_command() to
isdnloop_writecmd() which truncates anything over 60 characters to make
it fit in card->omsg[].  (It can accept values up to 255 characters so
long as there is a '\n' character every 60 characters).  For now I have
just fixed the memory corruption bug and left the other problems in this
driver alone.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

David Laight April 8, 2014, 9:34 a.m. UTC | #1 
From: Dan Carpenter
> There are three buffer overflows addressed in this patch.
...
> 2) In isdnloop_parse_cmd(), p points to a 6 characters into a 60
> character buffer so we have 54 characters.  The ->eazlist[] is 11
> characters long.  I have modified the code to return if the source
> buffer is too long.
...
> @@ -903,6 +903,8 @@ isdnloop_parse_cmd(isdnloop_card *card)
>  	case 7:
>  		/* 0x;EAZ */
>  		p += 3;
> +		if (strlen(p) >= sizeof(card->eazlist[0]))
> +			break;
>  		strcpy(card->eazlist[ch - 1], p);
>  		break;
>  	case 8:

If you've done the strlen() you might as well use memcpy().
There are also functions that will do a bounded strlen(),
(eg memchr()).

	David



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Carpenter April 8, 2014, 11:02 a.m. UTC | #2 
On Tue, Apr 08, 2014 at 09:34:09AM +0000, David Laight wrote:
> From: Dan Carpenter
> > There are three buffer overflows addressed in this patch.
> ...
> > 2) In isdnloop_parse_cmd(), p points to a 6 characters into a 60
> > character buffer so we have 54 characters.  The ->eazlist[] is 11
> > characters long.  I have modified the code to return if the source
> > buffer is too long.
> ...
> > @@ -903,6 +903,8 @@ isdnloop_parse_cmd(isdnloop_card *card)
> >  	case 7:
> >  		/* 0x;EAZ */
> >  		p += 3;
> > +		if (strlen(p) >= sizeof(card->eazlist[0]))
> > +			break;
> >  		strcpy(card->eazlist[ch - 1], p);
> >  		break;
> >  	case 8:
> 
> If you've done the strlen() you might as well use memcpy().
> There are also functions that will do a bounded strlen(),
> (eg memchr()).
> 

I re-wrote the patch based on your suggestion but decided that I prefer
the original just because the diff is smaller.  This is a driver that no
one uses and it's full of bugs.  Let's not worry about optimizing the
slow paths at this point.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller April 8, 2014, 4:42 p.m. UTC | #3 
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Tue, 8 Apr 2014 12:23:09 +0300

> There are three buffer overflows addressed in this patch.
> 
> 1) In isdnloop_fake_err() we add an 'E' to a 60 character string and
> then copy it into a 60 character buffer.  I have made the destination
> buffer 64 characters and I'm changed the sprintf() to a snprintf().
> 
> 2) In isdnloop_parse_cmd(), p points to a 6 characters into a 60
> character buffer so we have 54 characters.  The ->eazlist[] is 11
> characters long.  I have modified the code to return if the source
> buffer is too long.
> 
> 3) In isdnloop_command() the cbuf[] array was 60 characters long but the
> max length of the string then can be up to 79 characters.  I made the
> cbuf array 80 characters long and changed the sprintf() to snprintf().
> I also removed the temporary "dial" buffer and changed it to use "p"
> directly.
> 
> Unfortunately, we pass the "cbuf" string from isdnloop_command() to
> isdnloop_writecmd() which truncates anything over 60 characters to make
> it fit in card->omsg[].  (It can accept values up to 255 characters so
> long as there is a '\n' character every 60 characters).  For now I have
> just fixed the memory corruption bug and left the other problems in this
> driver alone.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied and queued up for -stable, thanks.

This thing is marked broken-on-smp so the impact is very close to zero.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tilman Schmidt April 10, 2014, 11:16 a.m. UTC | #4 
On Tue, Apr 8, 2014, at 18:42, David Miller wrote:
> 
> Applied and queued up for -stable, thanks.
> 
> This thing is marked broken-on-smp so the impact is very close to zero.

isdnloop is a loopback test driver for the deprecated old ISDN4Linux
subsystem.
I doubt anyone is still interested in it.
diff mbox

Patch

diff --git a/drivers/isdn/isdnloop/isdnloop.c b/drivers/isdn/isdnloop/isdnloop.c
index e1f8748..5a4da94 100644
--- a/drivers/isdn/isdnloop/isdnloop.c
+++ b/drivers/isdn/isdnloop/isdnloop.c
@@ -518,9 +518,9 @@  static isdnloop_stat isdnloop_cmd_table[] =
 static void
 isdnloop_fake_err(isdnloop_card *card)
 {
-	char buf[60];
+	char buf[64];
 
-	sprintf(buf, "E%s", card->omsg);
+	snprintf(buf, sizeof(buf), "E%s", card->omsg);
 	isdnloop_fake(card, buf, -1);
 	isdnloop_fake(card, "NAK", -1);
 }
@@ -903,6 +903,8 @@  isdnloop_parse_cmd(isdnloop_card *card)
 	case 7:
 		/* 0x;EAZ */
 		p += 3;
+		if (strlen(p) >= sizeof(card->eazlist[0]))
+			break;
 		strcpy(card->eazlist[ch - 1], p);
 		break;
 	case 8:
@@ -1133,7 +1135,7 @@  isdnloop_command(isdn_ctrl *c, isdnloop_card *card)
 {
 	ulong a;
 	int i;
-	char cbuf[60];
+	char cbuf[80];
 	isdn_ctrl cmd;
 	isdnloop_cdef cdef;
 
@@ -1198,7 +1200,6 @@  isdnloop_command(isdn_ctrl *c, isdnloop_card *card)
 			break;
 		if ((c->arg & 255) < ISDNLOOP_BCH) {
 			char *p;
-			char dial[50];
 			char dcode[4];
 
 			a = c->arg;
@@ -1210,10 +1211,10 @@  isdnloop_command(isdn_ctrl *c, isdnloop_card *card)
 			} else
 				/* Normal Dial */
 				strcpy(dcode, "CAL");
-			strcpy(dial, p);
-			sprintf(cbuf, "%02d;D%s_R%s,%02d,%02d,%s\n", (int) (a + 1),
-				dcode, dial, c->parm.setup.si1,
-				c->parm.setup.si2, c->parm.setup.eazmsn);
+			snprintf(cbuf, sizeof(cbuf),
+				 "%02d;D%s_R%s,%02d,%02d,%s\n", (int) (a + 1),
+				 dcode, p, c->parm.setup.si1,
+				 c->parm.setup.si2, c->parm.setup.eazmsn);
 			i = isdnloop_writecmd(cbuf, strlen(cbuf), 0, card);
 		}
 		break;