isdn: icn: buffer overflow in icn_command()

The cbuf[] buffer is 60 characters but we're putting a potential 79 characters and a NUL into it. I've made it 80 characters and changed the sprintf() to snprintf().

Quoted 3 lines · Dan Carpenter

The cbuf[] buffer is 60 characters but we're putting a potential 79 characters and a NUL into it. I've made it 80 characters and changed the sprintf() to snprintf().

[]

drivers/isdn/icn/icn.c icn_command(isdn_ctrl *c, icn_card *card) · line 1312

Quoted 6 lines

drivers/isdn/icn/icn.c +0 -1

@@ -1309,7 +1309,6 @@ icn_command(isdn_ctrl *c, icn_card *card)
 			break;
 		if ((c->arg & 255) < ICN_BCH) {
 			char *p;
-			char dial[50];
 			char dcode[4];

The change log does not mentioned removal of dial.

As this subsystem likely has 0 active users, perhaps making the minimal correctness change might be better.

Also, if making other changes, perhaps it'd be better to similarly replace dcode with a pointer as well.

Quoted 4 lines

drivers/isdn/icn/icn.c +0 -0

@@ -1321,10 +1320,10 @@ icn_command(isdn_ctrl *c, icn_card *card)
 			} else
 				/* Normal Dial */

drivers/isdn/icn/icn.c icn_command(isdn_ctrl *c, icn_card *card) · line 1327

Quoted 10 lines

drivers/isdn/icn/icn.c +4 -4

 				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 = icn_writecmd(cbuf, strlen(cbuf), 0, card);

Maybe save the snprintf result length and use it
in icn_writecmd too.

snprintf() returns the number of bytes which would have been printed if there were enough space and not the number of bytes in the string. Using the value from snprintf() would not introduce a bug because I have carefully counted the number of bytes in the output string, but it would hopefully annoy human auditors of this code. ;) You are thinking of scnprintf().

I'm going to apply your minimal changes suggestion here.

This buffer over was detected using static analysis:

	drivers/isdn/icn/icn.c:1325 icn_command()
	error: format string overflow. buf_size: 60 length: 98

The calculation for the length of the string is off because it assumes that the dial[] buffer holds a 50 character string, but actually it is at most 31 characters and NUL. I have removed the dial[] buffer because it isn't needed.

The maximum length of the string is actually 79 characters and a NUL. I have made the cbuf[] array large enough to hold it and changed the sprintf() to an snprintf() as a further safety enhancement.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: update changelog.

drivers/isdn/icn/icn.c +1 -3

index 53d487f..88c0603 100644
@@ -1155,7 +1155,7 @@ icn_command(isdn_ctrl *c, icn_card *card)
 	ulong a;
 	ulong flags;
 	int i;
-	char cbuf[60];
+	char cbuf[80];
 	isdn_ctrl cmd;
 	icn_cdef cdef;
 	char __user *arg;

@@ -1309,7 +1309,6 @@ icn_command(isdn_ctrl *c, icn_card *card)
 			break;
 		if ((c->arg & 255) < ICN_BCH) {
 			char *p;
-			char dial[50];
 			char dcode[4];
 
 			a = c->arg;

@@ -1321,10 +1320,10 @@ icn_command(isdn_ctrl *c, icn_card *card)
 			} else
 				/* Normal Dial */
 				strcpy(dcode, "CAL");
-			strcpy(dial, p);

This buffer over was detected using static analysis:

	drivers/isdn/icn/icn.c:1325 icn_command()
	error: format string overflow. buf_size: 60 length: 98

The calculation for the length of the string is off because it assumes that the dial[] buffer holds a 50 character string, but actually it is at most 31 characters and NUL. I have removed the dial[] buffer because it isn't needed.

The maximum length of the string is actually 79 characters and a NUL. I have made the cbuf[] array large enough to hold it and changed the sprintf() to an snprintf() as a further safety enhancement.

Signed-off-by Dan Carpenter <[email protected]>